o/snapstate: download, link, unlink, and discard snap icon in snapstate handlers #15070
Conversation
olivercalder
changed the title
|
Wed Feb 19 16:19:29 UTC 2025 Failures:Executing:
|
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. |
Signed-off-by: Oliver Calder <[email protected]>
…icons Signed-off-by: Oliver Calder <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
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 <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
olivercalder
force-pushed
the
snap-icons-metadata-management
branch
from
54c4013 to
e7b453f
Compare
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?
| ctx := tomb.Context(nil) // XXX: should this be a real context? | |
| ctx := tomb.Context(context.Background()) |
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 <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
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