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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃 Add simple UKI test #1619

Merged
merged 2 commits into from
Jul 20, 2023
Merged

馃 Add simple UKI test #1619

merged 2 commits into from
Jul 20, 2023

Conversation

Itxaka
Copy link
Member

@Itxaka Itxaka commented Jul 19, 2023

Signed-off-by: Itxaka itxaka.garcia@spectrocloud.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>
Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>
@Itxaka Itxaka marked this pull request as ready for review July 19, 2023 12:23
@Itxaka Itxaka linked an issue Jul 19, 2023 that may be closed by this pull request
@Itxaka Itxaka requested a review from a team July 19, 2023 18:43
Comment on lines +85 to +86
stateContains(vm, "system.os.name", "alpine", "opensuse", "ubuntu", "debian", "fedora")
stateContains(vm, "kairos.flavor", "alpine", "opensuse", "ubuntu", "debian", "fedora")
Copy link
Member

Choose a reason for hiding this comment

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

how come we need to have here all the different flavors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a generic test that check that is indeed on of the flavors of the lost so if we change or add extra flavors for the test in the future it doesn't need a test change

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, I guess ideally we should pass it via an env var or pick it from the uki file name or something like that, but can be improved later on

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, this was mainly copied fromt he existing acceptance test. Its kind of generic and could be more specific.

Copy link
Member

@mauromorales mauromorales left a comment

Choose a reason for hiding this comment

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

lgtm

@Itxaka Itxaka merged commit ee22b45 into kairos-io:master Jul 20, 2023
23 checks passed
@Itxaka Itxaka deleted the uki_test branch July 20, 2023 13:51
},
// UKI boot
func(m *types.MachineConfig) error {
drive := os.Getenv("UKI_DRIVE")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late comment. I think the test below can easily turn to false positive if one omits these 2 environment variables. A quick solution would be to check in the test that these 2 environment variables are set (like we do for CONTAINER_IMAGE in the upgrade test)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I dont think so. If we do not set up this drive and only the iso, it will probably boot from the iso but the first test will fail as it wont detect that it has booted in UKI mode.

So no matter iof you skip this, your UKI test wont pass if you boot from anything but a UKI artifact.

But I agree, a check in there with a decent failure message before starting the VM would be much nicer

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.

Create a test that boots UKI artifacts
3 participants