-
Notifications
You must be signed in to change notification settings - Fork 562
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
daemon: attach affected-snaps data to tasks #13953
daemon: attach affected-snaps data to tasks #13953
Conversation
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.
Reviewed it and it looks fine.
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.
This looks fine to me as well.
overlord/snapstate/conflict.go
Outdated
@@ -76,7 +76,7 @@ func RegisterAffectedSnapsByKind(kind string, f AffectedSnapsFunc) { | |||
affectedSnapsByKind[kind] = f | |||
} | |||
|
|||
func affectedSnaps(t *state.Task) ([]string, error) { | |||
func AffectedSnapsByTask(t *state.Task) ([]string, error) { |
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.
reading the name it feels like this should return some sort of a map of task id -> list of affected snaps. How about SnapsAffectedByTask?
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.
Good point, updated!
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.
thank you, did a pass, couple of comments
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, tiny comment
overlord/snapstate/conflict.go
Outdated
@@ -76,6 +76,7 @@ func RegisterAffectedSnapsByKind(kind string, f AffectedSnapsFunc) { | |||
affectedSnapsByKind[kind] = f | |||
} | |||
|
|||
// SnapsAffectedByTask returns a list of snap names affected by the given task. |
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.
"... a list of names of snaps ..." would be slightly more precise
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
6c529fe
to
f20ded0
Compare
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
This allows API caller to associate each task with the relevant snaps which is useful in the case change progress tracking when a change is associated with multiple snaps. This exact use case is needed by snapd-desktop-integration. Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
f20ded0
to
0a3e5ef
Compare
This PR is an iteration based on #13902 which allows
/v2/changes
API caller to associate each task with the relevant affected snaps which is useful in the case change progress tracking per snap when a change is associated with multiple snaps.This exact use case is needed by snapd-desktop-integration.
Note: When this is merged, The REST API docs need to be updated as well.