Skip to content

Commit

Permalink
Remake dry-run=server optional (#547)
Browse files Browse the repository at this point in the history 
* Remake dry-run=server optional

This is a follow-up on #499 which redoes it without relying on DisableParsing. DisableParsing turned out to break far many things than I imagined!

* Remove redundant test case

* optimze little code

Signed-off-by: yxxhero <[email protected]>

---------

Signed-off-by: yxxhero <[email protected]>
Co-authored-by: yxxhero <[email protected]>
  • Loading branch information
@mumoshu @yxxhero
mumoshu and yxxhero authored Jan 27, 2024
1 parent 83a48fe commit 6623fd7
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 89 deletions.
111 changes: 34 additions & 77 deletions cmd/upgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import (
"fmt"
"log"
"os"
"slices"
"strconv"
"strings"

jsonpatch "github.com/evanphx/json-patch"
jsoniterator "github.com/json-iterator/go"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/cli"
"helm.sh/helm/v3/pkg/kube"
Expand All @@ -29,6 +29,14 @@ import (
"github.com/databus23/helm-diff/v3/manifest"
)

var (
validDryRunValues = []string{"server", "client", "true", "false"}
)

const (
dryRunNoOptDefVal = "client"
)

type diffCmd struct {
release string
chart string
Expand All @@ -38,8 +46,6 @@ type diffCmd struct {
devel bool
disableValidation bool
disableOpenAPIValidation bool
dryRunMode string
dryRunModeSpecified bool
enableDNS bool
namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace.
valueFiles valueFiles
Expand All @@ -62,6 +68,14 @@ type diffCmd struct {
kubeVersion string
useUpgradeDryRun bool
diff.Options

// dryRunMode can take the following values:
// - "none": no dry run is performed
// - "client": dry run is performed without remote cluster access
// - "server": dry run is performed with remote cluster access
// - "true": same as "client"
// - "false": same as "none"
dryRunMode string
}

func (d *diffCmd) isAllowUnreleased() bool {
Expand All @@ -73,10 +87,9 @@ func (d *diffCmd) isAllowUnreleased() bool {

// clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree.
//
// helm-diff basically have 3 modes of operation:
// 1. no cluster access at all when --dry-run, --dry-run=client, or --dry-run=true is specified.
// 2. basic cluster access when --dry-run is not specified.
// 3. extra cluster access when --dry-run=server is specified.
// helm-diff basically have 2 modes of operation:
// 1. without cluster access at all when --dry-run=true or --dry-run=client is specified.
// 2. with cluster access when --dry-run is unspecified, false, or server.
//
// clusterAccessAllowed returns true when the mode is either 2 or 3.
//
Expand All @@ -89,8 +102,7 @@ func (d *diffCmd) isAllowUnreleased() bool {
//
// See also https://github.com/helm/helm/pull/9426#discussion_r1181397259
func (d *diffCmd) clusterAccessAllowed() bool {
clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client"
return !clientOnly
return d.dryRunMode == "none" || d.dryRunMode == "false" || d.dryRunMode == "server"
}

const globalUsage = `Show a diff explaining what a helm upgrade would change.
Expand All @@ -111,10 +123,9 @@ func newChartCommand() *cobra.Command {
unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == "true"

cmd := &cobra.Command{
Use: "upgrade [flags] [RELEASE] [CHART]",
Short: "Show a diff explaining what a helm upgrade would change.",
Long: globalUsage,
DisableFlagParsing: true,
Use: "upgrade [flags] [RELEASE] [CHART]",
Short: "Show a diff explaining what a helm upgrade would change.",
Long: globalUsage,
Example: strings.Join([]string{
" helm diff upgrade my-release stable/postgresql --values values.yaml",
"",
Expand Down Expand Up @@ -153,71 +164,14 @@ func newChartCommand() *cobra.Command {
"# Read the flag usage below for more information on --context.",
"HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog",
}, "\n"),
Args: func(cmd *cobra.Command, args []string) error {
return checkArgsLength(len(args), "release name", "chart path")
},
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."
)

cmdFlags := cmd.Flags()

// see: https://github.com/databus23/helm-diff/issues/537
cmdFlags.ParseErrorsWhitelist.UnknownFlags = unknownFlags

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 {
cmdFlags.StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage)
}

// 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 := cmdFlags.Parse(args); err != nil {
return err
}

args = cmdFlags.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
}
if diff.dryRunMode == "" {
diff.dryRunMode = "none"
} else if !slices.Contains(validDryRunValues, diff.dryRunMode) {
return fmt.Errorf("flag %q must take a bool value or either %q or %q, but got %q", "dry-run", "client", "server", diff.dryRunMode)
}

// Suppress the command usage on error. See #77 for more info
Expand Down Expand Up @@ -293,6 +247,9 @@ func newChartCommand() *cobra.Command {
f.BoolVar(&diff.devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored.")
f.BoolVar(&diff.disableValidation, "disable-validation", false, "disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install")
f.BoolVar(&diff.disableOpenAPIValidation, "disable-openapi-validation", false, "disables rendered templates validation against the Kubernetes OpenAPI Schema")
f.StringVar(&diff.dryRunMode, "dry-run", "", "--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.")
f.Lookup("dry-run").NoOptDefVal = dryRunNoOptDefVal
f.BoolVar(&diff.enableDNS, "enable-dns", false, "enable DNS lookups when rendering templates")
f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path")
f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)")
Expand Down
27 changes: 16 additions & 11 deletions cmd/upgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,44 @@ func TestIsRemoteAccessAllowed(t *testing.T) {
expected bool
}{
{
name: "no flags",
cmd: diffCmd{},
name: "no flags",
cmd: diffCmd{
dryRunMode: "none",
},
expected: true,
},
{
name: "legacy explicit dry-run flag",
name: "legacy explicit dry-run=true flag",
cmd: diffCmd{
dryRunModeSpecified: true,
dryRunMode: "true",
dryRunMode: "true",
},
expected: false,
},
{
name: "legacy explicit dry-run=false flag",
cmd: diffCmd{
dryRunMode: "false",
},
expected: true,
},
{
name: "legacy empty dry-run flag",
cmd: diffCmd{
dryRunModeSpecified: true,
dryRunMode: "",
dryRunMode: dryRunNoOptDefVal,
},
expected: false,
},
{
name: "server-side dry-run flag",
cmd: diffCmd{
dryRunModeSpecified: true,
dryRunMode: "server",
dryRunMode: "server",
},
expected: true,
},
{
name: "client-side dry-run flag",
cmd: diffCmd{
dryRunModeSpecified: true,
dryRunMode: "client",
dryRunMode: "client",
},
expected: false,
},
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/evanphx/json-patch v5.8.1+incompatible
github.com/json-iterator/go v1.1.12
github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d
github.com/pkg/errors v0.9.1
github.com/pkg/errors v0.9.1 // indirect
github.com/spf13/cobra v1.8.0
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.8.4
Expand Down

0 comments on commit 6623fd7

Please sign in to comment.