-
Notifications
You must be signed in to change notification settings - Fork 26
build: patch osbuild; set img ref properly #201
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
build: patch osbuild; set img ref properly #201
Conversation
|
Opening this PR to see if I can get some feedback on if it works or not. |
Rather than write into container-storage and then export to an oci-archive let's just go straight there. Signed-off-by: Dusty Mabe <[email protected]>
06bc4d5 to
2154b33
Compare
6a2b2f1 to
18ee4ee
Compare
We need to patch OSBuild with patches from upstream PR [1] to allow us to set the imgref to the value we need and not have the image building fail with an error. [1] osbuild/osbuild#2270 Signed-off-by: Dusty Mabe <[email protected]>
18ee4ee to
0fd9ff4
Compare
We currently set the imgref to something we cannot use later with bootc or rpm-ostree. This should fix the reference and allow `bootc upgrade` to automatically work. Signed-off-by: Brent Baude <[email protected]>
0fd9ff4 to
8383444
Compare
|
I'll mark this as ready for review once osbuild/osbuild#2270 merges |
|
This dupes and extends #200 so i will close the previous PR as such. |
merged upstream! |
baude
left a 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 ... at what point do you think we can stop patching? any chance there is backports going on or is it the next version.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, dustymabe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| echo "Patching OSBuild" | ||
| dnf install -y --quiet patch | ||
| patch_osbuild |
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.
how urgent is this here?
I rather not see this merged until the fixes land properly in the main repos. Any runtime install like this is prone to introduce plenty of flakes.
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.
how urgent is this here?
That's a question for your team to answer.
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.
@Luap99 this is holding up my work on updates
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.
If you need this now sure, as long as we agree that we don't make the final 6.0 release with this I am good merging.
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.
I absolutely agree with you. But I will leave the merge up to you.
Probably mid-january. If you wanted to open a PR to backport the patch to current OSBuild RPM then maybe faster. When the new version of OSBuild makes it to the repos this patch would start failing. That's your cue to drop the patching (can probably just revert the commit that introduced it). |
|
/lgtm |
|
This broke CI I believe, the publish task can no longer push the images to quay https://cirrus-ci.com/task/4738727450247168 I guess the removal of |
It has nothing to do with the archive I don't guess. The log you linked is trying to build a manifest from containers-storage. I didn't realize there was a later step that ran that leveraged the images from containers-storage. We can just revert 16ff701 then. Sorry about that. |
No it is not about the local storage, the build tasks uploads the oci archive and the the publish task downloads the archive and all the other artifacts of the build and loads the archive as image so we can add it to the manifest list and then push that to quay.io. My guess (untested) is that the archive creation does not add the image name metadata adn as such the load doesn't restore the name. If the direct creation via rpm-ostree is faster I think we should keep that. Restoring the name is cheap and simple enough I assume, just an extra step after load. |
We currently set the imgref to something we cannot use later with bootc
or rpm-ostree. This should fix the reference and allow
bootc upgradeto automatically work.
In service of the above goal, backport a few patches from osbuild upstream.