Update help text template to fix target usage #94

mpanchajanya · 2023-09-13T02:38:53Z

What this PR does / why we need it

Refactored the CmdTemplate help text to use the new go lang function that generated the help text templates.
The same const is used CmdTemplate with internal implementation changed from text template to use go func.
Additional fix to Bug Fix: Update the usage text for kubernetes target plugins #92

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Added unit tests for each plugin target i.e global, k8s, tmc

Manual Testing

Global Test Plugin

Before
Usage text doesn't show the tanzu prefix

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test -h
Test the CLI

Usage:
   test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test fetch -h
Fetch the plugin tests

Usage:
   test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)>

After

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu plugin list
Standalone Plugins
  NAME  DESCRIPTION   TARGET  VERSION     STATUS
  test  Test the CLI  global  v1.1.0-dev  installed
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test -h
Test the CLI

Usage:
  tanzu test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)>

Kubernetes Test Plugin

Before

Usage text commands are not properly indented

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main) [1]> tanzu test -h
Test the CLI

Usage:
  tanzu test [command]tanzu kubernetes test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.Use "tanzu kubernetes test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]tanzu kubernetes test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu k8s test -h
Test the CLI

Usage:
  tanzu test [command]tanzu kubernetes test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.Use "tanzu kubernetes test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu k8s test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]tanzu kubernetes test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)>

After

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu plugin list
Standalone Plugins
  NAME  DESCRIPTION   TARGET      VERSION     STATUS
  test  Test the CLI  kubernetes  v1.1.0-dev  installed
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test -h
Test the CLI

Usage:
  tanzu test [command]
  tanzu kubernetes test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.
Use "tanzu kubernetes test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]
  tanzu kubernetes test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu k8s test -h
Test the CLI

Usage:
  tanzu test [command]
  tanzu kubernetes test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu test [command] --help" for more information about a command.
Use "tanzu kubernetes test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu k8s test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]
  tanzu kubernetes test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)>

Mission Control Test Plugin

mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu plugin list
Standalone Plugins
  NAME  DESCRIPTION   TARGET           VERSION     STATUS
  test  Test the CLI  mission-control  v1.1.0-dev  installed
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main) [1]> tanzu tmc test -h
Test the CLI

Usage:
  tanzu mission-control test [command]

Aliases:
  test, t

Available Commands:
  fetch         Fetch the plugin tests
  push          Push the plugin tests

Flags:
  -h, --help   help for test

Additional help topics:
  test plugin        Plugin tests

Use "tanzu mission-control test [command] --help" for more information about a command.
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)> tanzu tmc test fetch -h
Fetch the plugin tests

Usage:
  tanzu mission-control test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository
mpanchajanya@mpanchajanF09MK ~/v/r/tanzu-cli (main)>

Release note

Fix plugin help text for global, kubernetes, mission-control targets

Additional information

Special notes for your reviewer

marckhouzam

This is much more readable! Nice use of {{- -}} 👏

There is an extra empty line at the very end of the help for commands that are runnable:

$ tz test fetch -h
Fetch the plugin tests

Usage:
  tanzu test fetch [flags]

Flags:
  -h, --help           help for fetch
  -l, --local string   path to local repository

$ # Notice the new extra line before this new prompt

plugin/usage.go

Signed-off-by: Marc Khouzam <[email protected]>

marckhouzam · 2023-09-13T15:15:34Z

I've been wondering if there was a way to get rid of the go templating completely.
After some digging I came up with a way to stop using go templates but instead use plain Go code to generate the help.

After discussing it with @mpanchajanya we decided to add this change on top of this PR.

mpanchajanya · 2023-09-13T18:44:45Z

I've been wondering if there was a way to get rid of the go templating completely. After some digging I came up with a way to stop using go templates but instead use plain Go code to generate the help.

After discussing it with @mpanchajanya we decided to add this change on top of this PR.

Thanks @marckhouzam for the go implementation alternate to string template. This is way better.
I have refactored the printHelp to be more efficient and avoid lint issues.

marckhouzam

Looks super nice!

marckhouzam · 2023-09-13T19:57:16Z

plugin/usage.go


-{{ bold "Available Commands:" }}{{range .Commands}}{{if .IsAvailableCommand }}
-  {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableLocalFlags}}
+	output.WriteString(component.Bold(usageStr) + "\n")


Since you've done such a nice encapsulation job, making this function nicely readable, how about moving this line to the formatUsageHelpSection()?

plugin/usage.go

marckhouzam · 2023-09-13T20:14:59Z

plugin/usage.go

