CLI Enhancements #3897

calvn · 2018-02-02T16:09:29Z

Automatically disable colored UI if stdout no a tty
Add -no-color flag to all leaf subcommands ~~(caveat: Warnings printed by DeprecatedCommand is always colored, since flags gets parsed afterwards)~~
Add -format to all applicable subcommands
~~Ignore warnings and return only structured responses for non-table outputs (toggleable).~~ (This is not done since the current behavior for existing commands that supports formatted output do print warnings; e.g. token_renew.go).

Fixes #3869
Closes #3914

…auth list

calvn · 2018-02-07T16:29:58Z

/cc @sethvargo, it'd be great to get your feedback on this :)

This PR mainly aims to address two main points:

Ability to automatically detect and disable colored UI in a non-tty environment, with the option to disable globally as well.
Add -format option to all commands that returns any sort of non-error output from the client API. The bulk of the formatting logic/changes is in format.go, and commands simply use that helper function to print desired formatted output.

…stead

sethvargo · 2018-02-07T17:41:06Z

Hey @calvn

Thank you for the ping. This looks really good, but I do have a few comments.

It's worth noting that with "raw", we specifically do not use the CLI and just output to the underlying io.Writer in whatever CLI is given. Would it be possible to extend that further and just feed all the output functions through that?
The repeated if c.flagNoColor scares me a bit. It seems like we might be able to centralize that in the base command (untested) or at least a shared helper. Alternatively, we could create our own UI that just wraps ColoredUI and does the terminal.IsTerminal check on Write calls. (I think this would work really well and would always ensure that we don't include colors when piping). If you want some guidance on how this might look, feel free to ping me - happy to talk though it or sketch it out.
Having a CLI flag to disable color seems like more hassle than it's worth IMO. If we solve the "pipe" problem like mentioned above and give folks an envvar to disable globally, I think that covers most use cases. The challenge (as you've identified) with a CLI flag is that the CLI and UI are tightly coupled. If we just make it an envvar (not a CLI option), the code becomes a lot simpler and solves for that 1% use case that isn't covered by TTY and raw.

jefferai · 2018-02-07T19:50:48Z

command/main.go

@@ -23,6 +23,10 @@ func Run(args []string) int {
 			args = []string{"version"}
 			break
 		}
+
+		if arg == "-no-color" {
+			os.Setenv("VAULT_OUTPUT_NO_COLOR", "true")


I think VAULT_CLI_NO_COLOR would be better here

Going to remove this flag entirely. If we do the os.Getenv logic check on command.go's init() func as discussed, the envvar will be read before this gets applied.

Optionally, we could keep this if we change init() to a func that needs to be called explicitly (and call it after the args have been parsed) in main.go's Run().

calvn · 2018-02-07T20:43:29Z

@sethvargo thanks for taking a look! I think I've got a path forward with points 2 and 3, as we will probably end up removing the no-color flag in BaseCommand and use env-var at a higher level (probably in command.go's init() func) to toggle (which will determine what kind of UI to use).

I am still unclear on the first point though, and why we'd need to feed the output functions through PrintRaw.

sethvargo · 2018-02-07T21:48:44Z

Hey @calvn

Sorry - I'm not suggesting we need to use PrintRaw - I was just showcasing an example of how we can:

Detect if there's a TTY
Grab the underlying io.Writer out of a CLI object and write directly to it

calvn · 2018-02-07T22:01:57Z

Ahh, I see. I am using a pretty much the same logic to detect whether there's a TTY, but instead of grabbing the io.Writter, it's used to determine whether we need to wrap the UI around ColoredUI.

vault/command/commands.go

Lines 126 to 138 in 8f2a229

    
           if terminal.IsTerminal(int(os.Stdout.Fd())) { 
        
           	ui = &cli.ColoredUi{ 
        
           		ErrorColor: cli.UiColorRed, 
        
           		WarnColor:  cli.UiColorYellow, 
        
           		Ui:         ui, 
        
           	} 
        
           	serverCmdUi = &cli.ColoredUi{ 
        
           		ErrorColor: cli.UiColorRed, 
        
           		WarnColor:  cli.UiColorYellow, 
        
           		Ui:         serverCmdUi, 
        
           	} 
        
           }

sethvargo · 2018-02-07T22:21:14Z

Yea, so the problem is that, even if you don't use color, you'll still include a newline at the end of the line if piping to another process with the non-colored UI. The only way to avoid that is to use the underlying io.Writer object. I was envisioning just making our own like this:

type VaultUI struct {
  cli.ColoredUi
}

func (u *VaultUI) Output(m string) {
  terminal.IsTerminal(int(os.Stdout.Fd())) {
    c.ColoredUi.Output(m)
  } else {
    getWriter(c).Write(m)
  }
}

Then we just basically never think about it. We instantiate exactly 1 type of CLI, and that CLI decides, based on each output, whether there's a TTY present and "does the right thing". We could add the envvar logic in there too. If we want to be efficient, we could use an initializer and parse the envvar and tty check once and cache it on the struct.

