wrappers: refactor StopServices to account for sending --disable to the user-session agent #13921
Conversation
wrappers/services.go
Outdated
| } | ||
| // categorizeServices returns a list of system and user services | ||
| // ordered by the same order that 'app' is passed into. | ||
| func categorizeServices(apps []*snap.AppInfo, includeActivators bool) ([]string, []string) { |
There was a problem hiding this comment.
I have a cleanup planned for StartServices that will use this function too, which is why the includeActivators part exists.
There was a problem hiding this comment.
See #13958 for details
Meulengracht
force-pushed
the
feature/disable-per-user
branch
from
12f4205
to
6030ec4
Compare
There was a problem hiding this comment.
Approved, after fixing minor issues, disregarding the non-ideal stopServices with --disable, with knowledge of followup cleanup PR.
| @@ -195,7 +194,9 @@ func stopService(sysd systemd.Systemd, cli *userServiceClient, scope snap.Daemon | |||
| } | |||
|
|
|||
| case snap.UserDaemon: | |||
| if err := cli.stopServices(svcs...); err != nil { | |||
| // Only called as a part of undoing in StartServices | |||
| const disable = false | |||
There was a problem hiding this comment.
To me its not obvious why we have to set disable to false, one might expect this as default behavior of a "stop" function, to stop and not disable. I would recommend an explanation.
Furthermore, to hardcode disable into function so that it is appropriate for use in a single context does not seem ideal.
There was a problem hiding this comment.
After discussion this:
- There is inherent undo logic in the user-session agent itself
- So actually, what StartServices should not be doing, is handling undo logic for user-services
In a followup cleanup PR, stopService is removed and a cleaner solution implemented.
Will ignore this as a means to a followup improvement.
There was a problem hiding this comment.
for clarity might still be a good idea for now to add a doc comment to the function explaing that it stops services without disabling them
Meulengracht
force-pushed
the
feature/disable-per-user
branch
from
6030ec4
to
6de728b
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #13921 +/- ##
==========================================
- Coverage 78.86% 78.76% -0.10%
==========================================
Files 1043 1046 +3
Lines 134595 136393 +1798
==========================================
+ Hits 106144 107436 +1292
- Misses 21837 22234 +397
- Partials 6614 6723 +109
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
wrappers/services.go
Outdated
| } | ||
| // categorizeServices returns a list of system and user services | ||
| // ordered by the same order that 'app' is passed into. | ||
| func categorizeServices(apps []*snap.AppInfo, includeActivatedServices bool) ([]string, []string) { |
There was a problem hiding this comment.
it would be good for the return value to have names as well so it's clearer what is what
| @@ -195,7 +194,9 @@ func stopService(sysd systemd.Systemd, cli *userServiceClient, scope snap.Daemon | |||
| } | |||
|
|
|||
| case snap.UserDaemon: | |||
| if err := cli.stopServices(svcs...); err != nil { | |||
| // Only called as a part of undoing in StartServices | |||
| const disable = false | |||
There was a problem hiding this comment.
for clarity might still be a good idea for now to add a doc comment to the function explaing that it stops services without disabling them
|
|
||
| disableServices := []string{} | ||
| func filterAppsForStop(apps []*snap.AppInfo, reason snap.ServiceStopReason, flags *StopServicesFlags) []*snap.AppInfo { |
There was a problem hiding this comment.
now that this is a separate helper it might be worth adding a doc comment with a summary of what is does, how arguments affect that
There was a problem hiding this comment.
Same, this is a great point and will add this for start as well
…he user-session agent
Meulengracht
force-pushed
the
feature/disable-per-user
branch
from
6372c97
to
986ecbc
Compare
Refactor StopServices to make it easier to account for disabling user-services.
Changes from this refactor
--disablerequest to the user-session agent.