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
seed/seedwriter: consider modes when checking for deps availability #9710
seed/seedwriter: consider modes when checking for deps availability #9710
Conversation
the code before wasn't taking into account the modes in which the base or default content providers were available vs the modes of the dependent snap, with the changes here it now does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
seed/seedwriter/seed20.go
Outdated
if mode == "run" || mode == "ephemeral" { | ||
return false | ||
} | ||
// consider ephemeral as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could extend the comment a bit, eg.
// all non-run modes (eg. recover) are considered ephemeral,
// as a fallback check if the snap is listed for an ephemeral mode
byMode := availableByMode[mode] | ||
if !byMode.Contains(snapRef) { | ||
if mode == "run" || mode == "ephemeral" { | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// no additional fallback check for well known modes
@@ -90,6 +90,13 @@ type SeedSnap struct { | |||
optionSnap *OptionsSnap | |||
} | |||
|
|||
func (sn *SeedSnap) modes() []string { | |||
if sn.modelSnap == nil { | |||
return []string{"run"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// default mode for extra snaps not listed in the model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, post-merge LGTM from me
the code before wasn't taking into account the modes in which
the base or default content providers were available vs
the modes of the dependent snap, with the changes here
it now does
fixes LP: #1883970