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

o/snapstate: support user-services when refreshing snaps #13905

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Meulengracht
Copy link
Member

@Meulengracht Meulengracht commented Apr 29, 2024

Implements the remaining functionality to support user-services when refreshing snaps. SnapState now also keeps track of the last disabled services for user-services.

@Meulengracht Meulengracht force-pushed the feature/refresh-user-services-support branch from 6908f90 to 1762c26 Compare April 29, 2024 09:30
@Meulengracht Meulengracht marked this pull request as ready for review April 29, 2024 13:54
@bboozzoo bboozzoo self-requested a review April 30, 2024 11:58
@Meulengracht Meulengracht force-pushed the feature/refresh-user-services-support branch from fa70343 to 7a3d4e1 Compare April 30, 2024 21:37
@Meulengracht
Copy link
Member Author

I dropped the spread test I was doing, we were already covering this functionality in the unit tests - and extending them to cover the user services part made sense.

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.

some comments/questions

overlord/snapstate/handlers.go Outdated Show resolved Hide resolved
// Insert it into the overview, into the 'found' list. Re-use the entire
// list for all users (-1) as there won't be any difference between users
// in this specific case (and to allow us to keep a singular list of services
// that need to be disabled).
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment is not very clear, can we use a single list because hooks cannot target single users via snapctl commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your comment let me to a missing case of support for user-services. The service tracking code for hooks did not account for user-services - I added this in here, as it is required for completeness in the refresh handling.

overlord/snapstate/handlers.go Outdated Show resolved Hide resolved
usersession/client/client.go Outdated Show resolved Hide resolved
@Meulengracht Meulengracht force-pushed the feature/refresh-user-services-support branch from 5fd71d8 to abe095c Compare May 3, 2024 20:38
@Meulengracht
Copy link
Member Author

I split out the wrapper changes: #13957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants