Fix help text for plugins

When using `CommandDisplayNameAnnotation` we want to use it instead of
the command name in `--help` message or in the default help command.

With current code we get the wrong text in the --help usage text:

    Flags:
      -h, --help   help for kubectl-plugin

And in the long description of the default help command:

    $ kubectl cobraplugin help -h
    Help provides help for any command in the application.
    Simply type kubectl-plugin help [path to command] for full details.

The issue was hidden since the test checked only the Usage line.

Fixed by extracting a displayName() function and use it when creating
FlagSet and when formatting the default help flag usage and the help
command long description.

Enhance the TestPlugin to check all the lines including the command
name.
This commit is contained in:
Nir Soffer 2023-11-12 17:20:47 +02:00 committed by Marc Khouzam
parent e63925d321
commit df547f5fc6
2 changed files with 28 additions and 12 deletions

View file

@ -1187,10 +1187,11 @@ func (c *Command) InitDefaultHelpFlag() {
c.mergePersistentFlags() c.mergePersistentFlags()
if c.Flags().Lookup("help") == nil { if c.Flags().Lookup("help") == nil {
usage := "help for " usage := "help for "
if c.Name() == "" { name := c.displayName()
if name == "" {
usage += "this command" usage += "this command"
} else { } else {
usage += c.Name() usage += name
} }
c.Flags().BoolP("help", "h", false, usage) c.Flags().BoolP("help", "h", false, usage)
_ = c.Flags().SetAnnotation("help", FlagSetByCobraAnnotation, []string{"true"}) _ = c.Flags().SetAnnotation("help", FlagSetByCobraAnnotation, []string{"true"})
@ -1236,7 +1237,7 @@ func (c *Command) InitDefaultHelpCmd() {
Use: "help [command]", Use: "help [command]",
Short: "Help about any command", Short: "Help about any command",
Long: `Help provides help for any command in the application. Long: `Help provides help for any command in the application.
Simply type ` + c.Name() + ` help [path to command] for full details.`, Simply type ` + c.displayName() + ` help [path to command] for full details.`,
ValidArgsFunction: func(c *Command, args []string, toComplete string) ([]string, ShellCompDirective) { ValidArgsFunction: func(c *Command, args []string, toComplete string) ([]string, ShellCompDirective) {
var completions []string var completions []string
cmd, _, e := c.Root().Find(args) cmd, _, e := c.Root().Find(args)
@ -1427,6 +1428,10 @@ func (c *Command) CommandPath() string {
if c.HasParent() { if c.HasParent() {
return c.Parent().CommandPath() + " " + c.Name() return c.Parent().CommandPath() + " " + c.Name()
} }
return c.displayName()
}
func (c *Command) displayName() string {
if displayName, ok := c.Annotations[CommandDisplayNameAnnotation]; ok { if displayName, ok := c.Annotations[CommandDisplayNameAnnotation]; ok {
return displayName return displayName
} }
@ -1642,7 +1647,7 @@ func (c *Command) GlobalNormalizationFunc() func(f *flag.FlagSet, name string) f
// to this command (local and persistent declared here and by all parents). // to this command (local and persistent declared here and by all parents).
func (c *Command) Flags() *flag.FlagSet { func (c *Command) Flags() *flag.FlagSet {
if c.flags == nil { if c.flags == nil {
c.flags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.flags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
if c.flagErrorBuf == nil { if c.flagErrorBuf == nil {
c.flagErrorBuf = new(bytes.Buffer) c.flagErrorBuf = new(bytes.Buffer)
} }
@ -1656,7 +1661,7 @@ func (c *Command) Flags() *flag.FlagSet {
func (c *Command) LocalNonPersistentFlags() *flag.FlagSet { func (c *Command) LocalNonPersistentFlags() *flag.FlagSet {
persistentFlags := c.PersistentFlags() persistentFlags := c.PersistentFlags()
out := flag.NewFlagSet(c.Name(), flag.ContinueOnError) out := flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
c.LocalFlags().VisitAll(func(f *flag.Flag) { c.LocalFlags().VisitAll(func(f *flag.Flag) {
if persistentFlags.Lookup(f.Name) == nil { if persistentFlags.Lookup(f.Name) == nil {
out.AddFlag(f) out.AddFlag(f)
@ -1670,7 +1675,7 @@ func (c *Command) LocalFlags() *flag.FlagSet {
c.mergePersistentFlags() c.mergePersistentFlags()
if c.lflags == nil { if c.lflags == nil {
c.lflags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.lflags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
if c.flagErrorBuf == nil { if c.flagErrorBuf == nil {
c.flagErrorBuf = new(bytes.Buffer) c.flagErrorBuf = new(bytes.Buffer)
} }
@ -1697,7 +1702,7 @@ func (c *Command) InheritedFlags() *flag.FlagSet {
c.mergePersistentFlags() c.mergePersistentFlags()
if c.iflags == nil { if c.iflags == nil {
c.iflags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.iflags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
if c.flagErrorBuf == nil { if c.flagErrorBuf == nil {
c.flagErrorBuf = new(bytes.Buffer) c.flagErrorBuf = new(bytes.Buffer)
} }
@ -1725,7 +1730,7 @@ func (c *Command) NonInheritedFlags() *flag.FlagSet {
// PersistentFlags returns the persistent FlagSet specifically set in the current command. // PersistentFlags returns the persistent FlagSet specifically set in the current command.
func (c *Command) PersistentFlags() *flag.FlagSet { func (c *Command) PersistentFlags() *flag.FlagSet {
if c.pflags == nil { if c.pflags == nil {
c.pflags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.pflags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
if c.flagErrorBuf == nil { if c.flagErrorBuf == nil {
c.flagErrorBuf = new(bytes.Buffer) c.flagErrorBuf = new(bytes.Buffer)
} }
@ -1738,9 +1743,9 @@ func (c *Command) PersistentFlags() *flag.FlagSet {
func (c *Command) ResetFlags() { func (c *Command) ResetFlags() {
c.flagErrorBuf = new(bytes.Buffer) c.flagErrorBuf = new(bytes.Buffer)
c.flagErrorBuf.Reset() c.flagErrorBuf.Reset()
c.flags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.flags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
c.flags.SetOutput(c.flagErrorBuf) c.flags.SetOutput(c.flagErrorBuf)
c.pflags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.pflags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
c.pflags.SetOutput(c.flagErrorBuf) c.pflags.SetOutput(c.flagErrorBuf)
c.lflags = nil c.lflags = nil
@ -1857,7 +1862,7 @@ func (c *Command) mergePersistentFlags() {
// If c.parentsPflags == nil, it makes new. // If c.parentsPflags == nil, it makes new.
func (c *Command) updateParentsPflags() { func (c *Command) updateParentsPflags() {
if c.parentsPflags == nil { if c.parentsPflags == nil {
c.parentsPflags = flag.NewFlagSet(c.Name(), flag.ContinueOnError) c.parentsPflags = flag.NewFlagSet(c.displayName(), flag.ContinueOnError)
c.parentsPflags.SetOutput(c.flagErrorBuf) c.parentsPflags.SetOutput(c.flagErrorBuf)
c.parentsPflags.SortFlags = false c.parentsPflags.SortFlags = false
} }

View file

@ -371,7 +371,7 @@ func TestAliasPrefixMatching(t *testing.T) {
// text should reflect the way we run the command. // text should reflect the way we run the command.
func TestPlugin(t *testing.T) { func TestPlugin(t *testing.T) {
rootCmd := &Command{ rootCmd := &Command{
Use: "plugin", Use: "kubectl-plugin",
Args: NoArgs, Args: NoArgs,
Annotations: map[string]string{ Annotations: map[string]string{
CommandDisplayNameAnnotation: "kubectl plugin", CommandDisplayNameAnnotation: "kubectl plugin",
@ -387,6 +387,8 @@ func TestPlugin(t *testing.T) {
} }
checkStringContains(t, rootHelp, "kubectl plugin [command]") checkStringContains(t, rootHelp, "kubectl plugin [command]")
checkStringContains(t, rootHelp, "help for kubectl plugin")
checkStringContains(t, rootHelp, "kubectl plugin [command] --help")
childHelp, err := executeCommand(rootCmd, "sub", "-h") childHelp, err := executeCommand(rootCmd, "sub", "-h")
if err != nil { if err != nil {
@ -394,6 +396,15 @@ func TestPlugin(t *testing.T) {
} }
checkStringContains(t, childHelp, "kubectl plugin sub [flags]") checkStringContains(t, childHelp, "kubectl plugin sub [flags]")
checkStringContains(t, childHelp, "help for sub")
helpHelp, err := executeCommand(rootCmd, "help", "-h")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
checkStringContains(t, helpHelp, "kubectl plugin help [path to command]")
checkStringContains(t, helpHelp, "kubectl plugin help [command]")
} }
// TestChildSameName checks the correct behaviour of cobra in cases, // TestChildSameName checks the correct behaviour of cobra in cases,