+	// For kubernetes, k8s, global, or no target display tanzu command path without target
+	if target == types.TargetK8s || target == types.TargetGlobal || target == types.TargetUnknown {
+		footer.WriteString(`Use "`)
+		if !strings.HasSuffix(cmd.CommandPath(), "tanzu ") {


Woops, this is a typo from my own commit. HasSuffix() should actually be HasPrefix() here and a couple of lines below.

marckhouzam · 2023-09-13T20:17:14Z

plugin/usage.go

@@ -22,32 +24,142 @@ var UsageFunc = func(c *cobra.Command) error {
 }

 // CmdTemplate is the template for plugin commands.
-const CmdTemplate = `{{ bold "Usage:" }}
-  {{if .Runnable}}{{ $target := index .Annotations "target" }}{{ if or (eq $target "kubernetes") (eq $target "k8s") }}tanzu {{.UseLine}}{{ end }}{{ if and (ne $target "global") (ne $target "") }}tanzu {{ $target }} {{ else }} {{ end }}{{.UseLine}}{{end}}{{if .HasAvailableSubCommands}}{{ $target := index .Annotations "target" }}{{ if or (eq $target "kubernetes") (eq $target "k8s") }}tanzu {{.CommandPath}} [command]{{end}}{{ if and (ne $target "global") (ne $target "") }}tanzu {{ $target }} {{ else }} {{ end }}{{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}}
+const CmdTemplate = `{{ printHelp . }}`


I don't feel like this change has any backwards-compatibility issues, but I'm not sure.
Any opinions here?

I feel like this change is backward compatible since we are using the same constant and instead of string template we refactored the template generation into a go func that returns template.

@vuil @anujc25 can add any opinions here

@marckhouzam you mean as in the fact that the output has changed in general, or something more specific?

vuil · 2023-09-13T20:25:36Z

Nit: @mpanchajanya it is really hard to tell after the changes how much/little of the output has changed. It would help the review if:
in the PR description's command help outputs, either highlight what is added/deleted/modified or just include a diff of the outputs before vs after the changes.

marckhouzam · 2023-09-13T20:25:35Z

plugin/usage_test.go

+		RunE: func(cmd *cobra.Command, args []string) error {
+			fmt.Println("fetch")
+			return nil
+		},


Could you add Examples here and then test using fetch -h?
It will test the examples printout but also the case where the command is Runnable, which I don't believe we test

added tests to validate fetch --help cmd.

marckhouzam · 2023-09-13T20:26:23Z

plugin/usage_test.go

+
+	fetchCmd.Flags().StringVarP(&local, "local", "l", "", "path to local repository")
+	_ = fetchCmd.MarkFlagRequired("local")
+


Could you add a global flag by adding a persistent flag to the top command so we can test that case

Added tests with global flags

mpanchajanya · 2023-09-13T21:00:32Z

Nit: @mpanchajanya it is really hard to tell after the changes how much/little of the output has changed. It would help the review if: in the PR description's command help outputs, either highlight what is added/deleted/modified or just include a diff of the outputs before vs after the changes.

@vuil Updated the PR description with Before and After usage texts

vuil · 2023-09-13T21:11:32Z

Nit: @mpanchajanya it is really hard to tell after the changes how much/little of the output has changed. It would help the review if: in the PR description's command help outputs, either highlight what is added/deleted/modified or just include a diff of the outputs before vs after the changes.

The other related thing I wanted to add but forgot is: If addressing an issue involves essentially porting from one implementation to another (template-based logic vs straight golang code are two quite different things), I would strongly encourage we break up the change into a straight port, have existing (or even new tests) tests confirms that there is no observable behavior change, then proceed with another commit to apply the actual fix.

mpanchajanya · 2023-09-13T21:24:21Z

Nit: @mpanchajanya it is really hard to tell after the changes how much/little of the output has changed. It would help the review if: in the PR description's command help outputs, either highlight what is added/deleted/modified or just include a diff of the outputs before vs after the changes.

The other related thing I wanted to add but forgot is: If addressing an issue involves essentially porting from one implementation to another (template-based logic vs straight golang code are two quite different things), I would strongly encourage we break up the change into a straight port, have existing (or even new tests) tests confirms that there is no observable behavior change, then proceed with another commit to apply the actual fix.

There are no automated tests earlier to confirm the actual change. Its was all manual tests.
With the new tests added yes I have validated the previous and new help text outputs

…update all usages

marckhouzam

LGTM
Thanks for the great tests and working out all the kinks.

vuil

LGTM, thanks for the fixes and tests!

* Update help text template to fix target usage * Add two spaces for tanzu command * Convert the help template to go code Signed-off-by: Marc Khouzam <[email protected]> * testing the lint and tests * refactor the print help * refactor the indentation * Update check to HasPrefix from HasSuffix * Update tests to include global flags and fetch --help command * Update additional help topics section with tanzu and target prefix * Revert additional help topics targeted prefix * Deprecate CmdTemplate and introduce private cmdTemplate variable and update all usages * Fix sub command unit tests by executing plugin instead of command * Fix spacing in examples section and update tests --------- Signed-off-by: Marc Khouzam <[email protected]> Co-authored-by: Marc Khouzam <[email protected]> (cherry picked from commit 3923e33)

vmwclabot added the cla-not-required label Sep 13, 2023

mpanchajanya self-assigned this Sep 13, 2023

mpanchajanya requested review from marckhouzam and anujc25 September 13, 2023 02:49

Update help text template to fix target usage

b38be7f

mpanchajanya force-pushed the fix_help_text_usage branch from 571ad89 to b38be7f Compare September 13, 2023 02:53

mpanchajanya marked this pull request as ready for review September 13, 2023 02:53

mpanchajanya requested a review from a team as a code owner September 13, 2023 02:53

marckhouzam reviewed Sep 13, 2023

View reviewed changes

plugin/usage.go Outdated Show resolved Hide resolved

plugin/usage.go Outdated Show resolved Hide resolved

plugin/usage.go Outdated Show resolved Hide resolved

plugin/usage.go Outdated Show resolved Hide resolved

mpanchajanya and others added 2 commits September 13, 2023 09:50

Add two spaces for tanzu command

a78d91e

Convert the help template to go code

4fad4b7

Signed-off-by: Marc Khouzam <[email protected]>

mpanchajanya added 2 commits September 13, 2023 12:38

testing the lint and tests

c8c9c53

refactor the print help

fe30e54

mpanchajanya requested a review from marckhouzam September 13, 2023 18:51

refactor the indentation

acbb988

marckhouzam reviewed Sep 13, 2023

View reviewed changes

Update check to HasPrefix from HasSuffix

f092864

marckhouzam reviewed Sep 13, 2023

View reviewed changes

mpanchajanya requested a review from vuil September 13, 2023 20:26

Update tests to include global flags and fetch --help command

5c0e51d

mpanchajanya force-pushed the fix_help_text_usage branch from 365090d to 5c0e51d Compare September 13, 2023 20:49

Update additional help topics section with tanzu and target prefix

697910b

mpanchajanya force-pushed the fix_help_text_usage branch from f88def7 to 697910b Compare September 13, 2023 22:41

mpanchajanya added 2 commits September 14, 2023 09:53

Revert additional help topics targeted prefix

b8f20cb

Deprecate CmdTemplate and introduce private cmdTemplate variable and …

64f846b

…update all usages

mpanchajanya requested a review from marckhouzam September 14, 2023 16:48

mpanchajanya added 2 commits September 14, 2023 15:40

Fix sub command unit tests by executing plugin instead of command

444c296

Fix spacing in examples section and update tests

ecb59f4

marckhouzam approved these changes Sep 15, 2023

View reviewed changes

vuil approved these changes Sep 15, 2023

View reviewed changes

mpanchajanya merged commit 3923e33 into vmware-tanzu:main Sep 15, 2023
4 checks passed

marckhouzam added this to the v1.0.2 milestone Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update help text template to fix target usage #94

Update help text template to fix target usage #94

mpanchajanya commented Sep 13, 2023 •

edited

Loading

marckhouzam left a comment

marckhouzam commented Sep 13, 2023

mpanchajanya commented Sep 13, 2023

marckhouzam left a comment

marckhouzam Sep 13, 2023

mpanchajanya Sep 14, 2023

marckhouzam Sep 13, 2023

marckhouzam Sep 13, 2023

mpanchajanya Sep 13, 2023 •

edited

Loading

vuil Sep 13, 2023

vuil commented Sep 13, 2023 •

edited

Loading

marckhouzam Sep 13, 2023

mpanchajanya Sep 13, 2023 •

edited

Loading

marckhouzam Sep 13, 2023

mpanchajanya Sep 13, 2023

mpanchajanya commented Sep 13, 2023

vuil commented Sep 13, 2023

mpanchajanya commented Sep 13, 2023

marckhouzam left a comment

vuil left a comment


		fetchCmd.Flags().StringVarP(&local, "local", "l", "", "path to local repository")
		_ = fetchCmd.MarkFlagRequired("local")

Update help text template to fix target usage #94

Update help text template to fix target usage #94

Conversation

mpanchajanya commented Sep 13, 2023 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

marckhouzam left a comment

Choose a reason for hiding this comment

marckhouzam commented Sep 13, 2023

mpanchajanya commented Sep 13, 2023

marckhouzam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpanchajanya Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuil commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

mpanchajanya Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpanchajanya commented Sep 13, 2023

vuil commented Sep 13, 2023

mpanchajanya commented Sep 13, 2023

marckhouzam left a comment

Choose a reason for hiding this comment

vuil left a comment

Choose a reason for hiding this comment

mpanchajanya commented Sep 13, 2023 •

edited

Loading

mpanchajanya Sep 13, 2023 •

edited

Loading

vuil commented Sep 13, 2023 •

edited

Loading

mpanchajanya Sep 13, 2023 •

edited

Loading