cmd: Friendly error messages when plugin not found #432
Conversation
Also remove line from run-tests.sh for file that was deleted in kubernetes-sigs/krew@e37ee1d#diff-d78b6f30225cdc811adfe8d4e7c9fd34
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
Codecov Report
@@ Coverage Diff @@
## master #432 +/- ##
=======================================
Coverage 56.35% 56.35%
=======================================
Files 19 19
Lines 928 928
=======================================
Hits 523 523
Misses 350 350
Partials 55 55Continue to review full report at Codecov.
|
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
I think this wouldn't work currently, because (1) github/pkg/errors.Wrap doesn't wrap the errors in a way the new go1.13 errors.Is() can use (2) the rest of the codebase must migrate to the errors package properly before we start incorporating parts of it. |
cmd/krew/cmd/install.go
Outdated
| if _, ok := installed[name]; ok { | ||
| fmt.Fprintf(os.Stderr, "plugin \"%s\" is already installed\n", name) | ||
| skipped = true |
There was a problem hiding this comment.
I'm not sure what this part does. We already have the skipping behavior implemented (see below), we can't implement it again like this. I don't see how this changes anything?
$ kubectl krew install krew
Updated the local copy of plugin index.
Installing plugin: krew
W1209 19:07:29.190869 69309 install.go:124] Skipping plugin "krew", it is already installed
There was a problem hiding this comment.
I guess it helps us not look up the deleted manifest of a plugin that's already installed. I don't think we need this, the error you added below does a proper job.
There was a problem hiding this comment.
I thought that skipping the manifest lookup for an already installed plugin and displaying a different message was what we wanted for this based on your comment here:
... we try to load manifest from fs again, I wonder why. we should be treating it like ErrAlreadyInstalled and say it's already installed
If its not necessary I can totally remove it, but I thought it seemed a little nicer.
Another reason I added it was for the case when the plugin(s) that are being installed don't exist so that the help message doesn't get printed. It felt out of place to show that as well.
There was a problem hiding this comment.
Yeah, I think it's making the code unnecessarily more complex and it solves a small problem that rarely occurs if ever (installing an installed plugin directly by name, after its manifest is which occurred after installing it for the first time). Let's abandon this.
There was a problem hiding this comment.
Sounds good, I'll remove it
cmd/krew/cmd/install.go
Outdated
| plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPluginsPath(), name) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return fmt.Errorf("plugin \"%s\" does not exist in the plugin index", name) |
There was a problem hiding this comment.
you can use %q.
and also please use errors.New, as it creates a stacktrace.
|
/retitle cmd: Friendly error messages when plugin not found |
k8s-ci-robot
changed the title
k8s-ci-robot
added
size/XS
emove check on installed plugin and use %q and errors.New
chriskim06
force-pushed
the
error-messaging
branch
from
3f08f70
to
6700aa0
Compare
cmd/krew/cmd/install.go
Outdated
| @@ -89,6 +89,9 @@ Remarks: | |||
| for _, name := range pluginNames { | |||
| plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPluginsPath(), name) | |||
| if err != nil { | |||
| if os.IsNotExist(err) { | |||
| return errors.New(fmt.Sprintf("plugin %q does not exist in the plugin index", name)) | |||
There was a problem hiding this comment.
sorry for back and forth but you can use errors.Errorf for this :)
There was a problem hiding this comment.
No worries, I'll make the change tomorrow
|
/lgtm |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, chriskim06 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Fixes #431
I decided to just use
os.IsNotExiston the error rather thanerrors.Issince there's a package name conflict with the existinggit.luolix.top/pkg/errorspackage. I was thinking about using the new %w verb infmt.Errorfbut noticed that you lose stack trace info when setting the log level. There's an existing issue, #414, that is open to consider using 1.13 logging so I figured I'd use this for now and we could change it when the other issue gets figured out.