-
Notifications
You must be signed in to change notification settings - Fork 184
src/konflux-rpm-lockfile: pull $releasever from manifest #4344
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
src/konflux-rpm-lockfile: pull $releasever from manifest #4344
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates konflux-rpm-lockfile
to use the $releasever
from the manifest file, which is a good improvement for reproducibility. I've added a review comment to make this more robust. Currently, if releasever
is missing from the manifest, the script silently falls back to using the host's version. My suggestion is to add a check to ensure releasever
is present and fail if it's not, to avoid this unintended behavior.
8cf746b
to
69e6776
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly modifies konflux-rpm-lockfile
to use the releasever
from the manifest, preventing potential desynchronization with the host system. The logic is sound. I've identified a minor issue with a print statement, including a typo, an indentation error, and a suggestion to use stderr
for warning messages. Please see the detailed comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See gemini comment
Make sure to not rely on manifest-provided $releasever to resolves packages to avoid a desync. For RHEL we don't use releasever so using the host's value have no incidence, so no need to throw an error if it's missing.
69e6776
to
a595a5e
Compare
Updated and tested locally for next-devel (where the local install is F42 and we are resolving f43) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Make sure to not rely on the host $releasever to resolves the packages to avoid a desync