Skip to content

Commit

Permalink
Fix broken flag parsing
Browse files Browse the repository at this point in the history
  • Loading branch information
mumoshu committed Jan 4, 2024
1 parent 0a9777c commit 94c0f4e
Show file tree
Hide file tree
Showing 2 changed files with 175 additions and 54 deletions.
117 changes: 63 additions & 54 deletions cmd/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,60 +113,6 @@ func newChartCommand() *cobra.Command {
Short: "Show a diff explaining what a helm upgrade would change.",
Long: globalUsage,
DisableFlagParsing: true,
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
const (
dryRunUsage = "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation." +
" --dry-run=server enables the cluster access with helm-get and the lookup template function."
)

legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
diff.dryRunModeSpecified = true
args = legacyDryRunFlagSet.Args()
} else {
cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
}

fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)

// Here we parse the flags ourselves so that we can support
// both --dry-run and --dry-run=ARG syntaxes.
//
// If you don't do this, then cobra will treat --dry-run as
// a "flag needs an argument: --dry-run" error.
//
// This works becase we have `DisableFlagParsing: true`` above.
// Never turn that off, or you'll get the error again.
if err := cmd.Flags().Parse(args); err != nil {
return err
}

args = cmd.Flags().Args()

if !diff.dryRunModeSpecified {
dryRunModeSpecified := cmd.Flags().Changed("dry-run")
diff.dryRunModeSpecified = dryRunModeSpecified

if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
}
}

cmd.SetArgs(args)

// We have to do this here because we have to sort out the
// --dry-run flag ambiguity to determine the args to be checked.
//
// In other words, we can't just do:
//
// cmd.Args = func(cmd *cobra.Command, args []string) error {
// return checkArgsLength(len(args), "release name", "chart path")
// }
//
// Because it seems to take precedence over the PersistentPreRunE
return checkArgsLength(len(args), "release name", "chart path")
},
Example: strings.Join([]string{
" helm diff upgrade my-release stable/postgresql --values values.yaml",
"",
Expand Down Expand Up @@ -206,6 +152,69 @@ func newChartCommand() *cobra.Command {
"HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog",
}, "\n"),
RunE: func(cmd *cobra.Command, args []string) error {
// Note that we can't just move this block to PersistentPreRunE,
// because cmd.SetArgs(args) does not persist between PersistentPreRunE and RunE.
// The choice is between:
// 1. Doing this in RunE
// 2. Doing this in PersistentPreRunE, saving args somewhere, and calling cmd.SetArgs(args) again in RunE
// 2 is more complicated without much benefit, so we choose 1.
{
const (
dryRunUsage = "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation." +
" --dry-run=server enables the cluster access with helm-get and the lookup template function."
)

legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
diff.dryRunModeSpecified = true
args = legacyDryRunFlagSet.Args()
} else {
cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
}

fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)

// Here we parse the flags ourselves so that we can support
// both --dry-run and --dry-run=ARG syntaxes.
//
// If you don't do this, then cobra will treat --dry-run as
// a "flag needs an argument: --dry-run" error.
//
// This works becase we have `DisableFlagParsing: true`` above.
// Never turn that off, or you'll get the error again.
if err := cmd.Flags().Parse(args); err != nil {
return err
}

args = cmd.Flags().Args()

if !diff.dryRunModeSpecified {
dryRunModeSpecified := cmd.Flags().Changed("dry-run")
diff.dryRunModeSpecified = dryRunModeSpecified

if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
}
}

cmd.SetArgs(args)

// We have to do this here because we have to sort out the
// --dry-run flag ambiguity to determine the args to be checked.
//
// In other words, we can't just do:
//
// cmd.Args = func(cmd *cobra.Command, args []string) error {
// return checkArgsLength(len(args), "release name", "chart path")
// }
//
// Because it seems to take precedence over the PersistentPreRunE
if err := checkArgsLength(len(args), "release name", "chart path"); err != nil {
return err
}
}

// Suppress the command usage on error. See #77 for more info
cmd.SilenceUsage = true

Expand Down
112 changes: 112 additions & 0 deletions main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package main

import (
"fmt"
"os"
"reflect"
"testing"

"github.com/stretchr/testify/require"

"github.com/databus23/helm-diff/v3/cmd"
)

func TestMain(m *testing.M) {
if os.Getenv(env) == envValue {
os.Exit(runFakeHelm())
}

os.Exit(m.Run())
}

func TestHelmDiff(t *testing.T) {
os.Setenv(env, envValue)
defer os.Unsetenv(env)

helmBin, helmBinSet := os.LookupEnv("HELM_BIN")
os.Setenv("HELM_BIN", os.Args[0])
defer func() {
if helmBinSet {
os.Setenv("HELM_BIN", helmBin)
} else {
os.Unsetenv("HELM_BIN")
}
}()

os.Args = []string{"helm-diff", "upgrade", "-f", "test/testdata/test-values.yaml", "test-release", "test/testdata/test-chart"}
require.NoError(t, cmd.New().Execute())
}

const (
env = "BECOME_FAKE_HELM"
envValue = "1"
)

type fakeHelmSubcmd struct {
cmd []string
args []string
stdout string
stderr string
exitCode int
}

var helmSubcmdStubs = []fakeHelmSubcmd{
{
cmd: []string{"version"},
stdout: `version.BuildInfo{Version:"v3.1.0-rc.1", GitCommit:"12345", GitTreeState:"clean", GoVersion:"go1.20.12"}`,
},
{
cmd: []string{"get", "manifest"},
args: []string{"test-release"},
stdout: `---
# Source: test-chart/templates/cm.yaml
`,
},
{
cmd: []string{"template"},
args: []string{"test-release", "test/testdata/test-chart", "--values", "test/testdata/test-values.yaml", "--validate", "--is-upgrade"},
},
{
cmd: []string{"get", "hooks"},
args: []string{"test-release"},
},
}

func runFakeHelm() int {
var stub *fakeHelmSubcmd

if len(os.Args) < 2 {
fmt.Fprintln(os.Stderr, "fake helm does not support invocations without subcommands")
return 1
}

cmdAndArgs := os.Args[1:]
for i := range helmSubcmdStubs {
s := helmSubcmdStubs[i]
if reflect.DeepEqual(s.cmd, cmdAndArgs[:len(s.cmd)]) {
stub = &s
break
}
}

if stub == nil {
fmt.Fprintf(os.Stderr, "no stub for %s\n", cmdAndArgs)
return 1
}

want := stub.args
if want == nil {
want = []string{}
}
got := cmdAndArgs[len(stub.cmd):]
if !reflect.DeepEqual(want, got) {
fmt.Fprintf(os.Stderr, "want: %v\n", want)
fmt.Fprintf(os.Stderr, "got : %v\n", got)
fmt.Fprintf(os.Stderr, "args : %v\n", os.Args)
fmt.Fprintf(os.Stderr, "env : %v\n", os.Environ())
return 1
}
fmt.Fprintf(os.Stdout, "%s", stub.stdout)
fmt.Fprintf(os.Stderr, "%s", stub.stderr)
return stub.exitCode
}

0 comments on commit 94c0f4e

Please sign in to comment.