o/snapstate: download, link, unlink, and discard snap icon in snapstate handlers #15070
Conversation
This file contains 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
This was referenced Feb 13, 2025
olivercalder
changed the title
|
Thu Feb 20 21:33:42 UTC 2025 Failures:Executing:
Restoring:
|
olivercalder
force-pushed
the
snap-icons-metadata-management
branch
from
316d5db to
54c4013
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15070 +/- ##
==========================================
- Coverage 78.07% 78.06% -0.02%
==========================================
Files 1182 1185 +3
Lines 157743 157993 +250
==========================================
+ Hits 123154 123332 +178
- Misses 26943 26994 +51
- Partials 7646 7667 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
olivercalder
force-pushed
the
snap-icons-metadata-management
branch
from
54c4013 to
e7b453f
Compare
bboozzoo
reviewed
Feb 17, 2025
overlord/snapstate/handlers.go
Outdated
| iconURL = resultIconURL | ||
| } | ||
|
|
||
| ctx := tomb.Context(nil) // XXX: should this be a real context? |
There was a problem hiding this comment.
you mean for e.g. context.Background() as a parent?
AFAIU we want the 'tomb' wrapped context as it allows us to detect someone calling Kill() on the associated tomb, and thus allows for the download to be interrupted.
There was a problem hiding this comment.
Yes, I was thinking context.Background(). But it previously had nil passed as the ctx parameter directly, so I'm not sure what's correct. Would this work?
Suggested change
| ctx := tomb.Context(nil) // XXX: should this be a real context? | |
| ctx := tomb.Context(context.Background()) |
pedronis
reviewed
Feb 17, 2025
overlord/snapstate/handlers.go
Outdated
| @@ -753,17 +755,53 @@ func (m *SnapManager) doDownloadSnap(t *state.Task, tomb *tomb.Tomb) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
|
|
|||
| // Re-compute icon download filepath and URL if the result info has the | |||
There was a problem hiding this comment.
this is a very old compatibility path I'm not sure we should worry adding icon support here
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…icons Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
During `InstallStoreMetadata`, link snap icon from the icons download pool to the icons install directory. During `UninstallStoreMetadata`, remove the linked icon from the icons install directory, if it exists there. During `DiscardStoreMetadata`, remove the linked icon from both the icons install directory and the icons download pool, if it exists there. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
olivercalder
force-pushed
the
snap-icons-metadata-management
branch
from
33e02b0 to
487ebf8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 PR is based on #15051, and replaces #15003.
During the "download-snap" task, download the snap icon as well.
During "link-snap" and "unlink-snap", link/unlink the snap icon from the downloaded icons pool to the installed icons directory. Do this by adding snap icon management helpers, and calling them from the store metadata helpers in the backend.
Lastly, discard the downloaded snap icon when the final revision of the snap is discarded from disk (and there are no other instances). We want to ensure that the snap icon remains in the pool during any situation when it's possible to install the snap without doing another "download-snap" task, such as via a revert.
This PR is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-34436