Skip to content

Commit 94c0f4e

Browse files
mumoshumumoshu
committed
Fix broken flag parsing
1 parent 0a9777c commit 94c0f4e

File tree

2 files changed

+175
-54
lines changed

2 files changed

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

0 commit comments

Comments
 (0)