-
Notifications
You must be signed in to change notification settings - Fork 184
cmd-build-with-buildah: remove temporary stream label hack #4351
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
base: main
Are you sure you want to change the base?
Conversation
Remove the temporary workaround that manually set the `fedora-coreos.stream` label during image builds. This label is now applied directly by rpm-ostree through the build process, making the extra logic unnecessary and simplifying `cmd-build-with-buildah`. See: coreos#4337
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 removes a temporary workaround from cmd-build-with-buildah that manually set the fedora-coreos.stream label. As this functionality is now handled by rpm-ostree, this change correctly simplifies the build script by removing unnecessary code. The change is a good cleanup and improves maintainability.
|
@Roshan-R: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| stream=$(yaml2json "$manifest" /dev/stdout | jq -r '.variables.stream') | ||
| if [ "${stream}" != null ]; then | ||
| set -- "$@" --label fedora-coreos.stream="$stream" \ | ||
| --annotation fedora-coreos.stream="$stream" |
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.
mhmm is the annotation used somewhere ? we must be careful not to break anything relying on it
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.
in other words, as part of this and coreos/fedora-coreos-config#3867 we are retaining the label, but losing the annotation, correct?
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.
in other words, as part of this and coreos/fedora-coreos-config#3867 we are retaining the label, but losing the annotation, correct?
Yes, Exactly
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.
and I'm guessing we need to do something like https://github.com/coreos/rpm-ostree/pull/5454/files but for --annotation if we wanted to keep it, which is probably what we should do
Remove the temporary workaround that manually set the
fedora-coreos.streamlabel during image builds. This label is now applied directly by rpm-ostree through the build process, making the extra logic unnecessary and simplifyingcmd-build-with-buildah.See: #4337
Should be able to merge when we get coreos/fedora-coreos-config#3867 merged