Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/plugindev/style_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,9 @@ Available input components:

### Feedback principles

Useful for everyone, critical to the usability of screen-readers and automation
Support verbosity flags (`-v`, `--verbose`).
`--verbose` The global --verbose flag is meant to be automatically honoured by plugins when called through the tanzu CLI.

For this to work plugins are expected to use the `github.com/vmware-tanzu/tanzu-plugin-runtime/log` package.

Comment thread
mpanchajanya marked this conversation as resolved.
`--format` to define preferred output

Expand Down
98 changes: 97 additions & 1 deletion pkg/cli/plugin_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,12 @@ func getCmdForPluginEx(p *PluginInfo, cmdGroupName string) *cobra.Command {
Use: cmdGroupName,
Short: p.Description,
RunE: func(cmd *cobra.Command, args []string) error {
runner := NewRunner(p.Name, p.InstallationPath, args)
// Process the args to determine to pass the supported flags
pluginArgs, err := processArgs(p, args)
if err != nil {
return err
}
runner := NewRunner(p.Name, p.InstallationPath, pluginArgs)
ctx := context.Background()
setupPluginEnv()
return runner.Run(ctx)
Expand Down Expand Up @@ -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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Retrieve all plugin flags
pluginFlags, err := getPluginFlags(p)
if err != nil {
// If an error occurs while retrieving the plugin flags, return the error.
return known, err
}

// Iterate over all the arguments.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

for i := 0; i < len(args); i++ {
// If the current argument is the verbose flag...
if args[i] == "--verbose" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// ...check if the plugin supports the verbose flag.
if _, ok := pluginFlags[args[i]]; ok {
// If the plugin supports the verbose flag, add it to the known arguments.
known = append(known, args[i])
i++
// If there are more arguments, add the flag value to the known arguments.
if i < len(args) {
known = append(known, args[i])
}
} else {
// If the plugin does not support the verbose flag, skip it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

i++
}
} else {
// If the current argument is not the verbose flag, add it to the known arguments.
known = append(known, args[i])
}
}

// Return the known arguments and any error that occurred.
return known, err
}

// getPluginFlags parses the plugin's help command and returns all supported flags.
// 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"})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ctx := context.Background()

// Run the help command and capture the output.
stdout, _, err := runner.RunOutput(ctx)
if err != nil {
// If an error occurs while running the help command, return the error.
return nil, err
}

// Split the output into lines.
lines := strings.Split(stdout, "\n")

// Initialize a boolean to track when to start capturing flags.
start := false

// Initialize a map to store the flags.
flags := make(map[string]bool)

// Iterate over each line of the output.
for _, line := range lines {
// If the line starts with "Flags:", start capturing flags on the next line.
if strings.HasPrefix(line, "Flags:") {
start = true
continue
}

// If the line starts with "Use", stop capturing flags.
if start && strings.HasPrefix(line, "Use") {
break
}

// If capturing flags, split the line into parts.
if start {
parts := strings.Fields(line)
for i := 0; i < len(parts); i++ {
// Trim any trailing commas from each part.
part := strings.Trim(parts[i], ",")
// If the part is a flag (starts with "-") and is not just "-", add it to the flags map.
if strings.HasPrefix(part, "-") && part != "-" {
flags[part] = true
}
}
}
}

// Return the flags map and any error that occurred.
return flags, nil
}

// getHelpArguments extracts the command line to pass along to help calls.
// The help function is only ever called for help commands in the format of
// "tanzu help cmd", so we can assume anything two after "help" should get
Expand Down
2 changes: 2 additions & 0 deletions pkg/command/discovery_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What broke that we need this?

rootCmd.SetArgs([]string{"plugin", "source", "list"})
b = bytes.NewBufferString("")
rootCmd.SetOut(b)
Expand Down
104 changes: 95 additions & 9 deletions pkg/command/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ import (
"github.com/vmware-tanzu/tanzu-plugin-runtime/plugin"
)

// verbose flag to set the log level
var verbose int

// NewRootCmd creates a root command.
func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
var rootCmd = newRootCmd()
Expand All @@ -42,6 +45,9 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
// Configure defined environment variables found in the config file
cliconfig.ConfigureEnvVariables()

// Initialize the global verbose flag to set the log level verbosity (0 - 9)
rootCmd.PersistentFlags().IntVar(&verbose, "verbose", 0, "number for the log level verbosity (0 - 9)")

rootCmd.AddCommand(
newVersionCmd(),
newPluginCmd(),
Expand All @@ -58,6 +64,7 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
newCEIPParticipationCmd(),
newGenAllDocsCmd(),
)

if _, err := ensureCLIInstanceID(); err != nil {
return nil, errors.Wrap(err, "failed to ensure CLI ID")
}
Expand Down Expand Up @@ -240,8 +247,42 @@ func newRootCmd() *cobra.Command {
// silencing usage for now as we are getting double usage from plugins on errors
SilenceUsage: true,
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
// Sets the verbosity of the logger if TANZU_CLI_LOG_LEVEL is set
setLoggerVerbosity()
// Validate the verbose flag
if verbose < 0 || verbose > 9 {
return errors.New("Invalid value for verbose flag. It should be between 0 and 9")
}

// For CLI commands: Default the verbose value to -1 If the flag is not set by user
// For Plugin commands: This won't be executed since flags are not parsed by cli but send to plugin as args
if !cmd.Flags().Changed("verbose") {
verbose = -1
}

/**
Manually parse the flags when plugin command is triggered
1. For CLI commands; all flags have been parsed and removed from the list of args and this loop will not run
2. For Plugin commands; Since CLI doesn't parse args and sends all args to plugin this loop will run to verify if verbose flag is set by the user and then passes to plugin.
*/
if cmd.DisableFlagParsing {
err := determineVerbosityFromPluginCommandArgs(args)
if err != nil {
return err
}
}

// Sets the verbosity of the logger
Comment thread
mpanchajanya marked this conversation as resolved.
setLoggerVerbosity(verbose)
Comment thread
mpanchajanya marked this conversation as resolved.

//TODO: Remove test logs

log.Info("I am level default")
log.V(0).Info("I am level 0")
log.V(1).Info("I am level 1")
log.V(2).Info("I am level 2")
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")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄


// Ensure mutual exclusion in current contexts just in case if any plugins with old
// plugin-runtime sets k8s context as current when tanzu context is already set as current
Expand Down Expand Up @@ -290,14 +331,59 @@ func newRootCmd() *cobra.Command {
return rootCmd
}

// 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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Iterate over all command line arguments.
for i := 0; i < len(args); i++ {
// If the current argument is the verbose flag...
if args[i] == "--verbose" {
// ...check if there is a next argument that could be the value for the verbose flag.
if i+1 < len(args) {
nextArg := args[i+1]
// If the next argument is another flag (starts with '-' or '--'), return an error because the verbose flag is missing its value.
if strings.HasPrefix(nextArg, "-") || strings.HasPrefix(nextArg, "--") {
return errors.New("Missing value for verbose flag")
}
// Try to convert the next argument to an integer.
if v, err := strconv.Atoi(nextArg); err == nil {
// If the conversion is successful but the value is not between 0 and 9, return an error.
if v < 0 || v > 9 {
return errors.New("Invalid value for verbose flag. It should be between 0 and 9")
}
// If the value is valid, set the global verbose variable to this value.
verbose = v
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
// Skip the next argument because it has been processed as the value for the verbose flag.
i++
} else {
// If there is no next argument, return an error because the verbose flag is missing its value.
return errors.New("Missing value for verbose flag")
}
}
}
// If all arguments have been processed without errors, return nil.
return nil
}

// setLoggerVerbosity sets the verbosity of the logger if TANZU_CLI_LOG_LEVEL is set
func setLoggerVerbosity() {
// Configure the log level if env variable TANZU_CLI_LOG_LEVEL is set
logLevel := os.Getenv(log.EnvTanzuCLILogLevel)
if logLevel != "" {
logValue, err := strconv.ParseInt(logLevel, 10, 32)
if err == nil {
log.SetVerbosity(int32(logValue))
func setLoggerVerbosity(verbosity int) {
// If verbose global flag is passed set the log level verbosity if not then check if env variable TANZU_CLI_LOG_LEVEL is set
if verbosity >= 0 {
// Set the log level verbosity with the verbosity value
log.SetVerbosity(int32(verbosity))

// Set the TANZU_CLI_LOG_LEVEL env with the verbosity value
_ = os.Setenv(log.EnvTanzuCLILogLevel, strconv.Itoa(verbosity))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For plugins to consume transparently. Nice!

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

} else {
// Configure the log level if env variable TANZU_CLI_LOG_LEVEL is set
logLevel := os.Getenv(log.EnvTanzuCLILogLevel)
if logLevel != "" {
logValue, err := strconv.ParseInt(logLevel, 10, 32)
if err == nil {
log.SetVerbosity(int32(logValue))
}
}
}
}
Expand Down