calvn · 2018-02-07T22:44:07Z

Ah, I see what you mean, but I don't think that the additional newline would be an issue here as the intention for the no-color option was to avoid having ANSI color escape sequences specifically.

I think for most cases people still end up using awk, jq, or similar in their scripts to filter out values from the output before using any values. For instance, a script to get the unseal keys from vault operator init command would go through awk (or if -format=json is provided, jq), before the values are used, so having an extra newline in there wouldn't matter. In the cases where people might pipe directly, (e.g. operator generate-root), PrintRaw is already being used to output to the writer directly.

Jeff suggested that in the future we may add a -format=raw option that would output response as it would appear from curl, and in that case it would definitely make sense to use the underlying io.Writer. Maybe @jefferai could chime in on this.

jefferai · 2018-02-09T14:53:27Z

command/format.go

+	switch data.(type) {
+	case *api.Secret:
+		secret := data.(*api.Secret)
+		// Best-guess effort that is this a list, so parse out the keys


What's the reason for doing this? There could very well be "keys" coming back from various different backends, including k/v.

Right, so in that case you output things that are inside secret.Data[“keys”]

OutputWithFormat is basically the combined functionality of OutputSecret and OutputList

Right, so in that case you output things that are inside secret.Data[“keys”]

But....why? :-)

This is the original OutputList behavior: https://github.com/hashicorp/vault/pull/3897/files#diff-3e4377911a05a6f261016d1d5e1fec91L27

If you just passed in the secret, listing keys would be not formatted correctly:

Key Value --- ----- keys [foo]

Instead of:

$ vault list secret/ Keys ---- foo

…i-enhancements

#3950) * Pull formatting much further up to remove the need to use c.flagFormat Also remove OutputWithFormat as the logic can cause issues. * Use const for env var

…se days

…i-enhancements

sethvargo · 2018-02-11T15:23:10Z

command/base.go

-	flagField  string
+	flagFormat  string
+	flagField   string
+	flagNoColor bool


I think we can remove this and the var below if we are just using the envvar

The idea was to allow it in a flag but not advertise it. Now that you mention autocorrect below, I wonder if it's better to just remove it rather than have people get it autocorrected and then complain that it's not documented. What do you think?

I think the code will be a lot simpler if we drop the flag entirely. People are more likely to "never want colored output" that "not want colored output for this specific command", and even in the latter case, they can specify the envvar first.

sethvargo · 2018-02-11T15:24:55Z

command/format.go

@@ -58,6 +67,14 @@ var Formatters = map[string]Formatter{
 	"yml":   YamlFormatter{},
 }

