Add global --verbose flag #691

mpanchajanya · 2024-03-07T15:14:27Z

What this PR does / why we need it

Introduce a global flag --verbose of type int with values 0-9 to set the log verbosity level for a tanzu command.
For Plugin commands :- If plugin root command support --verbose flag the CLI would send the flag args to plugin.

If a plugin implement --verbose flag of any type int, bool, string that flag args are passed to plugin by the Tanzu CLI

Tanzu CLI only supports --verbose flag of int type
For Example :- When user runs

tanzu --verbose 5 telemetry version
# The CLI will set the log verbosity to 5 and also passes the --verbose flag to telemetry plugin if it supports.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

For Tanzu CLI Commands with different test log levels

> tz --verbose 10 version
[x] : Invalid value for verbose flag. It should be between 0 and 9
> tz --verbose true version
[x] : invalid argument "true" for "--verbose" flag: strconv.ParseInt: parsing "true": invalid syntax
> tz --verbose=true version
[x] : invalid argument "true" for "--verbose" flag: strconv.ParseInt: parsing "true": invalid syntax
> tz --verbose -10 version
[x] : Invalid value for verbose flag. It should be between 0 and 9

> tz version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> tz --verbose 0 version
[i] I am level default
[i] I am level 0
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> tz --verbose 2 version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> tz --verbose 5 version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] I am level 4
[i] I am level 5
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> TANZU_CLI_LOG_LEVEL=0 tz version
[i] I am level default
[i] I am level 0
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> TANZU_CLI_LOG_LEVEL=3 tz version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

> TANZU_CLI_LOG_LEVEL=6 tz version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] I am level 4
[i] I am level 5
[i] I am level 6
version: v1.3.0-dev
buildDate: 2024-03-13
sha: 61a35e05
arch: arm64

Created test plugins logb with boolean verbose flag, logi with int verbose flag

> tz --verbose 2 logi
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] [Plugin] I am level default # Test logs from the plugin
[i] [Plugin] I am level 0
[i] [Plugin] I am level 1
[i] [Plugin] I am level 2

> tz --verbose -2 logi
[x] : Missing value for verbose flag

> tz --verbose 6 logi
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] I am level 4
[i] I am level 5
[i] I am level 6
[i] [Plugin] I am level default
[i] [Plugin] I am level 0
[i] [Plugin] I am level 1
[i] [Plugin] I am level 2
[i] [Plugin] I am level 3
[i] [Plugin] I am level 4
[i] [Plugin] I am level 5
[i] [Plugin] I am level 6

> tz --verbose logi
[x] : invalid argument "logi" for "--verbose" flag: strconv.ParseInt: parsing "logi": invalid syntax


# For boolean verbose flag plugin

> tz --verbose=true logb
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] [Plugin] I am level default
[i] [Plugin] I am level 0
[i] [Plugin] I am level 1
[i] [Plugin] I am level 2
[i] [Plugin] I am level 3

> tz --verbose=false logb
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] [Plugin] I am level default
[i] [Plugin] I am level 0
[i] [Plugin] I am level 1
[i] [Plugin] I am level 2
[i] [Plugin] I am level 3

With existing plugin that doesn't support verbose flag

# The verbosity is only set in the Tanzu CLI and since builder plugin doesn't support verbose flag CLI doesn't pass the flag
> tz --verbose 5 builder version
[i] I am level default
[i] I am level 0
[i] I am level 1
[i] I am level 2
[i] I am level 3
[i] I am level 4
[i] I am level 5
v1.2.0

> tz --verbose 0 builder version
[i] I am level default
[i] I am level 0
v1.2.0

Invalid verbose flag values

> tz version --verbose
[x] : flag needs an argument: --verbose

> tz --verbose version
[x] : invalid argument "version" for "--verbose" flag: strconv.ParseInt: parsing "version": invalid syntax

> tz version --verbose hi
[x] : invalid argument "hi" for "--verbose" flag: strconv.ParseInt: parsing "hi": invalid syntax

> tz version --verbose -h
[x] : invalid argument "-h" for "--verbose" flag: strconv.ParseInt: parsing "-h": invalid syntax

Release note

Implement a global log level verbose flag --verbose

Additional information

Special notes for your reviewer

marckhouzam

Thanks for this. This may seem like a small change, but it is quite tricky.

Nit: can you fix the commit message? One title line, then an empty line, then a description.

