Plugins: Fail plugins installation on wrong args provided #71355
Conversation
oshirohugo
requested review from
sakjur,
papagian and
yangkb09
and removed request for
a team
| @@ -20,6 +20,11 @@ import ( | |||
| ) | |||
|
|
|||
| func validateInput(c utils.CommandLine, pluginFolder string) error { | |||
There was a problem hiding this comment.
pluginFolder argument isn't used inside this function and could be removed as well
There was a problem hiding this comment.
you can remove it then :)
oshirohugo
changed the title
sakjur
requested review from
a team,
wbrowne and
marefr
and removed request for
a team
There was a problem hiding this comment.
The lack of this functionality is surprising 😅 I checked it and if you copy the flag to the sub command (so you have the flag --pluginsDir in the install command), it no longer works if you specify it at the root level (so grafana cli --pluginsDir foo install bar would stop working).
Anyway, thanks for the investigation! I have some comments.
| if c.Args().Len() > 1 { | ||
| logger.Info("Install only supports one local argument\n") | ||
| showIgnoredArguments(c.Args().Slice()[1:]) | ||
| } |
There was a problem hiding this comment.
IMO a better UX would be to error, the log line can be missed (and the plugin may be installed in a directory the user does not want to, for example).
There was a problem hiding this comment.
Also, the length to check should be 2, not 1 (it's possible to specify the version, apart from the name). This has the caveat that if you specify ... install foo --pluginsDir=bar, the number of arguments is correct but it will fail anyway when trying to use the flag as a version.
oshirohugo
changed the title
oshirohugo
changed the title
|
@wbrowne could you please take a look. There was a change on the first proposal after @andresmgot suggestions and now we are failing the command. |
There was a problem hiding this comment.
A shame about the CLI package limitations, but definitely an improvement. Had an idea about potentially adding a log statement.
Think it would be good to follow this PR up with version field validation to avoid the possibility of using an "incorrect" single argument where version should be (like what @andresmgot mentioned in a comment).
Great work!
|
|
||
| func validateInput(c utils.CommandLine) error { | ||
| if c.Args().Len() > installArgsSize { | ||
| return errors.New("install only supports 2 arguments: plugin and version") |
There was a problem hiding this comment.
Maybe before we return the error here, we should add a log with something more descriptive to hopefully help the user form the right command.
For example:
./bin/darwin-amd64/grafana cli plugins install autohome-comparequeries-datasource --debug --pluginUrl /Users/wbrowne/Downloads/autohome-comparequeries-datasource.zip
Please specify the correct format. For example ./grafana cli (<command arguments>) plugins install <plugin ID> (<plugin version>)
Error: ✗ install only supports 2 arguments: plugin and version
| return errors.New("install only supports 2 arguments: plugin and version") | |
| logger.Info(color.RedString("Please specify the correct format. For example ./grafana cli (<command arguments>) plugins install <plugin ID> (<plugin version>)\n\n")) | |
| return errors.New("install only supports 2 arguments: plugin and version") |
Maybe @sympatheticmoose/@josmperez can weigh in here on best lingo
There was a problem hiding this comment.
For reference, this would look like:
* Return error on plugin install extra args * Remove unused valid argument * Add log for wrong installation args
What is this feature?
Fail plugin installation when more args than needed are provided
Why do we need this feature?
This is needed because we use hierarchical flags and arguments and we need to feedback the user if an argument is being ignored.
Who is this feature for?
Fixes #70056
Special notes for your reviewer:
The command line package that we use to parse our flags and arguments is based on hierarchical flags.
--pluginsDiris defined in the upper command (cli) to be availabel to allpluginssubcommands. The library doesn't seem to support a global flag being passed after a subcommand. This is confirmed by this maintainer comment and by this comment on an open issePlease check that: