o/snapstate: only create link-component tasks when needed - #16882
Merged
Conversation
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
from
April 8, 2026 16:52
8d4ec22 to
9aaf098
Compare
|
Thu May 7 14:26:45 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16882 +/- ##
==========================================
- Coverage 79.06% 79.05% -0.01%
==========================================
Files 1374 1381 +7
Lines 191076 191188 +112
Branches 2465 2465
==========================================
+ Hits 151065 151148 +83
- Misses 30918 30941 +23
- Partials 9093 9099 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
from
April 14, 2026 13:13
9aaf098 to
a39c311
Compare
Member
Author
|
This is rebased off of master now, and ready for review. |
alfonsosanchezbeato
left a comment
alfonsosanchezbeato
left a comment
Contributor
There was a problem hiding this comment.
Thanks, couple of questions
| return false | ||
| } | ||
|
|
||
| return cs.CompType == cc.compsup.CompType && *cs.SideInfo == *cc.compsup.CompSideInfo |
Contributor
There was a problem hiding this comment.
Is it really necessary to consider here the type as well? It seems that CompSideInfo would be unique already.
Member
Author
There was a problem hiding this comment.
Yeah, prob don't need it. Removed.
Comment on lines
+461
to
+464
| // when refreshing to the current snap revision, component refreshes | ||
| // rely on unlink-current-component/link-component. thus, undo can | ||
| // restore the old component revision cleanly. | ||
| return false |
Contributor
There was a problem hiding this comment.
Question, but is then necessary to have the link/unlink component task? Or is it just harmless, but unneeded?
Contributor
There was a problem hiding this comment.
this could be installing a new component as well? and then is needed, maybe the comment should also mention the install POV and not just the refresh POV?
Member
Author
There was a problem hiding this comment.
I updated the comments here, hopefully better now?
pedronis
reviewed
Apr 15, 2026
pedronis
left a comment
pedronis
left a comment
Contributor
There was a problem hiding this comment.
did a pass, couple of comments and questions
| if !cc.snapsup.Revert { | ||
| // finalize (sets SnapState). if we're reverting, there isn't anything to | ||
| // change in SnapState regarding the component | ||
| if !cc.targetSnapAlreadyHasComponentRevision() { |
Contributor
There was a problem hiding this comment.
is this equivalent also to !snapup.Revert because for the revert case we have made sure the revision is present already at this stage?
Member
Author
There was a problem hiding this comment.
Yes, this condition is essentially a super-set of the original revert condition.
Comment on lines
+461
to
+464
| // when refreshing to the current snap revision, component refreshes | ||
| // rely on unlink-current-component/link-component. thus, undo can | ||
| // restore the old component revision cleanly. | ||
| return false |
Contributor
There was a problem hiding this comment.
this could be installing a new component as well? and then is needed, maybe the comment should also mention the install POV and not just the refresh POV?
alfonsosanchezbeato
approved these changes
Apr 15, 2026
alfonsosanchezbeato
left a comment
alfonsosanchezbeato
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks for the changes
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
from
April 16, 2026 17:24
5a3904d to
0ab511e
Compare
andrewphelpsj
added
Run nested
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
from
April 16, 2026 17:24
0ab511e to
5ebb091
Compare
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
2 times, most recently
from
May 1, 2026 13:33
c64f4f8 to
442dc65
Compare
This resolves an issue that happens on the undo path of a refresh to an already existing snap revision that happened to have some components. Assume this sequence in the state: snap-a+one+two at revision 1 <- previous snap-a+one+two at revision 2 <- current Then, the user refreshes snap-a+one+two back to the still-local revision 1: $ snap refresh snap-a --revision=1 In that scenario, we were creating link-component tasks for the already linked components one and two. The undo of this refresh would result in the sequence: snap-a at revision 1 <- previous, missing components because of undone link-component tasks snap-a+one+two at revision 2 <- current
andrewphelpsj
force-pushed
the
no-link-if-linked
branch
from
May 5, 2026 16:30
442dc65 to
60533d3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This resolves an issue that happens on the undo path of a refresh to an already existing snap revision that happened to have some components.
Assume this sequence in the state:
Then, the user refreshes snap-a+one+two back to the still-local revision 1:
In that scenario, we were creating link-component tasks for the already linked components one and two. The undo of this refresh would result in the sequence: