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

many: allow systems installed via installer API to pick optional snaps and components #14426

Merged

Conversation

andrewphelpsj
Copy link
Member

This PR ties together work that enables users to pick which snaps and components to install from a set of available containers.

@github-actions github-actions bot added Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run Nested -auto- Label automatically added in case nested tests need to be executed labels Aug 27, 2024
@andrewphelpsj andrewphelpsj force-pushed the optional-install-api-impl branch 2 times, most recently from 3870e0b to 5eba35a Compare August 28, 2024 19:05
@andrewphelpsj andrewphelpsj marked this pull request as ready for review August 28, 2024 19:05
@andrewphelpsj andrewphelpsj force-pushed the optional-install-api-impl branch from 5eba35a to 6762c7a Compare August 29, 2024 16:55
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

Couple of minor test comments

@@ -295,6 +297,21 @@ func postSystemsInstallFinish(cli *client.Client,
return waitChange(chgId)
}

func maybeGetOptionalInstall() *client.OptionalInstallRequest {
f, err := os.Open("./optional-install.json")

Choose a reason for hiding this comment

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

I think it would be better to pass the json file as a program argument to make it more explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but agreed on this

Copy link
Member Author

Choose a reason for hiding this comment

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

Added flags to muinstaller in c853aa3.

Comment on lines 20 to 23
# default (unencrypted) case
NESTED_ENABLE_TPM: false
NESTED_ENABLE_SECURE_BOOT: false

Choose a reason for hiding this comment

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

You could remove the "plain" ones I think.

Copy link
Member Author

@andrewphelpsj andrewphelpsj Sep 3, 2024

Choose a reason for hiding this comment

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

I think it probably would be better to run the various test cases with encryption on, right? And then leave this plain case to cover the unencrypted case.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, then these two should be set to true and maybe remove the "encrypted" case as it will be covered by the "install*" cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in a881fd2.

Copy link
Collaborator

@pedronis pedronis 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

Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM, but please look at testing comment

Comment on lines 20 to 23
# default (unencrypted) case
NESTED_ENABLE_TPM: false
NESTED_ENABLE_SECURE_BOOT: false

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, then these two should be set to true and maybe remove the "encrypted" case as it will be covered by the "install*" cases.

@andrewphelpsj andrewphelpsj force-pushed the optional-install-api-impl branch from 0b9ce2a to a881fd2 Compare September 3, 2024 15:57
@andrewphelpsj andrewphelpsj added the Run nested The PR also runs tests inluded in nested suite label Sep 3, 2024
@andrewphelpsj andrewphelpsj reopened this Sep 3, 2024
@andrewphelpsj andrewphelpsj force-pushed the optional-install-api-impl branch 2 times, most recently from 5afd742 to 3a019e8 Compare September 5, 2024 01:24
@andrewphelpsj andrewphelpsj force-pushed the optional-install-api-impl branch from 3a019e8 to 437f018 Compare September 5, 2024 17:56
@andrewphelpsj
Copy link
Member Author

The tests failing here are known and unrelated to the changes in this PR, and I've verified that they're also present on master.

@alfonsosanchezbeato alfonsosanchezbeato merged commit 0dfd94b into canonical:master Sep 6, 2024
48 of 53 checks passed
@andrewphelpsj andrewphelpsj deleted the optional-install-api-impl branch September 6, 2024 13:03
soumyaDghosh pushed a commit to soumyaDghosh/snapd that referenced this pull request Dec 6, 2024
…s and components (canonical#14426)

* daemon, o/devicestate: rename devicestate.OptionalInstall to better represent uses devicestate.OptionalContainers

* o/devicestate: add available optional snap and components to the System struct

* daemon: report available optional containers for a system available for install

* o/devicestate, s/seedtest: handle optional-install field on install-finish task

* tests: test seeding optional snaps and components from snapd installer api

* tests: use flags for arguments passed to the muinstaller

* tests: use encrypted installation for cases with optional snaps and components

* tests: make test work for core24 too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Run Nested -auto- Label automatically added in case nested tests need to be executed Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants