client,cmd/snap: introduce --user, --system and --users switches for snap service operations #13368

Meulengracht · 2023-11-17T08:21:09Z

Introduces the new switches --user to select user services for the current user, --system to select system services and --users to select user services for all users for the following operations:
snap start, snap stop and snap restart

codecov-commenter · 2023-11-21T08:23:09Z

Codecov Report

Attention: Patch coverage is 92.30769% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 78.90%. Comparing base (81d3d51) to head (98352c0).

Files	Patch %	Lines
client/apps.go	87.14%	8 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13368   +/-   ##
=======================================
  Coverage   78.89%   78.90%           
=======================================
  Files        1040     1040           
  Lines      133862   133907   +45     
=======================================
+ Hits       105610   105655   +45     
  Misses      21658    21658           
  Partials     6594     6594

Flag	Coverage Δ
unittests	`78.90% <92.30%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Meulengracht · 2024-02-26T13:53:07Z

client/apps.go

@@ -183,13 +184,119 @@ func (client *Client) Logs(names []string, opts LogOptions) (<-chan Log, error)
 	return ch, nil
 }

+type UserSelection int


All of this has been moved from servicestate to share with client

bboozzoo · 2024-02-29T13:07:36Z

client/apps_test.go

-	for _, sc := range scs {
-		id, err := cs.cli.Stop(sc.names, sc.opts)
+	for _, sc := range tests {
+		comment := check.Commentf("{%q; %q; %q; %#v}", sc.names, sc.scope, sc.users, sc.opts)


btw. you can also c.Logf("testing ... "), IIRC the logs are displayed if a test fails

cmd/snap/cmd_services.go

pedronis

remark about aligning with pebble

cmd/snap/cmd_services.go

Meulengracht · 2024-03-07T08:13:00Z

@bboozzoo @pedronis I updated the PR on switch names and allowed input.

bboozzoo

LGTM

tests/main/services-user/task.yaml

pedronis

couple of questions

pedronis · 2024-03-11T17:12:41Z

cmd/snap/cmd_services.go

+type userAndScopeMixin struct {
+	System bool   `long:"system"`
+	User   bool   `long:"user"`
+	Users  string `long:"users" optional:"yes" optional-value:"all"`


can you remind me what optional-value:"all" does ?

Yes, it allows you to use --users and the value will default to =all. So doing --users is equivalent to using --users=all

I don't think we want this, as we discussed elsewhere --users and --user are a typo away, and pebble doesn't seem to do this for --users. We want to force to spell out the full --users=all

Updated this to not allow --users

pedronis · 2024-03-11T17:13:43Z

cmd/snap/cmd_services.go

+}
+
+func (um *userAndScopeMixin) validateScopes() error {
+	switch {


does is make sense to set --system and --user/--users together?

As maciej also pointed out, it probably makes sense to allow providing --system --users which is equivalent to the default behaviour which is providing nothing.

I also allow --system --user to allow restarting system services and only the current user's services, since they are allowed to use individually, it probably makes sense to just allow them in one go.

OTOH it's a bit confusing so I'm open to modifications on this

For now I would not allow, --system and --user together. OTOH if we support --system --users we need to have a spread test for it as well that checks both kind of services

I disabled --system --user together for now, if we think it should be enabled we can add a case in the spread test for it later.

pedronis · 2024-03-12T08:24:19Z

cmd/snap/cmd_services_test.go

+		})
+		c.Check(checkInvocation(op, summaries[i], []string{"foo", "bar"}, []string{"user", "system"}), check.DeepEquals, map[string]interface{}{
+			"action": op,
+			"names":  []interface{}{"foo", "bar"},


probably worth saying something about what happens with scope here, is system/users the default?

Added a comment revolving this

pedronis · 2024-03-12T08:26:07Z

cmd/snap/cmd_services.go

+}
+
+func (um *userAndScopeMixin) validateScopes() error {
+	switch {


For now I would not allow, --system and --user together. OTOH if we support --system --users we need to have a spread test for it as well that checks both kind of services

…snap service operations

…n client

…t/stop/restart

…ectly affected

… with =all

pedronis

question

cmd/snap/cmd_services.go

this will need a re-review

…ce a value for it, add case for --system --users=all in spread test

pedronis

thank you, comment about the help string for --users

cmd/snap/cmd_services.go

bboozzoo

LGTM

cmd/snap/cmd_services.go

… messages

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Nov 17, 2023

Meulengracht mentioned this pull request Nov 23, 2023

usersession: implement restart controls and status querying for user services #13336

Merged

Meulengracht mentioned this pull request Jan 3, 2024

wrappers: support for restarting user-daemons #13337

Merged

Meulengracht mentioned this pull request Jan 19, 2024

daemon,o/servicestate: support for scopes and users in API for services #13338

Merged

Meulengracht force-pushed the feature/user-service-5 branch 2 times, most recently from f4f3727 to 248cd9b Compare February 23, 2024 13:33

github-actions bot added Needs Documentation -auto- Label automatically added which indicates the change needs documentation and removed Needs Documentation -auto- Label automatically added which indicates the change needs documentation labels Feb 23, 2024

Meulengracht force-pushed the feature/user-service-5 branch from dbbef85 to 371b5fc Compare February 23, 2024 13:45

Meulengracht marked this pull request as ready for review February 23, 2024 13:46

Meulengracht requested review from pedronis and ernestl February 26, 2024 08:58

Meulengracht commented Feb 26, 2024

View reviewed changes

Meulengracht requested a review from bboozzoo February 29, 2024 07:43

Meulengracht added this to the 2.62 milestone Feb 29, 2024

bboozzoo reviewed Feb 29, 2024

View reviewed changes

pedronis reviewed Mar 1, 2024

View reviewed changes

cmd/snap/cmd_services.go Outdated Show resolved Hide resolved

Meulengracht requested review from bboozzoo and pedronis March 7, 2024 08:12

bboozzoo previously approved these changes Mar 7, 2024

View reviewed changes

tests/main/services-user/task.yaml Show resolved Hide resolved

Meulengracht force-pushed the feature/user-service-5 branch from 85bcf3e to cbaada6 Compare March 7, 2024 14:35

pedronis reviewed Mar 11, 2024

View reviewed changes

Meulengracht requested a review from pedronis March 12, 2024 07:34

pedronis reviewed Mar 12, 2024

View reviewed changes

Meulengracht added 4 commits March 12, 2024 09:50

client,cmd/snap: introduce --user, --system and --users switches for …

175f1d0

…snap service operations

client,o/servicestate: move Scope/UserSelection to client for reuse i…

efd3fe1

…n client

client,cmd/snap: improve handling of user and scope args

dfe01f4

NEWS: update news to reflect that we now support user daemons in star…

7378a61

…t/stop/restart

Meulengracht added 3 commits March 12, 2024 09:53

cmd/snap: some review feedback on allowed input

3ac52ad

t/main/services-user: add additional user to verify services are corr…

8301cb0

…ectly affected

cmd/snap: do not allow --system --user together, do not allow --users…

98352c0

… with =all

Meulengracht force-pushed the feature/user-service-5 branch from cbaada6 to 98352c0 Compare March 12, 2024 09:05

Meulengracht requested a review from pedronis March 12, 2024 09:07

pedronis reviewed Mar 12, 2024

View reviewed changes

cmd/snap/cmd_services.go Outdated Show resolved Hide resolved

Meulengracht requested a review from pedronis March 13, 2024 07:55

Meulengracht mentioned this pull request Mar 13, 2024

o/hookstate/ctlcmd: introduce --user, --system and --users switches for snap service operations #13696

Merged

Meulengracht requested a review from bboozzoo March 13, 2024 08:43

tests,cmd: use --users=all in test, dont mark --users optional, enfor…

1a9a790

…ce a value for it, add case for --system --users=all in spread test

Meulengracht force-pushed the feature/user-service-5 branch from 2f78e1e to 1a9a790 Compare March 13, 2024 08:45

pedronis approved these changes Mar 13, 2024

View reviewed changes

cmd/snap/cmd_services.go Outdated Show resolved Hide resolved

bboozzoo approved these changes Mar 13, 2024

View reviewed changes

cmd/snap/cmd_services.go Outdated Show resolved Hide resolved

cmd/snap/cmd_services.go Show resolved Hide resolved

cmd/snap: add a comment for unreachable code, and correct a couple of…

57217a9

… messages

Meulengracht merged commit 261d1c2 into canonical:master Mar 13, 2024
36 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client,cmd/snap: introduce --user, --system and --users switches for snap service operations #13368

client,cmd/snap: introduce --user, --system and --users switches for snap service operations #13368

Meulengracht commented Nov 17, 2023

codecov-commenter commented Nov 21, 2023 •

edited

Loading

Meulengracht Feb 26, 2024

bboozzoo Feb 29, 2024

pedronis left a comment

Meulengracht commented Mar 7, 2024

bboozzoo left a comment

pedronis left a comment

pedronis Mar 11, 2024

Meulengracht Mar 12, 2024

pedronis Mar 12, 2024

Meulengracht Mar 12, 2024

pedronis Mar 11, 2024

Meulengracht Mar 12, 2024

pedronis Mar 12, 2024

Meulengracht Mar 12, 2024

pedronis Mar 12, 2024

Meulengracht Mar 12, 2024

pedronis Mar 12, 2024

pedronis left a comment

pedronis left a comment

bboozzoo left a comment

client,cmd/snap: introduce --user, --system and --users switches for snap service operations #13368

client,cmd/snap: introduce --user, --system and --users switches for snap service operations #13368

Conversation

Meulengracht commented Nov 17, 2023

codecov-commenter commented Nov 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Meulengracht commented Mar 7, 2024

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 21, 2023 •

edited

Loading