-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
cli: Improve error handling for plugin commands #24250
Conversation
Build Results: |
CI Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from some minor nits, it seems good to me. I guess its better to get this breaking change out sooner rather than later, and it is definitely a good step forward.
@@ -206,7 +206,7 @@ func TestPluginDeregisterCommand_Run(t *testing.T) { | |||
t.Errorf("expected %d to be %d", code, exp) | |||
} | |||
|
|||
expected := "Success! Deregistered plugin (if it was registered): " | |||
expected := "does not exist in the catalog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if we should change the test below to be a bit more specific by using strings.HasSuffix
rather than strings.Contains
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already pretty uncomfortable with how brittle these tests are tbh (string-matching on error messages that can be arbitrarily updated), and it doesn't feel like that meaningfully improves the intent of the test, but it does make it more brittle. Unless you disagree strongly, I'd prefer to leave it as-is.
Thanks! |
vault plugin info
andvault plugin deregister
have accepted either 1 or 2 positional args since we introduced types for plugins way back in v1.0.0 (#5536). This PR drops CLI support for type-less plugins on those commands, but the API can still be used to query any extremely old legacy plugins (although based on the plugin catalog's UpgradePlugins function, run during unseal, I don't think it should be possible for v1.0.0+ versions of Vault to have functioning type-less plugins). In return, we can get a much nicer UX on the CLI. It also improves the error reporting forvault plugin reload
, which was simply buggy.Examples of previous bad behaviour:
To exercise all the possible
vault plugin deregister
outcomes and get a feel for the UX: