Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add CONTEST_CONTENT_PR and CONTEST_OSCAP_PR #127

Merged
merged 1 commit into from
Mar 26, 2024
Merged

add CONTEST_CONTENT_PR and CONTEST_OSCAP_PR #127

merged 1 commit into from
Mar 26, 2024

Conversation

comps
Copy link
Contributor

@comps comps commented Mar 21, 2024

Fixes #124

Comment on lines -8 to -9
adjust+:
- prepare+:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This is just moving all the code from adjust to root-level prepare, since there was no need for the adjust (no condition specified).

@comps comps marked this pull request as ready for review March 21, 2024 16:58
adjust:
- prepare:
- prepare+:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why +? It doesn't inherit from top level, leaving prepare: would make it clean for reader no additional prepare steps coming from top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember thinking the same and testing it, and it removed the other prepare content.

What I think is happening is that adjust is being applied on top of regular dictionary keys, so if /plans/foobar.fmf specifies prepare:, it gets used first, and then all the adjusts are applied, starting from the top-level /plans/main.fmf.

if ! rpm -q epel-release; then
curl -o epel-release.rpm --retry 10 https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm
curl --retry 10 -sSfkL -o epel-release.rpm https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer long form options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I do too, but we use curl short options on many places (at least internally) to the point where it became sort of a habit. As in - one should never use curl without these (for "gimme what's on that URL" style of use cases).

Writing it out would be a lot longer and need line breaks,

curl --output epel-release.rpm --retry 10 --silent --show-error --fail --insecure --location https://dl.fedoraproject.org/pub/epel/epel-release-latest-7.noarch.rpm

especially -o (as an alternative to wget) I've never seen expanded to --output.

I modified this line only because I added the extra curl options elsewhere and thought it's a good idea to keep the options consistent across plans.

So I personally prefer the short options for curl, but I can use the long ones if it matters to you.

@mildas mildas merged commit 0805ae8 into main Mar 26, 2024
@mildas mildas deleted the add_pr_testing branch March 26, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support testing CaC/content and openscap build from pull requests
2 participants