I noticed that the style guide recommends plugins support a --verbose flag. Could you update that section of the doc?

pkg/command/root.go

marckhouzam · 2024-03-12T12:55:43Z

pkg/command/root.go

+			log.V(3).Info("I am level 3")
+			log.V(4).Info("I am level 4")
+			log.V(5).Info("I am level 5")
+			log.V(6).Info("I am level 6")


I think it should be relatively easy to write a unit test to verify the new global flag.
Then you can use that test instead of these printouts 😄

marckhouzam · 2024-03-12T13:02:14Z

pkg/command/root.go

+		log.SetVerbosity(int32(verbosity))
+
+		// Set the TANZU_CLI_LOG_LEVEL env with the verbosity value
+		_ = os.Setenv(log.EnvTanzuCLILogLevel, strconv.Itoa(verbosity))


For plugins to consume transparently. Nice!

In the unit test you write, can you check that this variable is set as expected?

pkg/command/root.go

docs/plugindev/style_guide.md

pkg/command/root.go

marckhouzam · 2024-03-12T16:53:04Z

pkg/command/root.go

+			*/
+			for i := 0; i < len(args); i++ {
+				arg := args[i]
+				if arg == "--verbose" {


It is important to remove this flag and not pass it to plugins that don't support it, or else the plugin execution will fail because it does not know this flag:

$ tz --verbose 5 builder version [...] Error: unknown flag: --verbose Usage: tanzu builder version [flags] Flags: -h, --help help for version 2024-03-12T12:51:45-04:00 [x] : unknown flag: --verbose

Helm does this for its global flags when it calls plugins.
The code is a bit hard to read so we can discuss together if you want.
This is the code in question: https://github.com/helm/helm/blob/fa47752b892c05697ba54392ccc4fea0ba7397ce/cmd/helm/load_plugins.go#L154

Thanks @marckhouzam Added logic to verify the plugin flags and filter out verbose flag if plugin doesnt support

marckhouzam · 2024-03-12T16:59:00Z

pkg/command/root.go

+				// Try to convert the next argument to an integer
+				// If it's not an integer, CLI doesn't parse the verbose flag
+				if v, err := strconv.Atoi(nextArg); err == nil {
+					verbose = v


IIUC, we are honouring the --verbose flag AND we are passing it to the plugins that accept it for processing.

I'm hesitant to do this. Maybe it is ok. I'm not sure. The flag would technically be affecting two behaviours (the CLI and the plugin). We are just assuming the behaviours impacted are the same. Let's discuss.

Current Behaviour

Use Case 1 -> Plugin with --verbose int - Tanzu CLI only processes the --verbose flag of type int and then passes it to plugin if it supports --verbose flag.

Use Case 2 -> Plugin with --verbose bool/string - Tanzu CLI will not process the flag and passes the flag arg directly to plugin

and if plugin root command already implement a verbose flag pass the flag value down to plugin

marckhouzam · 2024-03-13T16:36:54Z

pkg/cli/plugin_cmd.go

@@ -148,6 +153,97 @@ func getCmdForPluginEx(p *PluginInfo, cmdGroupName string) *cobra.Command {
 	return cmd
 }

+// processArgs retrieves all plugin flags and filters out the verbose flag if the plugin doesn't support it.
+// It returns a slice of known arguments and an error if any occurred during the process.
+func processArgs(p *PluginInfo, args []string) (known []string, err error) {


Nit: the term known seems contradictory to me. What the CLI "knows" about is the --verbose flag. Whatever we pass to plugins, arguments or flags, the CLI does not know about: they could be anything.

Instead of known you can use pluginArgs

marckhouzam · 2024-03-13T16:40:08Z

pkg/cli/plugin_cmd.go

+					known = append(known, args[i])
+				}
+			} else {
+				// If the plugin does not support the verbose flag, skip it.


I suggest: // If the plugin does not support the verbose flag, also skip its value.

marckhouzam · 2024-03-13T16:41:31Z

pkg/cli/plugin_cmd.go

+	// Iterate over all the arguments.
+	for i := 0; i < len(args); i++ {
+		// If the current argument is the verbose flag...
+		if args[i] == "--verbose" {


We also have to consider the form --verbose=5, so here we should use HasPrefix and when skipping the flag value, we should only skip if it is not the --verbose= case.

marckhouzam · 2024-03-13T16:50:42Z

pkg/cli/plugin_cmd.go

+		return known, err
+	}
+
+	// Iterate over all the arguments.


If I may suggest a general approach to comments.

There is not as much value in saying the "what" in a comment; what we want is the "why".
So // Iterate over all the arguments. is "what" is being done. This can be easily understood from the next line of code so has little value.

What would be interesting to put in a comment is the "why" we are doing something.
So, in this case, why are we iterating over all the arguments?

marckhouzam · 2024-03-13T16:58:09Z

pkg/cli/plugin_cmd.go

+// It returns a map where the keys are the supported flags and the values are all set to true.
+func getPluginFlags(plugin *PluginInfo) (map[string]bool, error) {
+	// Create a new runner with the plugin's name, installation path, and the help command.
+	runner := NewRunner(plugin.Name, plugin.InstallationPath, []string{"-h"})


This only checks the flags on the root plugin command I believe.
What if there is a --verbose flag under a sub-command?
But I now realize this is probably not something we can do easily since we don't know the plugin sub-commands.
Let's discuss.

marckhouzam · 2024-03-13T17:00:59Z

pkg/command/root.go

+// determineVerbosityFromPluginCommandArgs processes the command line arguments for plugin commands.
+// It specifically looks for the '--verbose' flag and validates its value.
+// The function returns an error if the verbose flag is missing its value or the value is not an integer between 0 and 9.
+func determineVerbosityFromPluginCommandArgs(args []string) error {


This logic is similar to what we are doing in plugin_cmd.go.
I wonder if we could consolidate it in plugin_cmd.go and call Cobra's ParseFlags on the --verbose flag args like helm does.

That way Cobra would fill the verbose variable instead of us having to do it here.
And we'll be prepared for any other future global flag we may add later on.

marckhouzam · 2024-03-13T17:50:42Z

pkg/command/discovery_source_test.go

@@ -145,6 +145,8 @@ func Test_createAndListDiscoverySources(t *testing.T) {
 	testSource2 := "localhost:9876/tanzu-cli/plugins/sandbox1:small"
 	os.Setenv(constants.ConfigVariableAdditionalDiscoveryForTesting, testSource1+","+testSource2)

+	rootCmd, err = NewRootCmd()
+	assert.Nil(err)


What broke that we need this?

marckhouzam

we should not merge this until we figure out what to do for existing plugins that may already have such a flag

mpanchajanya self-assigned this Mar 7, 2024

vmwclabot added the cla-not-required label Mar 7, 2024

mpanchajanya force-pushed the add-global-verbose-flag branch 5 times, most recently from 8065426 to fa4fae6 Compare March 11, 2024 16:14

marckhouzam reviewed Mar 12, 2024

View reviewed changes

mpanchajanya changed the title ~~Add global --verbose / -v flag~~ Add global --verbose flag Mar 12, 2024

mpanchajanya force-pushed the add-global-verbose-flag branch 2 times, most recently from 43e08d9 to 889c768 Compare March 12, 2024 16:58

marckhouzam reviewed Mar 12, 2024

View reviewed changes

mpanchajanya force-pushed the add-global-verbose-flag branch 2 times, most recently from 280d857 to 4cc09fe Compare March 13, 2024 14:04

Implement global verbose flag to set logger verbosity

61a35e0

and if plugin root command already implement a verbose flag pass the flag value down to plugin

mpanchajanya force-pushed the add-global-verbose-flag branch from ab57246 to 61a35e0 Compare March 13, 2024 15:39

mpanchajanya marked this pull request as ready for review March 13, 2024 15:58

mpanchajanya requested a review from a team as a code owner March 13, 2024 15:58

marckhouzam reviewed Mar 13, 2024

View reviewed changes

marckhouzam requested changes Apr 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global --verbose flag #691

Add global --verbose flag #691

mpanchajanya commented Mar 7, 2024 •

edited

Loading

marckhouzam left a comment

marckhouzam Mar 12, 2024

marckhouzam Mar 12, 2024

marckhouzam Mar 12, 2024

marckhouzam Mar 12, 2024

mpanchajanya Mar 13, 2024

marckhouzam Mar 12, 2024

mpanchajanya Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam Mar 13, 2024

marckhouzam left a comment

Add global --verbose flag #691

Are you sure you want to change the base?

Add global --verbose flag #691

Conversation

mpanchajanya commented Mar 7, 2024 • 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

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

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

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

marckhouzam left a comment

Choose a reason for hiding this comment

mpanchajanya commented Mar 7, 2024 •

edited

Loading