Skip to content
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

Make sure the default kickstart path is used when the option is unset #764

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

achilleas-k
Copy link
Member

image: allow unset kickstart options where possible

For the AnacondaContainerInstaller, having empty kickstart options when
creating the ImageKind should be possible. The only option that's
required is the Kickstart.Path, which has a default value we can set.

For the AnacondaOSTreeInstaller, at least the OSTree options are
required, so return an error when they're not set, but also set the
Kickstart.Path to the default when it's not already set from the caller.


image: drop InteractiveDefaultKickstart in AnacondaContainerInstaller

This isn't used and can cause unnecessary errors when 'img.Kickstart' is
not set.


test: instantiate and serialize anaconda manifests without ks options

Test that anaconda manifests can be generated without error when
kickstart options are not set (or when only the required ones are set).
This ensures that no errors or panics occur when the kickstart options
are nil or when the kickstart path is empty.


Fixes #763

@achilleas-k
Copy link
Member Author

We should have a lot more tests like these and also reconsider what we set in each constructor. I always liked the idea of having, for example, the NewAnacondaContainerInstaller() take as args all the properties it requires and exposing all other properties publicly so they can be set optionally from the caller. An alternative idea, from @mvo5, was to have the optional properties set by an options struct that's always the last argument and can be nil.

Either way, we sort of forgot about this in some places and I see cases where a required property isn't part of the constructor argument list, making the pipeline generator hard to work with; looking at the struct, you'd have to guess which properties are required and which aren't.

A lot of this will probably go away with otk, so it might not be worth investing into cleaning it all up now.

mvo5
mvo5 previously approved these changes Jul 3, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is nice, some small nitpicks/suggestions inline for your consideration

pkg/image/anaconda_ostree_installer.go Show resolved Hide resolved
pkg/image/installer_image_test.go Outdated Show resolved Hide resolved
@achilleas-k
Copy link
Member Author

Need to get #779 in first, then will rebase this.

achilleas-k and others added 3 commits July 3, 2024 20:32
For the AnacondaContainerInstaller, having empty kickstart options when
creating the ImageKind should be possible.  The only option that's
required is the Kickstart.Path, which has a default value we can set.

For the AnacondaOSTreeInstaller, at least the OSTree options are
required, so return an error when they're not set, but also set the
Kickstart.Path to the default when it's not already set from the caller.
This isn't used and can cause unnecessary errors when 'img.Kickstart' is
not set.
Test that anaconda manifests can be generated without error when
kickstart options are not set (or when only the required ones are set).
This ensures that no errors or panics occur when the kickstart options
are nil or when the kickstart path is empty.

Co-authored-by: Michael Vogt <michael.vogt@gmail.com>
@achilleas-k achilleas-k added this pull request to the merge queue Jul 4, 2024
Merged via the queue into osbuild:main with commit dcee98e Jul 4, 2024
16 of 17 checks passed
@achilleas-k achilleas-k deleted the fix/unset-kickstart-path branch July 4, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unset AnacondaContainerInstaller.Kickstart.Path returns an error
3 participants