-
Notifications
You must be signed in to change notification settings - Fork 55
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 support for building images from ostree containers #272
Add support for building images from ostree containers #272
Conversation
268143e
to
1733cb3
Compare
Needs #278 |
af6af76
to
621df93
Compare
Reverted the upgrade of the CI runners to F39. Looked like there might be some misconfiguration with the x86 image. We can investigate separately. |
621df93
to
090c216
Compare
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.
Thanks for this PR - I added some comments and nitpicks inline but a lot of it it personal preference and/or comemnts due to my ignorance. I hope it's still useful.
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.
Two small followup comments, still approved of course.
090c216
to
ee902e9
Compare
@mvo5 I went all in on the |
ee902e9
to
b2dd7a2
Compare
Creates an org.osbuild.ostree.deploy.container stage. Validates the options with a regular expression that matches the schema. Takes a container as input. Validates the length of the input references to be exactly 1. Also added tests for the validators for options and inputs. See osbuild/osbuild#1402
The pipeline either way only supported one ostree commit, enforced when adding the resolved commits to the pipeline during serializeStart() and by checking the array length during serialize(). To simplify the requirement and avoid potential confusion when reading the struct itself, replace the array on the pipeline struct definition with a single commit spec (pointer) and keep the len == 1 check in the serializeStart() function that, by its interface, must accept an array. Also document the commit fields for OSTreeDeployment. These are private, so they wont show up in public API docs, but it's still useful to have them documented in the code.
Support a container source for OSTreeDeployment as an alternative to the ostree commit source. Currently, if a container is defined, it fails with an error. The commit source and the container source are now both pointers since they can be unset, but (will) we need to make sure at least one is set when serializing.
Move the ostree pull stage to be right before the ostree deploy stage. This should have no functional impact on the build and will make it easier to replace the two stages together. Old vs new stage order: org.osbuild.ostree.init-fs | org.osbuild.ostree.init-fs org.osbuild.ostree.pull | org.osbuild.ostree.os-init org.osbuild.ostree.os-init | org.osbuild.mkdir org.osbuild.mkdir | org.osbuild.ostree.pull org.osbuild.ostree.deploy | org.osbuild.ostree.deploy
When creating the ostree deployment pipeline, add the appropriate stages only when the ostree commit is specified. The stages are: org.osbuild.pull org.osbuild.ostree.deploy org.osbuild.ostree.remote org.osbuild.ostree.fillvar
Rename the NewOSTreeDeployment() constructor to NewOSTreeCommitDeployment() and add a second one called NewOSTreeContainerDeployment(). Each takes a different kind of source spec (ostree commit or container). The container constructor also requires the ref to be specified since it's not part of the container spec.
Add the org.osbuild.ostree.deploy.container stage when the content source for the deployment is a container.
Support both content source types on the OSTreeDiskImage ImageKind. A second constructor is defined, just like with the deployment pipeline of the image. The original constructor is renamed to NewOSTreeDiskImageFromCommit to differentiate and have a more informative name.
Only required properties should be specified through the constructor to make sure they are not accidentally left empty or nil. Optional properties are added via public fields of the pipeline object. Since the ignition platform is optional, change it to a public field. Also, to simplify the interaction between the two ignition properties, remove the boolean that signifies if ignition should be enabled and use the IgnitionPlatform string instead. Treat the empty string as the disabled value.
We always aliased these types so that internal changes wouldn't always require changing our tooling interfaces but it's actually more annoying needing to change them every time something is added or changed in the internal types. These are development and testing tools. It's fine to use the internal types directly and keep them simple.
Use gpgkeys instead of concatenating multiple keys under gpgkey. The singular gpgkey was a leftover from an old format and now we use an array from the internal rpmmd.RepoConfig instead.
Not all image types with ostree deployments will want to lock the root user account. Let's instead make it an option of the image function so that each image kind can set it itself. This change has no effect on any of the current image types / manifests.
Not all image types that use ignition want to define the kernel option. Specifically, the (upcoming) CoreOS images wont want to enable this, so let's move it into the image function and add it when the ignition options are added. It is conditioned on the distro version for now, so we can't add it to the global image type kernel options. This change has no effect on any of the current image types / manifests.
b2dd7a2
to
a34dcd5
Compare
Handle the two sets of deployment stages in two separate functions to improve readability. Co-Authored-By: Michael Vogt <[email protected]>
We need this to get F39 CI runners but we're not switching them just yet because of a potential small issue.
a34dcd5
to
a91eafe
Compare
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.
Awesome.
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.
Thank you for this PR! It looks good to merge, I made a bunch of inline comments with ideas/suggestions/questions but all totally optional.
Cherry-picked commits from #243 without the CoreOS image.
This PR adds support for the org.osbuild.ostree.deploy.container stage and addresses the relevant comments from #243. The other PR will be rebased to only add the CoreOS image after this is merged.
This PR doesn't define a new image type, we will instead instantiate the manifest directly as a separate project, in the new repository https://github.com/osbuild/osbuild-deploy-container/.
Part of HMS-3121.