From 7602a1960f90619541a00e6a5235d3eb8870d235 Mon Sep 17 00:00:00 2001 From: sh0rez Date: Fri, 9 Aug 2019 16:48:10 +0200 Subject: [PATCH] fix(cli): diff UX enhancements (#34) * feat(cli): use pager for long outputs The outputs of eval, show and diff can be pretty overwhelming. Tanka now uses the systems pager to aid this. When finding no pager or running in non-interactive mode, plain fmt.Println is used. * feat(cli): exit status on diff Exits with `16` in case of differences * feat(cli): diff before apply Shows diff before applying --- cmd/tk/jsonnet.go | 3 +- cmd/tk/main.go | 11 +++++++- cmd/tk/util.go | 47 ++++++++++++++++++++++++++++++++ cmd/tk/workflow.go | 53 ++++++++++++++++++++++++++---------- pkg/kubernetes/kubectl.go | 14 ++++++---- pkg/kubernetes/kubernetes.go | 6 ++-- pkg/kubernetes/subsetdiff.go | 15 ++++++---- 7 files changed, 117 insertions(+), 32 deletions(-) create mode 100644 cmd/tk/util.go diff --git a/cmd/tk/jsonnet.go b/cmd/tk/jsonnet.go index 52caaa250..4ab8fa4d7 100644 --- a/cmd/tk/jsonnet.go +++ b/cmd/tk/jsonnet.go @@ -2,7 +2,6 @@ package main import ( "encoding/json" - "fmt" "log" "path/filepath" @@ -27,7 +26,7 @@ func evalCmd() *cobra.Command { if err != nil { log.Fatalln("evaluating:", err) } - fmt.Print(json) + pageln(json) } return cmd diff --git a/cmd/tk/main.go b/cmd/tk/main.go index c5dc0eb1d..aae6756ab 100644 --- a/cmd/tk/main.go +++ b/cmd/tk/main.go @@ -3,11 +3,13 @@ package main import ( "flag" "log" + "os" "path/filepath" "github.com/posener/complete" "github.com/spf13/cobra" "github.com/spf13/viper" + "golang.org/x/crypto/ssh/terminal" "github.com/grafana/tanka/pkg/cmp" "github.com/grafana/tanka/pkg/config/v1alpha1" @@ -18,11 +20,18 @@ import ( // To be overwritten at build time var Version = "dev" +// primary handlers var ( config = &v1alpha1.Config{} kube *kubernetes.Kubernetes ) +// describing variables +var ( + verbose = false + interactive = terminal.IsTerminal(int(os.Stdout.Fd())) +) + // list of deprecated config keys and their alternatives // however, they still work and are aliased internally var deprecated = map[string]string{ @@ -53,7 +62,7 @@ func main() { }, } - rootCmd.PersistentFlags().BoolP("verbose", "v", false, "") + rootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "") // Subcommands cobra.EnableCommandSorting = false diff --git a/cmd/tk/util.go b/cmd/tk/util.go new file mode 100644 index 000000000..ad1d5a001 --- /dev/null +++ b/cmd/tk/util.go @@ -0,0 +1,47 @@ +package main + +import ( + "bytes" + "fmt" + "log" + "os" + "os/exec" + "strings" + + "github.com/alecthomas/chroma/quick" +) + +// pageln invokes the systems pager with the supplied data +// falls back to fmt.Println() when paging fails or non-interactive +func pageln(i ...interface{}) { + // no paging in non-interactive mode + if !interactive { + fmt.Println(i...) + return + } + + // get system pager, fallback to `more` + pager := os.Getenv("PAGER") + if pager == "" { + pager = "more" + } + + // invoke pager + cmd := exec.Command(pager) + cmd.Stdin = strings.NewReader(fmt.Sprintln(i...)) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + // if this fails, just print it + if err := cmd.Run(); err != nil { + fmt.Println(i...) + } +} + +func highlight(lang, s string) string { + var buf bytes.Buffer + if err := quick.Highlight(&buf, s, lang, "terminal", "vim"); err != nil { + log.Fatalln("Highlighting:", err) + } + return buf.String() +} diff --git a/cmd/tk/workflow.go b/cmd/tk/workflow.go index 179b995d6..8e46d8b5d 100644 --- a/cmd/tk/workflow.go +++ b/cmd/tk/workflow.go @@ -5,12 +5,12 @@ import ( "log" "os" - "github.com/alecthomas/chroma/quick" - "github.com/grafana/tanka/pkg/cmp" "github.com/posener/complete" "github.com/spf13/cobra" "github.com/spf13/pflag" - "golang.org/x/crypto/ssh/terminal" + + "github.com/grafana/tanka/pkg/cmp" + "github.com/grafana/tanka/pkg/kubernetes" ) type workflowFlagVars struct { @@ -44,6 +44,10 @@ func applyCmd() *cobra.Command { log.Fatalln("Reconciling:", err) } + if !diff(desired, false) { + log.Println("Warning: There are no differences. Your apply may not do anything at all.") + } + if err := kube.Apply(desired); err != nil { log.Fatalln("Applying:", err) } @@ -80,21 +84,41 @@ func diffCmd() *cobra.Command { log.Fatalln("Reconciling:", err) } - changes, err := kube.Diff(desired) - if err != nil { - log.Fatalln("Diffing:", err) + if diff(desired, interactive) { + os.Exit(16) } + log.Println("No differences.") + } - if terminal.IsTerminal(int(os.Stdout.Fd())) { - if err := quick.Highlight(os.Stdout, changes, "diff", "terminal", "vim"); err != nil { - log.Fatalln("Highlighting:", err) - } + cmd.Flags().String("diff-strategy", "", "force the diff-strategy to use. Automatically chosen if not set.") + return cmd +} + +// computes the diff, prints to screen. +// set `pager` to false to disable the pager. +// When non-interactive, no paging happens anyways. +func diff(state []kubernetes.Manifest, pager bool) (changed bool) { + changes, err := kube.Diff(state) + if err != nil { + log.Fatalln("Diffing:", err) + } + + if changes == nil { + return false + } + + if interactive { + h := highlight("diff", *changes) + if pager { + pageln(h) } else { - fmt.Println(changes) + fmt.Println(h) } + } else { + fmt.Println(*changes) } - cmd.Flags().String("diff-strategy", "", "force the diff-strategy to use. Automatically chosen if not set.") - return cmd + + return true } func showCmd() *cobra.Command { @@ -122,7 +146,8 @@ func showCmd() *cobra.Command { if err != nil { log.Fatalln("Pretty printing state:", err) } - fmt.Println(pretty) + + pageln(pretty) } return cmd } diff --git a/pkg/kubernetes/kubectl.go b/pkg/kubernetes/kubectl.go index ade0f9cf5..a02643e4f 100644 --- a/pkg/kubernetes/kubectl.go +++ b/pkg/kubernetes/kubectl.go @@ -172,9 +172,9 @@ Please type 'yes' to perform: `, // Diff takes a desired state as yaml and returns the differences // to the system in common diff format -func (k Kubectl) Diff(yaml string) (string, error) { +func (k Kubectl) Diff(yaml string) (*string, error) { if err := k.setupContext(); err != nil { - return "", err + return nil, err } argv := []string{"diff", @@ -187,7 +187,7 @@ func (k Kubectl) Diff(yaml string) (string, error) { stdin, err := cmd.StdinPipe() if err != nil { - return "", err + return nil, err } go func() { @@ -199,11 +199,13 @@ func (k Kubectl) Diff(yaml string) (string, error) { if exitError, ok := err.(*exec.ExitError); ok { // kubectl uses this to tell us that there is a diff if exitError.ExitCode() == 1 { - return raw.String(), nil + s := raw.String() + return &s, nil } } - return "", err + return nil, err } - return raw.String(), nil + // no diff -> nil + return nil, nil } diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 0fed6d1c0..0854a8e4b 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -24,7 +24,7 @@ type Kubernetes struct { differs map[string]Differ // List of diff strategies } -type Differ func(yaml string) (string, error) +type Differ func(yaml string) (*string, error) // New creates a new Kubernetes func New(s v1alpha1.Spec) *Kubernetes { @@ -114,10 +114,10 @@ func (k *Kubernetes) Apply(state []Manifest) error { } // Diff takes the desired state and returns the differences from the cluster -func (k *Kubernetes) Diff(state []Manifest) (string, error) { +func (k *Kubernetes) Diff(state []Manifest) (*string, error) { yaml, err := k.Fmt(state) if err != nil { - return "", err + return nil, err } if k.Spec.DiffStrategy == "" { diff --git a/pkg/kubernetes/subsetdiff.go b/pkg/kubernetes/subsetdiff.go index 57408000f..54a7b0ec6 100644 --- a/pkg/kubernetes/subsetdiff.go +++ b/pkg/kubernetes/subsetdiff.go @@ -17,7 +17,7 @@ type difference struct { live, merged string } -func (k Kubectl) SubsetDiff(y string) (string, error) { +func (k Kubectl) SubsetDiff(y string) (*string, error) { docs := []difference{} d := yaml.NewDecoder(strings.NewReader(y)) @@ -34,7 +34,7 @@ func (k Kubectl) SubsetDiff(y string) (string, error) { } if err != nil { - return "", errors.Wrap(err, "decoding yaml") + return nil, errors.Wrap(err, "decoding yaml") } routines++ @@ -55,19 +55,22 @@ func (k Kubectl) SubsetDiff(y string) (string, error) { close(errCh) if lastErr != nil { - return "", errors.Wrap(lastErr, "calculating subset") + return nil, errors.Wrap(lastErr, "calculating subset") } - diffs := "" + var diffs *string for _, d := range docs { diffStr, err := diff(d.name, d.live, d.merged) if err != nil { - return "", errors.Wrap(err, "invoking diff") + return nil, errors.Wrap(err, "invoking diff") } if diffStr != "" { diffStr += "\n" } - diffs += diffStr + if diffs == nil { + *diffs = "" + } + *diffs += diffStr } return diffs, nil