o/assertstate, o/snapstate: always validate existence of snap-resource-pair for asserted components #14749
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14749 +/- ##
==========================================
+ Coverage 78.95% 79.04% +0.09%
==========================================
Files 1084 1087 +3
Lines 146638 147725 +1087
==========================================
+ Hits 115773 116767 +994
- Misses 23667 23730 +63
- Partials 7198 7228 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -347,6 +347,9 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS | |||
| if fromStore { | |||
| prepare = st.NewTask("download-component", fmt.Sprintf(i18n.G("Download component %q%s"), compSetup.ComponentName(), revisionStr)) | |||
| } else { | |||
| // if we're not downloading the component, then we can clear out any | |||
| // download info that might exist from the calling code | |||
| compSetup.DownloadInfo = nil | |||
There was a problem hiding this comment.
Is this really necessary? It should be actually nil for local files already, if that is not the case sounds a bit like a bug somewhere.
There was a problem hiding this comment.
Consider the case where we refresh a snap and the component revisions don't change. In that case, we will still have a DownloadInfo on the ComponentSetup, since we did check with the store for potential updates. It is up to doInstallComponent to decide that we don't need to re-download it.
Since it is valid for doInstallComponent to decide to not download a component, despite the caller providing a DownloadInfo, I think that this is needed.
There was a problem hiding this comment.
0a42660 adds a commit that shows the need for this.
There was a problem hiding this comment.
but in that case we still want to get a potentially updated pair from the store no? so it's not clear we want this behavior change
There was a problem hiding this comment.
Ah you're totally right.
| @@ -347,6 +347,9 @@ func doInstallComponent(st *state.State, snapst *SnapState, compSetup ComponentS | |||
| if fromStore { | |||
| prepare = st.NewTask("download-component", fmt.Sprintf(i18n.G("Download component %q%s"), compSetup.ComponentName(), revisionStr)) | |||
| } else { | |||
| // if we're not downloading the component, then we can clear out any | |||
| // download info that might exist from the calling code | |||
| compSetup.DownloadInfo = nil | |||
There was a problem hiding this comment.
but in that case we still want to get a potentially updated pair from the store no? so it's not clear we want this behavior change
Currently we don't enforce validating the existence of a snap-resource-pair when sideloading an asserted component. This change makes it so validate-component will now always be run when installing a component with a revision from the store. The task now handles components that are from the store differently than components that are being installing from a local file.