Skip to content
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

Merged
merged 2 commits into from
May 31, 2024

Conversation

ZeyadYasser
Copy link
Collaborator

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.

@ZeyadYasser ZeyadYasser added Needs Samuele review Needs a review from Samuele before it can land Needs Documentation 📖 labels May 8, 2024
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 8, 2024
Copy link
Contributor

@sergio-costas sergio-costas left a 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.

Copy link
Collaborator

@zyga zyga left a 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.

@@ -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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated!

@ZeyadYasser ZeyadYasser requested a review from bboozzoo May 23, 2024 10:41
Copy link
Collaborator

@pedronis pedronis left a 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

overlord/snapstate/conflict.go Show resolved Hide resolved
@ZeyadYasser ZeyadYasser requested a review from pedronis May 27, 2024 11:37
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, tiny comment

@@ -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.
Copy link
Collaborator

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

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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>
@ernestl ernestl merged commit c182ed0 into snapcore:master May 31, 2024
39 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Needs Documentation 📖 Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
6 participants