Skip to content

Commit 81d9545

Browse files
mumoshumumoshu
committed
Fix broken flag parsing
1 parent 0a9777c commit 81d9545

File tree

2 files changed

+173
-54
lines changed

2 files changed

+173
-54
lines changed

‎cmd/upgrade.go‎

Lines changed: 63 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -113,60 +113,6 @@ func newChartCommand() *cobra.Command {
113113
Short: "Show a diff explaining what a helm upgrade would change.",
114114
Long: globalUsage,
115115
DisableFlagParsing: true,
116-
PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
117-
const (
118-
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." +
119-
" --dry-run=server enables the cluster access with helm-get and the lookup template function."
120-
)
121-
122-
legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
123-
legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
124-
if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
125-
diff.dryRunModeSpecified = true
126-
args = legacyDryRunFlagSet.Args()
127-
} else {
128-
cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
129-
}
130-
131-
fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)
132-
133-
// Here we parse the flags ourselves so that we can support
134-
// both --dry-run and --dry-run=ARG syntaxes.
135-
//
136-
// If you don't do this, then cobra will treat --dry-run as
137-
// a "flag needs an argument: --dry-run" error.
138-
//
139-
// This works becase we have `DisableFlagParsing: true`` above.
140-
// Never turn that off, or you'll get the error again.
141-
if err := cmd.Flags().Parse(args); err != nil {
142-
return err
143-
}
144-
145-
args = cmd.Flags().Args()
146-
147-
if !diff.dryRunModeSpecified {
148-
dryRunModeSpecified := cmd.Flags().Changed("dry-run")
149-
diff.dryRunModeSpecified = dryRunModeSpecified
150-
151-
if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
152-
return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
153-
}
154-
}
155-
156-
cmd.SetArgs(args)
157-
158-
// We have to do this here because we have to sort out the
159-
// --dry-run flag ambiguity to determine the args to be checked.
160-
//
161-
// In other words, we can't just do:
162-
//
163-
// cmd.Args = func(cmd *cobra.Command, args []string) error {
164-
// return checkArgsLength(len(args), "release name", "chart path")
165-
// }
166-
//
167-
// Because it seems to take precedence over the PersistentPreRunE
168-
return checkArgsLength(len(args), "release name", "chart path")
169-
},
170116
Example: strings.Join([]string{
171117
" helm diff upgrade my-release stable/postgresql --values values.yaml",
172118
"",
@@ -206,6 +152,69 @@ func newChartCommand() *cobra.Command {
206152
"HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog",
207153
}, "\n"),
208154
RunE: func(cmd *cobra.Command, args []string) error {
155+
// Note that we can't just move this block to PersistentPreRunE,
156+
// because cmd.SetArgs(args) does not persist between PersistentPreRunE and RunE.
157+
// The choice is between:
158+
// 1. Doing this in RunE
159+
// 2. Doing this in PersistentPreRunE, saving args somewhere, and calling cmd.SetArgs(args) again in RunE
160+
// 2 is more complicated without much benefit, so we choose 1.
161+
{
162+
const (
163+
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." +
164+
" --dry-run=server enables the cluster access with helm-get and the lookup template function."
165+
)
166+
167+
legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError)
168+
legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage)
169+
if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun {
170+
diff.dryRunModeSpecified = true
171+
args = legacyDryRunFlagSet.Args()
172+
} else {
173+
cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
174+
}
175+
176+
fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args)
177+
178+
// Here we parse the flags ourselves so that we can support
179+
// both --dry-run and --dry-run=ARG syntaxes.
180+
//
181+
// If you don't do this, then cobra will treat --dry-run as
182+
// a "flag needs an argument: --dry-run" error.
183+
//
184+
// This works becase we have `DisableFlagParsing: true`` above.
185+
// Never turn that off, or you'll get the error again.
186+
if err := cmd.Flags().Parse(args); err != nil {
187+
return err
188+
}
189+
190+
args = cmd.Flags().Args()
191+
192+
if !diff.dryRunModeSpecified {
193+
dryRunModeSpecified := cmd.Flags().Changed("dry-run")
194+
diff.dryRunModeSpecified = dryRunModeSpecified
195+
196+
if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") {
197+
return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode)
198+
}
199+
}
200+
201+
cmd.SetArgs(args)
202+
203+
// We have to do this here because we have to sort out the
204+
// --dry-run flag ambiguity to determine the args to be checked.
205+
//
206+
// In other words, we can't just do:
207+
//
208+
// cmd.Args = func(cmd *cobra.Command, args []string) error {
209+
// return checkArgsLength(len(args), "release name", "chart path")
210+
// }
211+
//
212+
// Because it seems to take precedence over the PersistentPreRunE
213+
if err := checkArgsLength(len(args), "release name", "chart path"); err != nil {
214+
return err
215+
}
216+
}
217+
209218
// Suppress the command usage on error. See #77 for more info
210219
cmd.SilenceUsage = true
211220

