-
Notifications
You must be signed in to change notification settings - Fork 15
Extend plugin mapping to command-level remapping #177
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
Conversation
anujc25
left a comment
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.
Looks good 👍 . Just a few really minor comments.
ee0eb15 to
34eeb73
Compare
marckhouzam
left a comment
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.
Very nice. I like the clear way for a plugin to indicate any mapping. I also like that there can be multiple mappings and from sub-commands. Very powerful.
We'll need some release notes for this nice change.
This is pretty complex stuff to think about so I'm not quite done but here are minor comments to start.
| return cmd.UseLine() | ||
| } | ||
|
|
||
| // TODO(vuil) look into still incorporating relevant parts of UseLine into output |
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.
note so we don't forget this as a follow-up
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.
Fixed in #180
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.
The aliases need some tweaks in the help. Specifically, if a plugin sub-command is mapped to a different name. For example, I mapped tanzu builder cli to tanzu mycli but then the alias is incorrect:
$ tz mycli -h
Build CLIs
Usage:
tanzu mycli
tanzu kubernetes mycli
tanzu mycli [command]
tanzu kubernetes mycli [command]
Aliases:
cli, c
[...]
$ tz cli -h
[x] : unknown command "cli" for "tanzu"
Did you mean this?
mycli
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.
Thanks, the issue is with still assuming the name of the plugin is always a valid alias entry (the first item int the alias list), which is no longer true for a mapped command. I will address it in a followup.
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.
Fixed in #180
34eeb73 to
63e97b6
Compare
marckhouzam
left a comment
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.
Once the missing quote x2 is fixed (see comments) and that CI passes, this LGTM.
Nice job @vuil, this is some powerful stuff!
63e97b6 to
da4a99a
Compare
Replaces InvokedAs field in the PluginDescriptor with a list of CommandMapEntry's that can provide additional mapping details. In particular it can specify a destination command path of the Tanzu CLI to map to a specific non root command path in the plugin. Adds support for capturing the invocation "context" by consuming environment variables that are expected to be set by the Tanzu CLI. Information in this context allows the plugin to construct proper Help information, and learn if the its command being invoked is done via a command-level mapping or not. Updates the usage to provides better output when help is invoked on a command-level mapping.
The aliases and description of a remapped subcommand CommandMapEntry should default to the Aliases and Short values of the Command in the plugin. Hence, the 'info' command will interrogate the plugin command tree for these values unless they are already explicitly specified in the CommandMapEntry.
- and some typos, linter warnings Signed-off-by: Vui Lam <[email protected]>
da4a99a to
c8b12e5
Compare
What this PR does / why we need it
Replaces InvokedAs field in the PluginDescriptor with a list of CommandMapEntry's that can provide additional mapping details. In particular it can specify a destination command path of the Tanzu CLI to map to a specific non root command path in the plugin.
Adds support for capturing the invocation "context" by consuming environment variables that are expected to be set by the Tanzu CLI when invoking a command that is being mapped.
Information in this context allows the plugin to construct proper Help information, and learn if the its command being invoked is done via a command-level mapping or not.
Updates the usage to provides better output when help is invoked on a command-level mapping.
Some additional handling added to propagate aliases and description of a mapped subcommand.
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer