Customization: Add a empty boolean to ignition#46
Conversation
This allow setting a `Firstboot.Ignition.Enable` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
supakeen
left a comment
There was a problem hiding this comment.
You can undraft this one. The change is minor and a precondition to your change in images.
There's no real good solution here in regards to the boolean since historically we've used the 'non-empty URL' as meaning something is enabled (but now we want to enable without a URL) so just handling it in images would be perfectly fine to me 🙂
pkg/blueprint/customizations.go
Outdated
|
|
||
| type FirstBootIgnitionCustomization struct { | ||
| ProvisioningURL string `json:"url,omitempty" toml:"url,omitempty"` | ||
| Enable bool `json:"enable,omitempty" toml:"enable,omitempty"` |
There was a problem hiding this comment.
Hmm, this will default to false, no? So all existing blueprints would have to be updated to set enable = true.
There was a problem hiding this comment.
Depends how we handle this in images. If we consider the passing of ProvisioningURL to mean that things are enabled then pre-existing blueprints don't need to change and we only grow a new capability (enabling firstboot without a provisioning URL).
The companion PR in images seems to do exactly that: osbuild/images#2222 and the PR description mentions doing so explicitly as well.
There was a problem hiding this comment.
There was a problem hiding this comment.
The description says it'll enable automatically if a URL is given, so I guess the behaviour would be the same for any existing blueprint.
It's a bit of a weird option in general though. I get the backstory and I understand the behaviour, but that's because I know where this came from and why the PR was made. But having a story or need for an option doesn't make it good UI design. We're adding this now to enable the scenario where you don't want a provisioning URL but want the file on disk, so:
ProvisioningURL = ""andEnable = true
But the change introduces more combinations:
ProvisioningURL = "$url"and explicitly settingEnable = falsewouldn't do what the user expects if they're interpreting the wordEnablewithout understanding the reason behind its introduction. You might say there's no reason to set a URL and not enable the customization, it doesn't make sense, so why would anyone do that? To which I'd say, why does the configuration format support it?
Perhaps this can be solved a different way. My mind jumps to having a keyword for the provisioning URL that is clearly not a URL, like [DEFAULT] or [EMPTY], but magic strings aren't much of an improvement. We could change the name of the new option to convey it's meaning better. Something like CreateDefault or CreateEmpty (or just default = true or empty = true) and make the option be mutually exclusive with provisioning_url.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I quite like @achilleas-k's idea of renaming the option to empty and making it mutually exclusive with provisioning_url.
You could add validation to GetIgnition() to enforce that: https://github.com/osbuild/blueprint/blob/main/pkg/blueprint/customizations.go#L337-L342.
There was a problem hiding this comment.
Perhaps this can be solved a different way.
The most intuitive approach would be to allow an empty string for ProvisionningUrl and write nothing in the file IMHO
There was a problem hiding this comment.
Right, it'd be possible to change it to a pointer so we can differentiate. I'll let @achilleas-k / @croissanne chime in.
Personally I prefer the mutually exclusive empty option if we're changing this as it might work better with potential future options (if any) where empty is always exclusive.
There was a problem hiding this comment.
Perhaps this can be solved a different way.
The most intuitive approach would be to allow an empty string for
ProvisionningUrland write nothing in the file IMHO
Crossed my mind too, but I agree with Simon about the possible expansion into other options and how that might change behaviour in the future.
Let's go with an explicit empty = true|false and make it an error to enable alongside provisioning URL.
There was a problem hiding this comment.
I updated the PR with an empty parameter and added a few tests for the mutually exclusive condition
Add a simple `empty` bool to allow trigerring the osbuild stage without any parameters. This is used for CoreOS where we only need that file to exist in `/boot` without caring about the content. The boolean is mutually exclusive with the existing `url` parameter. See https://issues.redhat.com/browse/HMS-10227
6e44414 to
47942c4
Compare
enable boolean to ignitionempty boolean to ignition
bcl
left a comment
There was a problem hiding this comment.
Ack. This also needs documentation added to https://github.com/osbuild/osbuild.github.io/
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Note that we specify the target directory to create the `ignition.firstboot` stamp, as introduced in [2]. This is required because `bootc install to-filesystem` wants an empty /boot FS, so we need to run the stage after. We reuse the bootupd mounts generator. We really only need /boot, so bootupd is probably a bit overkill but does the job. We cannot reuse the existing mounts here because the `ostree` generated mounts are shadowing /boot so the file end up in the wrong place. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition [2] osbuild/osbuild#2149 Signed-off-by: jbtrystram <jbtrystram@redhat.com> Co-authored-by: Simon de Vlieger <cmdr@supakeen.com>
This allow setting a `Firstboot.Ignition.Empty` boolean to inject the igntition firstboot stage[1] after the container is deployed. Note that we specify the target directory to create the `ignition.firstboot` stamp, as introduced in [2]. This is required because `bootc install to-filesystem` wants an empty /boot FS, so we need to run the stage after. We reuse the bootupd mounts generator. We really only need /boot, so bootupd is probably a bit overkill but does the job. We cannot reuse the existing mounts here because the `ostree` generated mounts are shadowing /boot so the file end up in the wrong place. Requires osbuild/blueprint#46 [1] https://github.com/osbuild/osbuild/blob/main/stages/org.osbuild.ignition [2] osbuild/osbuild#2149 Signed-off-by: jbtrystram <jbtrystram@redhat.com> Co-authored-by: Simon de Vlieger <cmdr@supakeen.com>
Add a simple
enablebool to allow trigerring the osbuild stage without any parameters. This is used for CoreOS where we only need that file to exist in/bootwithout caring about the content.Obviously, this boolean will be set to true if an URL is given. See https://issues.redhat.com/browse/HMS-10227