o/hookstate/ctlcmd: unify the error message when context is missing #10606

mardy · 2021-08-11T06:32:28Z

Make the error message more user friendly, since the concept of a
"context" is not obvious to the end user. Also let it be translated, and
unified across the various snapctl subcommands.

MiguelPires

I noticed there's a few usages of context() that pass the context into runServiceCommand (which then performs this check), could we use ensureContext() there as well? It'd have to be slightly refactored though

mardy · 2021-08-11T14:01:47Z

I noticed there's a few usages of context() that pass the context into runServiceCommand (which then performs this check), could we use ensureContext() there as well? It'd have to be slightly refactored though

I noticed that, but then I forgot to change that. I now added a commit which changes the error message for the service commands too, but I cannot think of a smart way to refactor this.

MiguelPires

LGTM, thanks!

mvo5 · 2021-08-12T15:43:18Z

overlord/hookstate/ctlcmd/ctlcmd.go

@@ -66,6 +68,13 @@ func (c *baseCommand) context() *hookstate.Context {
 	return c.c
 }

+func (c *baseCommand) ensureContext() (context *hookstate.Context, err error) {
+	if c.c == nil {
+		err = errors.New(i18n.G(`The "snapctl" command can only be invoked from within a snap`))


Thanks for working on this, the change itself is great and a nice idea!

Just a quick drive-by highlevel comment - I think we should not have go error types localized. Instead I would suggest the pattern is something like:

var ErrNoContext = errors.New("cannot use snapctl without a context") func (c *baseCommand) ensureContext() (context *hookstate.Context, err error) { ... return ErrNoContext }

and then in the relevant "main" (cmd/snapctl/main.go I presume) we check for ErrNoContext and if we see that we print the localized error and exit.

I hope this makes sense, sorry that it's a bit terse, I'm a bit short on time right now.

Mmm... I do recognize the problem, but I think that a proper solution cannot be done in the context of this PR. The problem is far from trivial: we have a process boundary between snapctl (which knows the user's locale) and snapd (which doesn't). There are (at least) two possible approaches:

Have snapd return errors in machine form, either as an untranslated string (as you suggest), or as error codes. Then the client will translate them properly when presenting them to the user.

Have the client pass locale information to snapd over the REST API, and make i18n.G() (or some other function) aware of the context so that it would translate the messages to the locale encoded in the context.

The first option has the big downside that it would only work for fixed messages (a %s in them would immediately make the message non translatable), unless we design a way to pass more context variables back to the client process and manage to build a sensible error message there.

Anyway, regardless of the path we choose, both require a substantial amount of work, as both snapctl and snapd are not ready for either solution. That's why I propose to leave this for later. :-)

Indeed this is more complicated due to how snapctl is implemented, we already have similar case with ctlcmd.UnsuccessfulError type and respective client.ErrorKindUnsuccessful which requires changes in api_snapctl.go, cmd/snapctl/main.go (as well as overlord/hookstate/ctlcmd/..); I'm not sure it's worth doing atm, but if we want it localized, then we should follow this pattern.

But I agree with the high level remark from @mvo5, the error itself shouldn't be localized.

and nitpick^2, the error isn't strictly true because we deliberately allow 'snapctl --help' invocation from the shell; I think the error message I suggested below in the other comment would solve it.

OK, I removed the i18n call. However, there are still many of such calls in this module; whould I remove all of them (maybe in a separate PR)?

stolowski · 2021-08-16T10:18:12Z

overlord/hookstate/ctlcmd/fde_setup.go

@@ -60,9 +60,9 @@ func init() {
 }

 func (c *fdeSetupRequestCommand) Execute(args []string) error {
-	context := c.context()
-	if context == nil {
-		return fmt.Errorf("cannot run fde-setup-request without a context")


I think including snapctl subcommand name in the message was nice/useful; maybe "'snapctl %s' can only be invoked from within a snap"?

pedronis

The new error message is a bit confusing. I need to think of a suggestion

pedronis · 2021-08-19T11:25:03Z

overlord/hookstate/ctlcmd/ctlcmd.go

@@ -66,7 +71,16 @@ func (c *baseCommand) context() *hookstate.Context {
 	return c.c
 }

+func (c *baseCommand) ensureContext() (context *hookstate.Context, err error) {
+	if c.c == nil {
+		err = fmt.Errorf(`The "snapctl %s" command can only be invoked from within a snap`, c.name)


the slight problem with this new error is that it makes almost sound like there are snapctl commands that can be invoked without a snap, but there are none.

The old messages weren't good but din't have this problem

I would use:

"cannot invoke snapctl operation commands (here %q) from outside of a snap", c.name

pedronis

my comments on error messages

pedronis · 2021-08-19T12:45:50Z

overlord/hookstate/ctlcmd/helpers.go

@@ -132,7 +132,7 @@ func queueCommand(context *hookstate.Context, tts []*state.TaskSet) error {
 func runServiceCommand(context *hookstate.Context, inst *servicestate.Instruction) error {
 	if context == nil {
 		// this message is reused in health.go
-		return fmt.Errorf(i18n.G("cannot %s without a context"), inst.Action)
+		return fmt.Errorf(i18n.G(`The snapctl %s action can only be invoked from within a snap`), inst.Action)


this should use the same message as well but command name here is inst.Action

@mardy was this part addressed?

Yes, it now uses the same error message as in other parts: https://github.com/mardy/snapd/blob/snapctl-context-err/overlord/hookstate/ctlcmd/helpers.go#L133-L135

(if you are merging this yourself please remember to squash :-) )

bboozzoo

LGTM

overlord/hookstate/ctlcmd/helpers.go

stolowski · 2021-08-24T08:31:01Z

overlord/hookstate/ctlcmd/ctlcmd.go

@@ -32,10 +32,19 @@ import (
 	"github.com/snapcore/snapd/strutil"
 )

+func createMissingContextError(subcommand string) error {
+	return fmt.Errorf(`Cannot invoke snapctl operation commands (here %q) from outside of a snap`, subcommand)


AFAICT errors in go lang shouldn't be capitalized (because they can be appended/wrapped in another error message and it then looks weird); I'm slightly surprised it's not caught by our static checks, I could swear I saw an error about such issue before...

It's because I used a cyrillic "С" instead ;-)

Fixed now.

codecov-commenter · 2021-08-24T09:32:43Z

Codecov Report

Merging #10606 (1a6ebbb) into master (8170654) will decrease coverage by 2.40%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10606      +/-   ##
==========================================
- Coverage   80.71%   78.30%   -2.41%     
==========================================
  Files         727      882     +155     
  Lines       58158    99422   +41264     
==========================================
+ Hits        46940    77856   +30916     
- Misses       7544    16665    +9121     
- Partials     3674     4901    +1227

Flag	Coverage Δ
unittests	`78.30% <ø> (?)`

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

Impacted Files	Coverage Δ
cmd/snap/wait.go
gadget/mountedfilesystem.go
logger/logger.go
daemon/api_systems.go
systemd/emulation.go
cmd/snap/cmd_model.go
interfaces/builtin/kubernetes_support.go
cmd/snap-update-ns/xdg.go
cmd/snap-update-ns/utils.go
interfaces/udev/udev.go
... and 1516 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 308f4eb...1a6ebbb. Read the comment docs.

stolowski

It nicely simplified these checks, thank you! +1

mvo5 · 2021-08-25T07:14:24Z

overlord/hookstate/ctlcmd/ctlcmd.go

@@ -32,10 +32,19 @@ import (
 	"github.com/snapcore/snapd/strutil"
 )

+func createMissingContextError(subcommand string) error {


(nitpick/bike-shed) I wonder if something like:

index 20b8b09067..76bac68240 100644 --- a/overlord/hookstate/ctlcmd/ctlcmd.go +++ b/overlord/hookstate/ctlcmd/ctlcmd.go @@ -32,8 +32,12 @@ import ( "github.com/snapcore/snapd/strutil" ) -func createMissingContextError(subcommand string) error { - return fmt.Errorf(`cannot invoke snapctl operation commands (here %q) from outside of a snap`, subcommand) +type MissingContextError struct { + subcommand string +} + +func (e *MissingContextError) Error() string { + return fmt.Sprintf(`cannot invoke snapctl operation commands (here %q) from outside of a snap`, e.subcommand) } type baseCommand struct { @@ -77,7 +81,7 @@ func (c *baseCommand) context() *hookstate.Context { func (c *baseCommand) ensureContext() (context *hookstate.Context, err error) { if c.c == nil { - err = createMissingContextError(c.name) + err = &MissingContextError{c.name} } return c.c, err } diff --git a/overlord/hookstate/ctlcmd/helpers.go b/overlord/hookstate/ctlcmd/helpers.go index 5260027224..595d99560b 100644 --- a/overlord/hookstate/ctlcmd/helpers.go +++ b/overlord/hookstate/ctlcmd/helpers.go @@ -131,7 +131,7 @@ func queueCommand(context *hookstate.Context, tts []*state.TaskSet) error { func runServiceCommand(context *hookstate.Context, inst *servicestate.Instruction) error { if context == nil { - return createMissingContextError(inst.Action) + return &MissingContextError{inst.Action} } st := context.State()

is a bit more idiomatic go. I.e. having an error type for this instead of creating different error strings. The end result is the same so it's not really a big deal but I could not not mention it :)

Thanks, applied!

Make the error message more user friendly, since the concept of a "context" is not obvious to the end user.

mardy · 2021-08-26T09:11:53Z

@mvo5, I'm afraid I need your magic powers to merge this. :-)

mardy added the Simple 😃 A small PR which can be reviewed quickly label Aug 11, 2021

MiguelPires reviewed Aug 11, 2021

View reviewed changes

MiguelPires approved these changes Aug 11, 2021

View reviewed changes

mvo5 reviewed Aug 12, 2021

View reviewed changes

stolowski reviewed Aug 16, 2021

View reviewed changes

anonymouse64 self-requested a review August 18, 2021 12:53

pedronis self-requested a review August 18, 2021 14:33

pedronis reviewed Aug 19, 2021

View reviewed changes

pedronis self-assigned this Aug 19, 2021

pedronis reviewed Aug 19, 2021

View reviewed changes

pedronis removed their assignment Aug 19, 2021

mardy added the Squash-merge Please squash this PR when merging. label Aug 20, 2021

bboozzoo approved these changes Aug 24, 2021

View reviewed changes

overlord/hookstate/ctlcmd/helpers.go Outdated Show resolved Hide resolved

stolowski reviewed Aug 24, 2021

View reviewed changes

stolowski approved these changes Aug 24, 2021

View reviewed changes

mvo5 reviewed Aug 25, 2021

View reviewed changes

o/hookstate/ctlcmd: unify the error message when context is missing

a23cc21

Make the error message more user friendly, since the concept of a "context" is not obvious to the end user.

mardy force-pushed the snapctl-context-err branch from 1a6ebbb to a23cc21 Compare August 25, 2021 07:28

anonymouse64 removed their request for review August 25, 2021 23:41

stolowski removed the Simple 😃 A small PR which can be reviewed quickly label Aug 26, 2021

mvo5 merged commit 03f6f5e into snapcore:master Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/hookstate/ctlcmd: unify the error message when context is missing #10606

o/hookstate/ctlcmd: unify the error message when context is missing #10606

mardy commented Aug 11, 2021

MiguelPires left a comment

mardy commented Aug 11, 2021

MiguelPires left a comment

mvo5 Aug 12, 2021 •

edited

mardy Aug 13, 2021

stolowski Aug 16, 2021

stolowski Aug 16, 2021

mardy Aug 16, 2021

stolowski Aug 16, 2021

mardy Aug 16, 2021

pedronis left a comment

pedronis Aug 19, 2021

pedronis Aug 19, 2021

mardy Aug 20, 2021

pedronis left a comment

pedronis Aug 19, 2021

mvo5 Aug 24, 2021

mardy Aug 24, 2021

bboozzoo left a comment

stolowski Aug 24, 2021

mardy Aug 24, 2021

stolowski Aug 24, 2021

codecov-commenter commented Aug 24, 2021

stolowski left a comment

mvo5 Aug 25, 2021 •

edited

mardy Aug 25, 2021

mardy commented Aug 26, 2021

o/hookstate/ctlcmd: unify the error message when context is missing #10606

o/hookstate/ctlcmd: unify the error message when context is missing #10606

Conversation

mardy commented Aug 11, 2021

MiguelPires left a comment

Choose a reason for hiding this comment

mardy commented Aug 11, 2021

MiguelPires left a comment

Choose a reason for hiding this comment

mvo5 Aug 12, 2021 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo 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

codecov-commenter commented Aug 24, 2021

Codecov Report

stolowski left a comment

Choose a reason for hiding this comment

mvo5 Aug 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mardy commented Aug 26, 2021

mvo5 Aug 12, 2021 •

edited

mvo5 Aug 25, 2021 •

edited