‎main_test.go‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
package main
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"reflect"
7+
"testing"
8+
9+
"github.com/databus23/helm-diff/v3/cmd"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
func TestMain(m *testing.M) {
14+
if os.Getenv(env) == envValue {
15+
os.Exit(runFakeHelm())
16+
}
17+
18+
os.Exit(m.Run())
19+
}
20+
21+
func TestHelmDiff(t *testing.T) {
22+
os.Setenv(env, envValue)
23+
defer os.Unsetenv(env)
24+
25+
helmBin, helmBinSet := os.LookupEnv("HELM_BIN")
26+
os.Setenv("HELM_BIN", os.Args[0])
27+
defer func() {
28+
if helmBinSet {
29+
os.Setenv("HELM_BIN", helmBin)
30+
} else {
31+
os.Unsetenv("HELM_BIN")
32+
}
33+
}()
34+
35+
os.Args = []string{"helm-diff", "upgrade", "-f", "test/testdata/test-values.yaml", "test-release", "test/testdata/test-chart"}
36+
require.NoError(t, cmd.New().Execute())
37+
}
38+
39+
const (
40+
env = "BECOME_FAKE_HELM"
41+
envValue = "1"
42+
)
43+
44+
type fakeHelmSubcmd struct {
45+
cmd []string
46+
args []string
47+
stdout string
48+
stderr string
49+
exitCode int
50+
}
51+
52+
var helmSubcmdStubs = []fakeHelmSubcmd{
53+
{
54+
cmd: []string{"version"},
55+
stdout: `version.BuildInfo{Version:"v3.1.0-rc.1", GitCommit:"12345", GitTreeState:"clean", GoVersion:"go1.20.12"}`,
56+
},
57+
{
58+
cmd: []string{"get", "manifest"},
59+
args: []string{"test-release"},
60+
stdout: `---
61+
# Source: test-chart/templates/cm.yaml
62+
`,
63+
},
64+
{
65+
cmd: []string{"template"},
66+
args: []string{"test-release", "test/testdata/test-chart", "--values", "test/testdata/test-values.yaml", "--validate", "--is-upgrade"},
67+
},
68+
{
69+
cmd: []string{"get", "hooks"},
70+
args: []string{"test-release"},
71+
},
72+
}
73+
74+
func runFakeHelm() int {
75+
var stub *fakeHelmSubcmd
76+
77+
if len(os.Args) < 2 {
78+
fmt.Fprintln(os.Stderr, "fake helm does not support invocations without subcommands")
79+
return 1
80+
}
81+
82+
cmdAndArgs := os.Args[1:]
83+
for _, s := range helmSubcmdStubs {
84+
if reflect.DeepEqual(s.cmd, cmdAndArgs[:len(s.cmd)]) {
85+
stub = &s
86+
break
87+
}
88+
}
89+
90+
if stub == nil {
91+
fmt.Fprintf(os.Stderr, "no stub for %s\n", cmdAndArgs)
92+
return 1
93+
}
94+
95+
want := stub.args
96+
if want == nil {
97+
want = []string{}
98+
}
99+
got := cmdAndArgs[len(stub.cmd):]
100+
if !reflect.DeepEqual(want, got) {
101+
fmt.Fprintf(os.Stderr, "want: %v\n", want)
102+
fmt.Fprintf(os.Stderr, "got : %v\n", got)
103+
fmt.Fprintf(os.Stderr, "args : %v\n", os.Args)
104+
fmt.Fprintf(os.Stderr, "env : %v\n", os.Environ())
105+
return 1
106+
}
107+
fmt.Fprintf(os.Stdout, "%s", stub.stdout)
108+
fmt.Fprintf(os.Stderr, "%s", stub.stderr)
109+
return stub.exitCode
110+
}

0 commit comments

Comments
 (0)