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: support creating recovery systems with components from the store #14883

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrewphelpsj
Copy link
Member

@andrewphelpsj andrewphelpsj commented Dec 18, 2024

This PR allows us to create recovery systems that contain components using the /v2/systems API.

@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Dec 18, 2024
@andrewphelpsj andrewphelpsj force-pushed the create-rec-system-components-from-store branch 3 times, most recently from 97dcdbc to f5b4456 Compare December 19, 2024 21:38
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 82.14286% with 30 lines in your changes missing coverage. Please review.

Project coverage is 78.23%. Comparing base (24a0034) to head (579ace9).
Report is 94 commits behind head on master.

Files with missing lines Patch % Lines
overlord/devicestate/devicestate.go 84.16% 13 Missing and 6 partials ⚠️
overlord/devicestate/systems.go 77.08% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14883      +/-   ##
==========================================
+ Coverage   78.20%   78.23%   +0.02%     
==========================================
  Files        1151     1155       +4     
  Lines      151396   152952    +1556     
==========================================
+ Hits       118402   119659    +1257     
- Misses      25662    25906     +244     
- Partials     7332     7387      +55     
Flag Coverage Δ
unittests 78.23% <82.14%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewphelpsj andrewphelpsj force-pushed the create-rec-system-components-from-store branch from f5b4456 to ff1739f Compare December 20, 2024 14:09
@andrewphelpsj andrewphelpsj force-pushed the create-rec-system-components-from-store branch from ff1739f to 44f4ac7 Compare January 6, 2025 15:14
@github-actions github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jan 6, 2025
@andrewphelpsj andrewphelpsj force-pushed the create-rec-system-components-from-store branch from 44f4ac7 to 24552e7 Compare January 7, 2025 14:13
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.

couple of questions

overlord/devicestate/devicestate.go Show resolved Hide resolved
overlord/devicestate/devicestate.go Outdated Show resolved Hide resolved
@andrewphelpsj andrewphelpsj requested a review from pedronis January 8, 2025 14:10
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.

thanks, the required predicates need a bit more commenting I think

overlord/devicestate/devicestate.go Show resolved Hide resolved
overlord/devicestate/devicestate.go Show resolved Hide resolved
… creating recovery systems with components from the store
… that require snaps that are at least in the model
@andrewphelpsj andrewphelpsj force-pushed the create-rec-system-components-from-store branch from 8d163e4 to e4735ed Compare January 9, 2025 14:19
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.

Some minor comment, mostly about the spread tests

@@ -53,7 +53,7 @@ execute: |
boot_id="$(tests.nested boot-id)"

# create the system
change_id=$(post_json_data /v2/systems '{"action": "create", "label": "new-system", "validation-sets": ["test-snapd/recovery-system-pinned=1"], "test-system": %s, "mark-default": %s}' "${TEST_SYSTEM}" "${MARK_DEFAULT}")
change_id=$(post_json_data /v2/systems '{"action": "create", "label": "new-system", "validation-sets": ["test-snapd/recovery-system-pinned=2"], "test-system": %s, "mark-default": %s}' "${TEST_SYSTEM}" "${MARK_DEFAULT}")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a bug in the test, found by me fixing a bug in the code. The old version of the validation set was requiring "core20", which doesn't make sense as this test is used on multiple systems with various bases.

tests/nested/manual/component-recovery-system/task.yaml Outdated Show resolved Hide resolved
tests/nested/manual/component-recovery-system/task.yaml Outdated Show resolved Hide resolved
Comment on lines 240 to 241
// * passed into the task via a list of side infos (these would have
// come from a user posting components via the API)
Copy link
Member

Choose a reason for hiding this comment

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

Where is this case handled? I'm not sure if I see that in this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

That comment contains too much, this PR doesn't implement the offline case where we provide components. It'll be in the next one, but I'll fix this comment for now.

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.

LGTM, thanks!

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.

3 participants