+func Format() string {
+	format := os.Getenv(EnvVaultFormat)


Could we retrieve this as part of an init function instead of doing the envvar lookup each time?

We just got away from init functions in favor of being able to intercept the args earlier in the main run function. This isn't going to be meaningfully slow for interactive uses, is there a reason not to just call this a couple of times?

I've experienced Getenv being slow (in relative terms), particularly on Windows systems. I wonder if we could use a Once instead and cache the result?

sethvargo · 2018-02-11T15:26:31Z

command/main.go

@@ -23,8 +47,85 @@ func Run(args []string) int {
 			args = []string{"version"}
 			break
 		}
+
+		if arg == "-no-color" {


UX-ey thing. We should be nice and add || arg == "-nocolor". Even if we never advertise that, people will forget the dash and "auto-correcting them" will be a nice UX.

jefferai · 2018-02-11T22:08:10Z

@sethvargo Your comments should be addressed in 9b62480

calvn · 2018-02-12T22:47:51Z

command/main.go

@@ -23,7 +48,83 @@ func Run(args []string) int {
 			args = []string{"version"}
 			break
 		}
+
+		// Parse a given flag here, which overrides the env var
+		if strings.HasPrefix(arg, "-format=") {


Haven't tested, but does this handle --format as well? (L53-L59)

Didn't even know we supported that. Will address.

calvn · 2018-02-12T22:50:20Z

command/main.go

+}
+
+// setupEnv parses args and may replace them and sets some env vars to known
+// values based on color/format options


Doesn't handle color (anymore..?)

calvn · 2018-02-12T22:53:37Z

command/main.go

+	envVaultFormat := os.Getenv(EnvVaultFormat)
+	// If we did not parse a value, fetch the env var
+	if format == "" && envVaultFormat != "" {
+		format = envVaultFormat


We can probably do format = strings.ToLower(envVaultFormat) directly.

We'd still need it outside the if block so it doesn't buy us anything.

calvn · 2018-02-12T22:54:43Z

command/main.go

+}
+
+func Run(args []string) int {
+	args = setupEnv(args)


Probably should rename setupEnv to setupEnvFormat.

calvn added 9 commits February 1, 2018 12:47

Use Colored UI if stdout is a tty

c14210e

Add format options to operator unseal

58be122

Add format test on operator unseal

a1fcb87

Add -no-color output flag, and use BasicUi if no-color flag is provided

e149184

Move seal status formatting logic to OutputSealStatus

45627d6

Apply no-color to warnings from DeprecatedCommands as well

ae12135

Add OutputWithFormat to support arbitrary data, add format option to …

fa462a4

…auth list

Add ability to output arbitrary list data on TableFormatter

14e70dd

Clear up switch logic on format

caae214

vishalnayak added this to the 0.9.4 milestone Feb 6, 2018

calvn added 2 commits February 6, 2018 14:13

Add format option for list-related commands

e0f96d6

Add format option to rest of commands that returns a client API response

c8cfcb5

calvn changed the title ~~[WIP] CLI Enhancements~~ CLI Enhancements Feb 7, 2018

calvn requested a review from jefferai February 7, 2018 16:21

calvn added 2 commits February 7, 2018 11:51

Remove initOutputYAML and initOutputJSON, and use OutputWithFormat in…

d2ba2fe

…stead

Remove outputAsYAML and outputAsJSON, and use OutputWithFormat instead

8f2a229

jefferai reviewed Feb 7, 2018

View reviewed changes

calvn added 3 commits February 7, 2018 17:09

Remove -no-color flag, use env var exclusively to toggle colored output

204180b

Fix compile

2cd9e8e

Remove -no-color flag in main.go

c9573ea

calvn added 3 commits February 7, 2018 18:05

Add missing FlagSetOutputFormat

7ecec64

Fix generate-root/decode test

189f8c9

Merge branch 'master-oss' into cli-enhancements

60184e9

jefferai added 2 commits February 8, 2018 16:37

Migrate init functions to main.go

a7225da

Add no-color flag back as hidden

2b3f18d

jefferai reviewed Feb 9, 2018

View reviewed changes

calvn and others added 10 commits February 9, 2018 11:10

Handle non-supported data types for TableFormatter.OutputList

63d4957

Merge branch 'cli-enhancements' of github.com:hashicorp/vault into cl…

53d4534

…i-enhancements

Pull formatting much further up to remove the need to use c.flagFormat (

4ff62dd

#3950) * Pull formatting much further up to remove the need to use c.flagFormat Also remove OutputWithFormat as the logic can cause issues. * Use const for env var

Minor updates

3ff53bc

Remove unnecessary check

a81c42e

Fix SSH output and some tests

79992f3

Fix tests

07cf36e

Make race detector not run on generate root since it kills Travis the…

7982def

…se days

Update docs

9ca29d3

Merge branch 'cli-enhancements' of github.com:hashicorp/vault into cl…

df07a3d

…i-enhancements

calvn mentioned this pull request Feb 9, 2018

Fix Typo in Documentation #3914

Closed

Update docs

1583e23

sethvargo reviewed Feb 11, 2018

View reviewed changes

jefferai added 2 commits February 11, 2018 16:42

Merge branch 'master-oss' into cli-enhancements

51a07e2

Address review feedback

9b62480

calvn commented Feb 12, 2018

View reviewed changes

jefferai and others added 2 commits February 12, 2018 18:04

Handle --format as well as -format

d0e35e1

Merge branch 'master' into cli-enhancements

e4ae7ba

calvn merged commit 3189278 into master Feb 12, 2018

calvn deleted the cli-enhancements branch February 12, 2018 23:12

ixe013 mentioned this pull request Feb 14, 2018

VAULT_CLI_NO_COLOR produces ANSI sequences #3973

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI Enhancements #3897

CLI Enhancements #3897

calvn commented Feb 2, 2018 •

edited

Loading

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

jefferai Feb 7, 2018

calvn Feb 7, 2018

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

calvn commented Feb 7, 2018

jefferai Feb 9, 2018

calvn Feb 9, 2018

calvn Feb 9, 2018

jefferai Feb 9, 2018

calvn Feb 9, 2018

sethvargo Feb 11, 2018

jefferai Feb 11, 2018

sethvargo Feb 11, 2018

sethvargo Feb 11, 2018

jefferai Feb 11, 2018

sethvargo Feb 11, 2018

sethvargo Feb 11, 2018

jefferai commented Feb 11, 2018

calvn Feb 12, 2018 •

edited

Loading

jefferai Feb 12, 2018

calvn Feb 12, 2018

calvn Feb 12, 2018

jefferai Feb 12, 2018

calvn Feb 12, 2018 •

edited

Loading

CLI Enhancements #3897

CLI Enhancements #3897

Conversation

calvn commented Feb 2, 2018 • edited Loading

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

calvn commented Feb 7, 2018

sethvargo commented Feb 7, 2018

calvn commented Feb 7, 2018

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

jefferai commented Feb 11, 2018

calvn Feb 12, 2018 • edited Loading

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

calvn Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

calvn commented Feb 2, 2018 •

edited

Loading

calvn Feb 12, 2018 •

edited

Loading

calvn Feb 12, 2018 •

edited

Loading