From 4aa54423b6f44eed1c3bba40289651d28a9a2c1e Mon Sep 17 00:00:00 2001 From: Hubertbits <170125456+hubertbits@users.noreply.github.com> Date: Fri, 11 Apr 2025 23:07:35 +0200 Subject: [PATCH] Adds include-crds and refactor using flagRegistrar and generic flag system Signed-off-by: yxxhero --- cmd/apply.go | 11 +- cmd/diff.go | 8 ++ cmd/sync.go | 9 +- cmd/template.go | 7 +- pkg/app/app.go | 17 +-- pkg/app/config.go | 20 ++- pkg/common/bool_flag.go | 43 ++++++ pkg/common/bool_flag_test.go | 94 +++++++++++++ pkg/common/string_array_flag.go | 58 ++++++++ pkg/common/string_array_flag_test.go | 195 +++++++++++++++++++++++++++ pkg/common/string_flag.go | 35 +++++ pkg/common/string_flag_test.go | 100 ++++++++++++++ pkg/config/apply.go | 32 ++++- pkg/config/diff.go | 30 ++++- pkg/config/options.go | 65 +++++++++ pkg/config/sync.go | 32 ++++- pkg/config/template.go | 30 ++++- pkg/flags/apply.go | 24 ++++ pkg/flags/common.go | 49 +++++++ pkg/flags/common_test.go | 140 +++++++++++++++++++ pkg/flags/diff.go | 22 +++ pkg/flags/initializer.go | 32 +++++ pkg/flags/sync.go | 24 ++++ pkg/flags/template.go | 22 +++ pkg/state/state.go | 64 +++++---- 25 files changed, 1098 insertions(+), 65 deletions(-) create mode 100644 pkg/common/bool_flag.go create mode 100644 pkg/common/bool_flag_test.go create mode 100644 pkg/common/string_array_flag.go create mode 100644 pkg/common/string_array_flag_test.go create mode 100644 pkg/common/string_flag.go create mode 100644 pkg/common/string_flag_test.go create mode 100644 pkg/config/options.go create mode 100644 pkg/flags/apply.go create mode 100644 pkg/flags/common.go create mode 100644 pkg/flags/common_test.go create mode 100644 pkg/flags/diff.go create mode 100644 pkg/flags/initializer.go create mode 100644 pkg/flags/sync.go create mode 100644 pkg/flags/template.go diff --git a/cmd/apply.go b/cmd/apply.go index 6b5a23f4..437877c3 100644 --- a/cmd/apply.go +++ b/cmd/apply.go @@ -5,15 +5,20 @@ import ( "github.com/helmfile/helmfile/pkg/app" "github.com/helmfile/helmfile/pkg/config" + "github.com/helmfile/helmfile/pkg/flags" ) // NewApplyCmd returns apply subcmd func NewApplyCmd(globalCfg *config.GlobalImpl) *cobra.Command { - applyOptions := &config.ApplyOptions{} + applyOptions := config.NewApplyOptions() + flagRegistrar := flags.NewApplyFlagRegistrar() cmd := &cobra.Command{ Use: "apply", Short: "Apply all resources from state file only when there are changes", + PreRun: func(cmd *cobra.Command, args []string) { + flagRegistrar.TransferFlags(cmd, applyOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { applyImpl := config.NewApplyImpl(globalCfg, applyOptions) @@ -44,7 +49,6 @@ func NewApplyCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringVar(&applyOptions.SyncArgs, "sync-args", "", `pass args to helm upgrade`) f.StringVar(&globalCfg.GlobalOptions.Args, "args", "", "pass args to helm exec") f.BoolVar(&applyOptions.SkipCleanup, "skip-cleanup", false, "Stop cleaning up temporary values generated by helmfile and helm-secrets. Useful for debugging. Don't use in production for security") - f.BoolVar(&applyOptions.SkipCRDs, "skip-crds", false, "if set, no CRDs will be installed on sync. By default, CRDs are installed if not already present") f.BoolVar(&applyOptions.SkipNeeds, "skip-needs", true, `do not automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided. Defaults to true when --include-needs or --include-transitive-needs is not provided`) f.BoolVar(&applyOptions.IncludeNeeds, "include-needs", false, `automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided`) f.BoolVar(&applyOptions.IncludeTransitiveNeeds, "include-transitive-needs", false, `like --include-needs, but also includes transitive needs (needs of needs). Does nothing when --selector/-l flag is not provided. Overrides exclusions of other selectors and conditions.`) @@ -68,5 +72,8 @@ func NewApplyCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringVar(&applyOptions.Cascade, "cascade", "", "pass cascade to helm exec, default: background") f.StringArrayVar(&applyOptions.SuppressOutputLineRegex, "suppress-output-line-regex", nil, "a list of regex patterns to suppress output lines from the diff output") + // Register flags using the registrar + flagRegistrar.RegisterFlags(cmd) + return cmd } diff --git a/cmd/diff.go b/cmd/diff.go index 020bc6f1..96a516f5 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -5,15 +5,20 @@ import ( "github.com/helmfile/helmfile/pkg/app" "github.com/helmfile/helmfile/pkg/config" + "github.com/helmfile/helmfile/pkg/flags" ) // NewDiffCmd returns diff subcmd func NewDiffCmd(globalCfg *config.GlobalImpl) *cobra.Command { diffOptions := config.NewDiffOptions() + flagRegistrar := flags.NewDiffFlagRegistrar() cmd := &cobra.Command{ Use: "diff", Short: "Diff releases defined in state file", + PreRun: func(cmd *cobra.Command, args []string) { + flagRegistrar.TransferFlags(cmd, diffOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { diffImpl := config.NewDiffImpl(globalCfg, diffOptions) err := config.NewCLIConfigImpl(diffImpl.GlobalImpl) @@ -56,5 +61,8 @@ func NewDiffCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringArrayVar(&diffOptions.PostRendererArgs, "post-renderer-args", nil, `pass --post-renderer-args to "helm template" or "helm upgrade --install"`) f.StringArrayVar(&diffOptions.SuppressOutputLineRegex, "suppress-output-line-regex", nil, "a list of regex patterns to suppress output lines from the diff output") + // Register flags using the registrar + flagRegistrar.RegisterFlags(cmd) + return cmd } diff --git a/cmd/sync.go b/cmd/sync.go index 1d8ed45f..f65dfeda 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -5,15 +5,20 @@ import ( "github.com/helmfile/helmfile/pkg/app" "github.com/helmfile/helmfile/pkg/config" + "github.com/helmfile/helmfile/pkg/flags" ) // NewSyncCmd returns sync subcmd func NewSyncCmd(globalCfg *config.GlobalImpl) *cobra.Command { syncOptions := config.NewSyncOptions() + flagRegistrar := flags.NewSyncFlagRegistrar() cmd := &cobra.Command{ Use: "sync", Short: "Sync releases defined in state file", + PreRun: func(cmd *cobra.Command, args []string) { + flagRegistrar.TransferFlags(cmd, syncOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { syncImpl := config.NewSyncImpl(globalCfg, syncOptions) err := config.NewCLIConfigImpl(syncImpl.GlobalImpl) @@ -38,7 +43,6 @@ func NewSyncCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.IntVar(&syncOptions.Concurrency, "concurrency", 0, "maximum number of concurrent helm processes to run, 0 is unlimited") f.BoolVar(&syncOptions.Validate, "validate", false, "validate your manifests against the Kubernetes cluster you are currently pointing at. Note that this requires access to a Kubernetes cluster to obtain information necessary for validating, like the sync of available API versions") f.BoolVar(&syncOptions.SkipNeeds, "skip-needs", true, `do not automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided. Defaults to true when --include-needs or --include-transitive-needs is not provided`) - f.BoolVar(&syncOptions.SkipCRDs, "skip-crds", false, "if set, no CRDs will be installed on sync. By default, CRDs are installed if not already present") f.BoolVar(&syncOptions.IncludeNeeds, "include-needs", false, `automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided`) f.BoolVar(&syncOptions.IncludeTransitiveNeeds, "include-transitive-needs", false, `like --include-needs, but also includes transitive needs (needs of needs). Does nothing when --selector/-l flag is not provided. Overrides exclusions of other selectors and conditions.`) f.BoolVar(&syncOptions.HideNotes, "hide-notes", false, "add --hide-notes flag to helm") @@ -53,5 +57,8 @@ func NewSyncCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.BoolVar(&syncOptions.SkipSchemaValidation, "skip-schema-validation", false, `pass --skip-schema-validation to "helm template" or "helm upgrade --install"`) f.StringVar(&syncOptions.Cascade, "cascade", "", "pass cascade to helm exec, default: background") + // Register flags using the registrar + flagRegistrar.RegisterFlags(cmd) + return cmd } diff --git a/cmd/template.go b/cmd/template.go index 6cb81fea..9bd0cbdb 100644 --- a/cmd/template.go +++ b/cmd/template.go @@ -5,15 +5,20 @@ import ( "github.com/helmfile/helmfile/pkg/app" "github.com/helmfile/helmfile/pkg/config" + "github.com/helmfile/helmfile/pkg/flags" ) // NewTemplateCmd returm template subcmd func NewTemplateCmd(globalCfg *config.GlobalImpl) *cobra.Command { templateOptions := config.NewTemplateOptions() + flagRegistrar := flags.NewDiffFlagRegistrar() cmd := &cobra.Command{ Use: "template", Short: "Template releases defined in state file", + PreRun: func(cmd *cobra.Command, args []string) { + flagRegistrar.TransferFlags(cmd, templateOptions) + }, RunE: func(cmd *cobra.Command, args []string) error { templateImpl := config.NewTemplateImpl(globalCfg, templateOptions) err := config.NewCLIConfigImpl(templateImpl.GlobalImpl) @@ -38,7 +43,6 @@ func NewTemplateCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringVar(&templateOptions.OutputDirTemplate, "output-dir-template", "", "go text template for generating the output directory. Default: {{ .OutputDir }}/{{ .State.BaseName }}-{{ .State.AbsPathSHA1 }}-{{ .Release.Name}}") f.IntVar(&templateOptions.Concurrency, "concurrency", 0, "maximum number of concurrent helm processes to run, 0 is unlimited") f.BoolVar(&templateOptions.Validate, "validate", false, "validate your manifests against the Kubernetes cluster you are currently pointing at. Note that this requires access to a Kubernetes cluster to obtain information necessary for validating, like the template of available API versions") - f.BoolVar(&templateOptions.IncludeCRDs, "include-crds", false, "include CRDs in the templated output") f.BoolVar(&templateOptions.SkipTests, "skip-tests", false, "skip tests from templated output") f.BoolVar(&templateOptions.SkipNeeds, "skip-needs", true, `do not automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided. Defaults to true when --include-needs or --include-transitive-needs is not provided`) f.BoolVar(&templateOptions.IncludeNeeds, "include-needs", false, `automatically include releases from the target release's "needs" when --selector/-l flag is provided. Does nothing when --selector/-l flag is not provided`) @@ -51,5 +55,6 @@ func NewTemplateCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringVar(&templateOptions.KubeVersion, "kube-version", "", `pass --kube-version to "helm template". Overrides kubeVersion in helmfile.yaml`) f.StringArrayVar(&templateOptions.ShowOnly, "show-only", nil, `pass --show-only to "helm template"`) + flagRegistrar.RegisterFlags(cmd) return cmd } diff --git a/pkg/app/app.go b/pkg/app/app.go index 65751681..37a45065 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -152,13 +152,11 @@ func (a *App) Diff(c DiffConfigProvider) error { var errs []error - includeCRDs := !c.SkipCRDs() - prepErr := run.withPreparedCharts("diff", state.ChartPrepareOptions{ SkipRepos: c.SkipRefresh() || c.SkipDeps(), SkipRefresh: c.SkipRefresh(), SkipDeps: c.SkipDeps(), - IncludeCRDs: &includeCRDs, + IncludeCRDs: c.ShouldIncludeCRDs(), Validate: c.Validate(), Concurrency: c.Concurrency(), IncludeTransitiveNeeds: c.IncludeNeeds(), @@ -215,8 +213,6 @@ func (a *App) Diff(c DiffConfigProvider) error { func (a *App) Template(c TemplateConfigProvider) error { return a.ForEachState(func(run *Run) (ok bool, errs []error) { - includeCRDs := c.IncludeCRDs() - // Live output should never be enabled for the "template" subcommand to avoid breaking `helmfile template | kubectl apply -f -` run.helm.SetEnableLiveOutput(false) @@ -228,7 +224,7 @@ func (a *App) Template(c TemplateConfigProvider) error { SkipRepos: c.SkipRefresh() || c.SkipDeps(), SkipRefresh: c.SkipRefresh(), SkipDeps: c.SkipDeps(), - IncludeCRDs: &includeCRDs, + IncludeCRDs: c.ShouldIncludeCRDs(), SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), @@ -358,8 +354,6 @@ func (a *App) Fetch(c FetchConfigProvider) error { func (a *App) Sync(c SyncConfigProvider) error { return a.ForEachState(func(run *Run) (ok bool, errs []error) { - includeCRDs := !c.SkipCRDs() - prepErr := run.withPreparedCharts("sync", state.ChartPrepareOptions{ SkipRepos: c.SkipRefresh() || c.SkipDeps(), SkipRefresh: c.SkipRefresh(), @@ -367,7 +361,7 @@ func (a *App) Sync(c SyncConfigProvider) error { Wait: c.Wait(), WaitRetries: c.WaitRetries(), WaitForJobs: c.WaitForJobs(), - IncludeCRDs: &includeCRDs, + IncludeCRDs: c.ShouldIncludeCRDs(), IncludeTransitiveNeeds: c.IncludeNeeds(), Validate: c.Validate(), Concurrency: c.Concurrency(), @@ -393,8 +387,6 @@ func (a *App) Apply(c ApplyConfigProvider) error { opts = append(opts, SetRetainValuesFiles(c.SkipCleanup())) err := a.ForEachState(func(run *Run) (ok bool, errs []error) { - includeCRDs := !c.SkipCRDs() - prepErr := run.withPreparedCharts("apply", state.ChartPrepareOptions{ SkipRepos: c.SkipRefresh() || c.SkipDeps(), SkipRefresh: c.SkipRefresh(), @@ -402,7 +394,7 @@ func (a *App) Apply(c ApplyConfigProvider) error { Wait: c.Wait(), WaitRetries: c.WaitRetries(), WaitForJobs: c.WaitForJobs(), - IncludeCRDs: &includeCRDs, + IncludeCRDs: c.ShouldIncludeCRDs(), SkipCleanup: c.SkipCleanup(), Validate: c.Validate(), Concurrency: c.Concurrency(), @@ -1385,6 +1377,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { SkipDiffOnInstall: c.SkipDiffOnInstall(), ReuseValues: c.ReuseValues(), ResetValues: c.ResetValues(), + IncludeCRDs: c.ShouldIncludeCRDs(), DiffArgs: c.DiffArgs(), PostRenderer: c.PostRenderer(), PostRendererArgs: c.PostRendererArgs(), diff --git a/pkg/app/config.go b/pkg/app/config.go index 34dff76d..667c48f9 100644 --- a/pkg/app/config.go +++ b/pkg/app/config.go @@ -50,13 +50,14 @@ type ApplyConfigProvider interface { Values() []string Set() []string - SkipCRDs() bool SkipDeps() bool SkipRefresh() bool Wait() bool WaitRetries() int WaitForJobs() bool + CRDConfig + IncludeTests() bool Suppress() []string @@ -101,9 +102,11 @@ type SyncConfigProvider interface { Values() []string Set() []string - SkipCRDs() bool SkipDeps() bool SkipRefresh() bool + + CRDConfig + Wait() bool WaitRetries() int WaitForJobs() bool @@ -135,12 +138,13 @@ type DiffConfigProvider interface { Values() []string Set() []string Validate() bool - SkipCRDs() bool SkipDeps() bool SkipRefresh() bool IncludeTests() bool + CRDConfig + Suppress() []string SuppressSecrets() bool ShowSecrets() bool @@ -226,8 +230,10 @@ type TemplateConfigProvider interface { SkipRefresh() bool SkipCleanup() bool SkipTests() bool + + CRDConfig + OutputDir() string - IncludeCRDs() bool NoHooks() bool KubeVersion() string ShowOnly() []string @@ -237,6 +243,12 @@ type TemplateConfigProvider interface { concurrencyConfig } +type CRDConfig interface { + SkipCRDs() bool + IncludeCRDs() bool + ShouldIncludeCRDs() bool +} + type DAGConfig interface { SkipNeeds() bool IncludeNeeds() bool diff --git a/pkg/common/bool_flag.go b/pkg/common/bool_flag.go new file mode 100644 index 00000000..8deeeeea --- /dev/null +++ b/pkg/common/bool_flag.go @@ -0,0 +1,43 @@ +package common + +// BoolFlag represents a boolean flag that tracks whether it was explicitly set +type BoolFlag interface { + // Value returns the current boolean value + Value() bool + + // WasExplicitlySet returns whether the flag was explicitly set + WasExplicitlySet() bool + + // Set sets the value and marks the flag as explicitly set + Set(value bool) +} + +// boolFlag is the implementation of BoolFlag +type boolFlag struct { + value bool + wasExplicitlySet bool +} + +// NewBoolFlag creates a new BoolFlag with default values +func NewBoolFlag(defaultValue bool) BoolFlag { + return &boolFlag{ + value: defaultValue, + wasExplicitlySet: false, + } +} + +// Value returns the current boolean value +func (bf *boolFlag) Value() bool { + return bf.value +} + +// WasExplicitlySet returns whether the flag was explicitly set +func (bf *boolFlag) WasExplicitlySet() bool { + return bf.wasExplicitlySet +} + +// Set sets the value and marks the flag as explicitly set +func (bf *boolFlag) Set(value bool) { + bf.value = value + bf.wasExplicitlySet = true +} diff --git a/pkg/common/bool_flag_test.go b/pkg/common/bool_flag_test.go new file mode 100644 index 00000000..2475118e --- /dev/null +++ b/pkg/common/bool_flag_test.go @@ -0,0 +1,94 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewBoolFlag(t *testing.T) { + tests := []struct { + name string + defaultValue bool + expected bool + }{ + { + name: "default true", + defaultValue: true, + expected: true, + }, + { + name: "default false", + defaultValue: false, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewBoolFlag(tt.defaultValue) + + // Check initial state + assert.Equal(t, tt.expected, flag.Value(), "Value should match default") + assert.False(t, flag.WasExplicitlySet(), "New flag should not be marked as explicitly set") + }) + } +} + +func TestBoolFlag_Set(t *testing.T) { + tests := []struct { + name string + defaultValue bool + setValue bool + expected bool + }{ + { + name: "default false, set true", + defaultValue: false, + setValue: true, + expected: true, + }, + { + name: "default true, set false", + defaultValue: true, + setValue: false, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewBoolFlag(tt.defaultValue) + + // Set the value + flag.Set(tt.setValue) + + // Check state after setting + assert.Equal(t, tt.expected, flag.Value(), "Value should match set value") + assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + }) + } +} + +func TestBoolFlag_MultipleSet(t *testing.T) { + flag := NewBoolFlag(false) + + // Initial state + assert.False(t, flag.Value()) + assert.False(t, flag.WasExplicitlySet()) + + // First set + flag.Set(true) + assert.True(t, flag.Value()) + assert.True(t, flag.WasExplicitlySet()) + + // Second set + flag.Set(false) + assert.False(t, flag.Value()) + assert.True(t, flag.WasExplicitlySet(), "Flag should remain explicitly set") +} + +func TestBoolFlag_Implementation(t *testing.T) { + // Test that boolFlag properly implements BoolFlag interface + var _ BoolFlag = &boolFlag{} +} diff --git a/pkg/common/string_array_flag.go b/pkg/common/string_array_flag.go new file mode 100644 index 00000000..ed55073d --- /dev/null +++ b/pkg/common/string_array_flag.go @@ -0,0 +1,58 @@ + + + +package common + +// For array/slice flags +type StringArrayFlag interface { + Values() []string + WasExplicitlySet() bool + Add(value string) + Set(values []string) +} + +type stringArrayFlag struct { + values []string + wasExplicitlySet bool +} + +// NewStringArrayFlag creates a new StringArrayFlag with the given default values +// Create a defensive copy of the default values to prevent external modifications +// and to ensure that the original values remain unchanged. +func NewStringArrayFlag(defaultValues []string) StringArrayFlag { + valuesCopy := make([]string, len(defaultValues)) + copy(valuesCopy, defaultValues) + + return &stringArrayFlag{ + values: valuesCopy, + wasExplicitlySet: false, + } +} +// Values returns the values of the flag +// It returns a copy of the values to prevent external modifications +// and to ensure that the original values remain unchanged. +// This is important for flags that may be modified by the user +// or other parts of the program. +func (f *stringArrayFlag) Values() []string { + // Return a copy to prevent external modifications + valuesCopy := make([]string, len(f.values)) + copy(valuesCopy, f.values) + return valuesCopy +} + +// WasExplicitlySet returns whether the flag was explicitly set +func (f *stringArrayFlag) WasExplicitlySet() bool { + return f.wasExplicitlySet +} + +// Set sets the values and marks the flag as explicitly set +func (f *stringArrayFlag) Set(values []string) { + f.values = values + f.wasExplicitlySet = true +} + +// Add sets the value and marks the flag as explicitly set +func (f *stringArrayFlag) Add(value string) { + f.values = append(f.values, value) + f.wasExplicitlySet = true +} diff --git a/pkg/common/string_array_flag_test.go b/pkg/common/string_array_flag_test.go new file mode 100644 index 00000000..5bd45aba --- /dev/null +++ b/pkg/common/string_array_flag_test.go @@ -0,0 +1,195 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewStringArrayFlag(t *testing.T) { + tests := []struct { + name string + defaultValue []string + expected []string + }{ + { + name: "nil default", + defaultValue: nil, + expected: []string{}, + }, + { + name: "empty default", + defaultValue: []string{}, + expected: []string{}, + }, + { + name: "non-empty default", + defaultValue: []string{"value1", "value2"}, + expected: []string{"value1", "value2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewStringArrayFlag(tt.defaultValue) + + // Check initial state + assert.Equal(t, tt.expected, flag.Values(), "Values should match default") + assert.False(t, flag.WasExplicitlySet(), "New flag should not be marked as explicitly set") + }) + } +} + +func TestStringArrayFlag_Set(t *testing.T) { + tests := []struct { + name string + defaultValue []string + setValue []string + expected []string + }{ + { + name: "nil default, set values", + defaultValue: nil, + setValue: []string{"new1", "new2"}, + expected: []string{"new1", "new2"}, + }, + { + name: "non-empty default, set empty", + defaultValue: []string{"default1", "default2"}, + setValue: []string{}, + expected: []string{}, + }, + { + name: "non-empty default, set new values", + defaultValue: []string{"default1", "default2"}, + setValue: []string{"new1", "new2"}, + expected: []string{"new1", "new2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewStringArrayFlag(tt.defaultValue) + + // Set the values + flag.Set(tt.setValue) + + // Check state after setting + assert.Equal(t, tt.expected, flag.Values(), "Values should match set values") + assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + }) + } +} + +func TestStringArrayFlag_Add(t *testing.T) { + tests := []struct { + name string + defaultValue []string + addValue string + expected []string + }{ + { + name: "nil default, add value", + defaultValue: nil, + addValue: "new", + expected: []string{"new"}, + }, + { + name: "empty default, add value", + defaultValue: []string{}, + addValue: "new", + expected: []string{"new"}, + }, + { + name: "non-empty default, add value", + defaultValue: []string{"default1", "default2"}, + addValue: "new", + expected: []string{"default1", "default2", "new"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewStringArrayFlag(tt.defaultValue) + + // Add the value + flag.Add(tt.addValue) + + // Check state after adding + assert.Equal(t, tt.expected, flag.Values(), "Values should include added value") + assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + }) + } +} + +func TestStringArrayFlag_MultipleOperations(t *testing.T) { + flag := NewStringArrayFlag([]string{"initial"}) + + // Initial state + assert.Equal(t, []string{"initial"}, flag.Values()) + assert.False(t, flag.WasExplicitlySet()) + + // Add a value + flag.Add("added") + assert.Equal(t, []string{"initial", "added"}, flag.Values()) + assert.True(t, flag.WasExplicitlySet()) + + // Set completely new values + flag.Set([]string{"new1", "new2"}) + assert.Equal(t, []string{"new1", "new2"}, flag.Values()) + assert.True(t, flag.WasExplicitlySet(), "Flag should remain explicitly set") + + // Add another value after Set + flag.Add("added2") + assert.Equal(t, []string{"new1", "new2", "added2"}, flag.Values()) + assert.True(t, flag.WasExplicitlySet()) +} + +func TestStringArrayFlag_Implementation(t *testing.T) { + // Test that stringArrayFlag properly implements StringArrayFlag interface + var _ StringArrayFlag = &stringArrayFlag{} +} + +func TestStringArrayFlag_DefensiveCopy(t *testing.T) { + // Test that modifying the original slice doesn't affect the flag + original := []string{"value1", "value2"} + flag := NewStringArrayFlag(original) + + // Verify initial state + assert.Equal(t, []string{"value1", "value2"}, flag.Values()) + + // Modify the original slice - should NOT affect the flag's internal state + // because NewStringArrayFlag creates a defensive copy + original[0] = "modified" + original = append(original, "added") + + // Flag values should remain unchanged + assert.Equal(t, []string{"value1", "value2"}, flag.Values()) + + // Test that modifying the returned slice doesn't affect the flag + values := flag.Values() + values[0] = "modified" + values = append(values, "added") + + // Flag values should remain unchanged because Values() returns a copy + assert.Equal(t, []string{"value1", "value2"}, flag.Values()) +} + +func TestStringArrayFlag_SetDefensiveCopy(t *testing.T) { + // Test that Set doesn't create a defensive copy (current implementation) + flag := NewStringArrayFlag([]string{}) + + // Create a slice to set + setValues := []string{"value1", "value2"} + flag.Set(setValues) + + // Verify state after setting + assert.Equal(t, []string{"value1", "value2"}, flag.Values()) + + // Modify the original slice - this WILL affect the flag's internal state + // because Set doesn't create a defensive copy in the current implementation + setValues[0] = "modified" + + // Flag values will reflect the modification + assert.Equal(t, []string{"modified", "value2"}, flag.Values()) +} diff --git a/pkg/common/string_flag.go b/pkg/common/string_flag.go new file mode 100644 index 00000000..4c878e84 --- /dev/null +++ b/pkg/common/string_flag.go @@ -0,0 +1,35 @@ +package common + +type StringFlag interface { + Value() string + WasExplicitlySet() bool + Set(value string) +} + +type stringFlag struct { + value string + wasExplicitlySet bool +} + +func NewStringFlag(defaultValue string) StringFlag { + return &stringFlag{ + value: defaultValue, + wasExplicitlySet: false, + } +} + +// Value returns the current boolean value +func (f *stringFlag) Value() string { + return f.value +} + +// WasExplicitlySet returns whether the flag was explicitly set +func (f *stringFlag) WasExplicitlySet() bool { + return f.wasExplicitlySet +} + +// Set sets the value and marks the flag as explicitly set +func (f *stringFlag) Set(value string) { + f.value = value + f.wasExplicitlySet = true +} \ No newline at end of file diff --git a/pkg/common/string_flag_test.go b/pkg/common/string_flag_test.go new file mode 100644 index 00000000..ad246edb --- /dev/null +++ b/pkg/common/string_flag_test.go @@ -0,0 +1,100 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNewStringFlag(t *testing.T) { + tests := []struct { + name string + defaultValue string + expected string + }{ + { + name: "empty default", + defaultValue: "", + expected: "", + }, + { + name: "non-empty default", + defaultValue: "default", + expected: "default", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewStringFlag(tt.defaultValue) + + // Check initial state + assert.Equal(t, tt.expected, flag.Value(), "Value should match default") + assert.False(t, flag.WasExplicitlySet(), "New flag should not be marked as explicitly set") + }) + } +} + +func TestStringFlag_Set(t *testing.T) { + tests := []struct { + name string + defaultValue string + setValue string + expected string + }{ + { + name: "empty default, set value", + defaultValue: "", + setValue: "new value", + expected: "new value", + }, + { + name: "non-empty default, set empty", + defaultValue: "default", + setValue: "", + expected: "", + }, + { + name: "non-empty default, set new value", + defaultValue: "default", + setValue: "new value", + expected: "new value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + flag := NewStringFlag(tt.defaultValue) + + // Set the value + flag.Set(tt.setValue) + + // Check state after setting + assert.Equal(t, tt.expected, flag.Value(), "Value should match set value") + assert.True(t, flag.WasExplicitlySet(), "Flag should be marked as explicitly set") + }) + } +} + +func TestStringFlag_MultipleSet(t *testing.T) { + flag := NewStringFlag("initial") + + // Initial state + assert.Equal(t, "initial", flag.Value()) + assert.False(t, flag.WasExplicitlySet()) + + // First set + flag.Set("first") + assert.Equal(t, "first", flag.Value()) + assert.True(t, flag.WasExplicitlySet()) + + // Second set + flag.Set("second") + assert.Equal(t, "second", flag.Value()) + assert.True(t, flag.WasExplicitlySet(), "Flag should remain explicitly set") +} + +func TestStringFlag_Implementation(t *testing.T) { + // Test that stringFlag properly implements StringFlag interface + var _ StringFlag = &stringFlag{} +} diff --git a/pkg/config/apply.go b/pkg/config/apply.go index 7cfc6d3c..73ddc355 100644 --- a/pkg/config/apply.go +++ b/pkg/config/apply.go @@ -1,5 +1,7 @@ package config +import "github.com/helmfile/helmfile/pkg/common" + // ApplyOptoons is the options for the apply command type ApplyOptions struct { // Set is a list of key value pairs to be merged into the command @@ -20,8 +22,10 @@ type ApplyOptions struct { StripTrailingCR bool // SkipCleanup is true if the cleanup of temporary values files should be skipped SkipCleanup bool - // SkipCRDs is true if the CRDs should be skipped - SkipCRDs bool + // SkipCRDsFlag is true if the CRDs should be skipped + SkipCRDsFlag common.BoolFlag + // IncludeCRDsFlag is true if the CRDs should be included + IncludeCRDsFlag common.BoolFlag // SkipNeeds is true if the needs should be skipped SkipNeeds bool // IncludeNeeds is true if the needs should be included @@ -77,7 +81,12 @@ type ApplyOptions struct { // NewApply creates a new Apply func NewApplyOptions() *ApplyOptions { - return &ApplyOptions{} + newOptions := &ApplyOptions{ + IncludeCRDsFlag: common.NewBoolFlag(false), + SkipCRDsFlag: common.NewBoolFlag(false), + } + + return newOptions } // ApplyImpl is impl for applyOptions @@ -149,9 +158,22 @@ func (a *ApplyImpl) NoHooks() bool { return a.ApplyOptions.NoHooks } -// SkipCRDs returns the skip crds. +// SkipCRDs returns the skip CRDs. func (a *ApplyImpl) SkipCRDs() bool { - return a.ApplyOptions.SkipCRDs + return a.ApplyOptions.SkipCRDsFlag.Value() +} + +// IncludeCRDs returns the include CRDs. +func (a *ApplyImpl) IncludeCRDs() bool { + return a.ApplyOptions.IncludeCRDsFlag.Value() +} + +// ShouldIncludeCRDs returns true if CRDs should be included. +func (a *ApplyImpl) ShouldIncludeCRDs() bool { + includeCRDsExplicit := a.IncludeCRDsFlag.WasExplicitlySet() && a.IncludeCRDsFlag.Value() + skipCRDsExplicit := a.SkipCRDsFlag.WasExplicitlySet() && !a.SkipCRDsFlag.Value() + + return includeCRDsExplicit || skipCRDsExplicit } // SkipCleanup returns the skip cleanup. diff --git a/pkg/config/diff.go b/pkg/config/diff.go index ac8018b9..e103836d 100644 --- a/pkg/config/diff.go +++ b/pkg/config/diff.go @@ -1,5 +1,7 @@ package config +import "github.com/helmfile/helmfile/pkg/common" + // DiffOptions is the options for the build command type DiffOptions struct { // Set is the set flag @@ -12,6 +14,10 @@ type DiffOptions struct { StripTrailingCR bool // IncludeTests is the include tests flag IncludeTests bool + // SkipCRDsFlag is the skip crds flag + SkipCRDsFlag common.BoolFlag + // IncludeCRDsFlag is the include crds flag + IncludeCRDsFlag common.BoolFlag // SkipNeeds is the include crds flag SkipNeeds bool // IncludeNeeds is the include needs flag @@ -53,7 +59,12 @@ type DiffOptions struct { // NewDiffOptions creates a new Apply func NewDiffOptions() *DiffOptions { - return &DiffOptions{} + newOptions := &DiffOptions{ + IncludeCRDsFlag: common.NewBoolFlag(false), + SkipCRDsFlag: common.NewBoolFlag(false), + } + + return newOptions } // DiffImpl is impl for applyOptions @@ -144,9 +155,22 @@ func (t *DiffImpl) NoHooks() bool { return t.DiffOptions.NoHooks } -// ShowCRDs returns the show crds +// SkipCRDs returns the skip crds func (t *DiffImpl) SkipCRDs() bool { - return false + return t.DiffOptions.SkipCRDsFlag.Value() +} + +// IncludeCRDs returns the include crds +func (t *DiffImpl) IncludeCRDs() bool { + return t.DiffOptions.IncludeCRDsFlag.Value() +} + +// ShouldIncludeCRDs returns true if CRDs should be included +func (t *DiffImpl) ShouldIncludeCRDs() bool { + includeCRDsExplicit := t.IncludeCRDsFlag.WasExplicitlySet() && t.IncludeCRDsFlag.Value() + skipCRDsExplicit := t.SkipCRDsFlag.WasExplicitlySet() && !t.SkipCRDsFlag.Value() + + return includeCRDsExplicit || skipCRDsExplicit } // SkipDiffOnInstall returns the skip diff on install diff --git a/pkg/config/options.go b/pkg/config/options.go new file mode 100644 index 00000000..c875d915 --- /dev/null +++ b/pkg/config/options.go @@ -0,0 +1,65 @@ +package config + +func (o *ApplyOptions) HandleFlag(name string, value interface{}, changed bool) { + if !changed { + return + } + + switch name { + case "include-crds": + if boolVal, ok := value.(*bool); ok { + o.IncludeCRDsFlag.Set(*boolVal) + } + case "skip-crds": + if boolVal, ok := value.(*bool); ok { + o.SkipCRDsFlag.Set(*boolVal) + } + // Handle other flags... + } +} + +func (o *DiffOptions) HandleFlag(name string, value interface{}, changed bool) { + if !changed { + return + } + + switch name { + case "include-crds": + if boolVal, ok := value.(*bool); ok { + o.IncludeCRDsFlag.Set(*boolVal) + } + // Handle other flags... + } +} + +func (o *SyncOptions) HandleFlag(name string, value interface{}, changed bool) { + if !changed { + return + } + + switch name { + case "include-crds": + if boolVal, ok := value.(*bool); ok { + o.IncludeCRDsFlag.Set(*boolVal) + } + case "skip-crds": + if boolVal, ok := value.(*bool); ok { + o.SkipCRDsFlag.Set(*boolVal) + } + // Handle other flags... + } +} + +func (o *TemplateOptions) HandleFlag(name string, value interface{}, changed bool) { + if !changed { + return + } + + switch name { + case "include-crds": + if boolVal, ok := value.(*bool); ok { + o.IncludeCRDsFlag.Set(*boolVal) + } + // Handle other flags... + } +} \ No newline at end of file diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 71d38034..388e48eb 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -1,5 +1,7 @@ package config +import "github.com/helmfile/helmfile/pkg/common" + // SyncOptions is the options for the build command type SyncOptions struct { // Set is the set flag @@ -16,8 +18,10 @@ type SyncOptions struct { IncludeNeeds bool // IncludeTransitiveNeeds is the include transitive needs flag IncludeTransitiveNeeds bool - // SkipCrds is the skip crds flag - SkipCRDs bool + // SkipCRDsFlag is the skip crds flag + SkipCRDsFlag common.BoolFlag + // IncludeCRDsFlag is the include crds flag + IncludeCRDsFlag common.BoolFlag // Wait is the wait flag Wait bool // WaitRetries is the wait retries flag @@ -48,7 +52,12 @@ type SyncOptions struct { // NewSyncOptions creates a new Apply func NewSyncOptions() *SyncOptions { - return &SyncOptions{} + newOptions := &SyncOptions{ + IncludeCRDsFlag: common.NewBoolFlag(false), + SkipCRDsFlag: common.NewBoolFlag(false), + } + + return newOptions } // SyncImpl is impl for applyOptions @@ -104,9 +113,22 @@ func (t *SyncImpl) Values() []string { return t.SyncOptions.Values } -// SkipCRDS returns the skip crds +// SkipCRDs returns the skip crds func (t *SyncImpl) SkipCRDs() bool { - return t.SyncOptions.SkipCRDs + return t.SyncOptions.SkipCRDsFlag.Value() +} + +// IncludeCRDs returns the include crds +func (t *SyncImpl) IncludeCRDs() bool { + return t.SyncOptions.IncludeCRDsFlag.Value() +} + +// ShouldIncludeCRDs returns true if CRDs should be included +func (t *SyncImpl) ShouldIncludeCRDs() bool { + includeCRDsExplicit := t.IncludeCRDsFlag.WasExplicitlySet() && t.IncludeCRDsFlag.Value() + skipCRDsExplicit := t.SkipCRDsFlag.WasExplicitlySet() && !t.SkipCRDsFlag.Value() + + return includeCRDsExplicit || skipCRDsExplicit } // Wait returns the wait diff --git a/pkg/config/template.go b/pkg/config/template.go index 90c82691..902e2d38 100644 --- a/pkg/config/template.go +++ b/pkg/config/template.go @@ -4,6 +4,8 @@ import ( "fmt" "os" "strings" + + "github.com/helmfile/helmfile/pkg/common" ) // TemplateOptions is the options for the build command @@ -20,8 +22,11 @@ type TemplateOptions struct { Concurrency int // Validate is the validate flag Validate bool - // IncludeCRDs is the include crds flag - IncludeCRDs bool + // SkipCRDsFlag is the skip crds flag + // Deprecated: Use IncludeCRDsFlag instead + SkipCRDsFlag common.BoolFlag + // IncludeCRDsFlag is the include crds flag + IncludeCRDsFlag common.BoolFlag // SkipTests is the skip tests flag SkipTests bool // SkipNeeds is the skip needs flag @@ -48,7 +53,11 @@ type TemplateOptions struct { // NewTemplateOptions creates a new Apply func NewTemplateOptions() *TemplateOptions { - return &TemplateOptions{} + newOptions := &TemplateOptions{ + SkipCRDsFlag: common.NewBoolFlag(false), + IncludeCRDsFlag: common.NewBoolFlag(false), + } + return newOptions } // TemplateImpl is impl for applyOptions @@ -70,9 +79,22 @@ func (t *TemplateImpl) Concurrency() int { return t.TemplateOptions.Concurrency } +// SkipCRDs returns the skip crds +func (t *TemplateImpl) SkipCRDs() bool { + return t.TemplateOptions.SkipCRDsFlag.Value() +} + // IncludeCRDs returns the include crds func (t *TemplateImpl) IncludeCRDs() bool { - return t.TemplateOptions.IncludeCRDs + return t.TemplateOptions.IncludeCRDsFlag.Value() +} + +// ShouldIncludeCRDs returns whether to include crds +func (t *TemplateImpl) ShouldIncludeCRDs() bool { + includeCRDsExplicit := t.IncludeCRDsFlag.WasExplicitlySet() && t.IncludeCRDsFlag.Value() + skipCRDsExplicit := t.SkipCRDsFlag.WasExplicitlySet() && !t.SkipCRDsFlag.Value() + + return includeCRDsExplicit || skipCRDsExplicit } // NoHooks returns the no hooks diff --git a/pkg/flags/apply.go b/pkg/flags/apply.go new file mode 100644 index 00000000..873350ed --- /dev/null +++ b/pkg/flags/apply.go @@ -0,0 +1,24 @@ +package flags + +import "github.com/spf13/cobra" + +// ApplyFlagRegistrar handles flags specific to the apply command +type ApplyFlagRegistrar struct { + *GenericFlagRegistrar + IncludeCRDs bool + SkipCRDs bool +} + +// NewApplyFlagRegistrar creates a new ApplyFlagRegistrar +func NewApplyFlagRegistrar() *ApplyFlagRegistrar { + return &ApplyFlagRegistrar{ + GenericFlagRegistrar: NewGenericFlagRegistrar(), + } +} + +// RegisterFlags registers apply-specific flags +func (r *ApplyFlagRegistrar) RegisterFlags(cmd *cobra.Command) { + r.RegisterBoolFlag(cmd, "include-crds", &r.IncludeCRDs, false, "include CRDs in the diffing") + r.RegisterBoolFlag(cmd, "skip-crds", &r.SkipCRDs, false, "if set, no CRDs will be installed on sync. By default, CRDs are installed if not already present") +} + diff --git a/pkg/flags/common.go b/pkg/flags/common.go new file mode 100644 index 00000000..703d912f --- /dev/null +++ b/pkg/flags/common.go @@ -0,0 +1,49 @@ +package flags + +import ( + "github.com/spf13/cobra" +) + +// FlagRegistrar defines an interface for registering and transferring flags +type FlagRegistrar interface { + RegisterFlags(cmd *cobra.Command) + TransferFlags(cmd *cobra.Command, opts interface{}) +} + +// FlagHandler is a generic interface for handling flag values +type FlagHandler interface { + // HandleFlag receives a flag name, value, and whether it was changed + HandleFlag(name string, value interface{}, changed bool) +} + +// GenericFlagRegistrar is a base struct for flag registrars +type GenericFlagRegistrar struct { + // Map of flag names to their values + values map[string]interface{} +} + +// NewGenericFlagRegistrar creates a new GenericFlagRegistrar +func NewGenericFlagRegistrar() *GenericFlagRegistrar { + return &GenericFlagRegistrar{ + values: make(map[string]interface{}), + } +} + +// TransferFlags transfers all registered flags to the options +func (r *GenericFlagRegistrar) TransferFlags(cmd *cobra.Command, opts interface{}) { + if handler, ok := opts.(FlagHandler); ok { + flags := cmd.Flags() + + // Transfer each registered flag + for name, value := range r.values { + changed := flags.Changed(name) + handler.HandleFlag(name, value, changed) + } + } +} + +// RegisterBoolFlag registers a boolean flag and stores its reference +func (r *GenericFlagRegistrar) RegisterBoolFlag(cmd *cobra.Command, name string, value *bool, defaultValue bool, usage string) { + cmd.Flags().BoolVar(value, name, defaultValue, usage) + r.values[name] = value +} diff --git a/pkg/flags/common_test.go b/pkg/flags/common_test.go new file mode 100644 index 00000000..605edd55 --- /dev/null +++ b/pkg/flags/common_test.go @@ -0,0 +1,140 @@ +package flags + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" +) + +// MockFlagHandler implements FlagHandler for testing +type MockFlagHandler struct { + handledFlags map[string]interface{} + changedFlags map[string]bool +} + +func NewMockFlagHandler() *MockFlagHandler { + return &MockFlagHandler{ + handledFlags: make(map[string]interface{}), + changedFlags: make(map[string]bool), + } +} + +func (h *MockFlagHandler) HandleFlag(name string, value interface{}, changed bool) { + h.handledFlags[name] = value + h.changedFlags[name] = changed +} + +func TestNewGenericFlagRegistrar(t *testing.T) { + registrar := NewGenericFlagRegistrar() + assert.NotNil(t, registrar) + assert.NotNil(t, registrar.values) + assert.Len(t, registrar.values, 0) +} + +func TestGenericFlagRegistrar_RegisterBoolFlag(t *testing.T) { + registrar := NewGenericFlagRegistrar() + cmd := &cobra.Command{Use: "test"} + + var testFlag bool + registrar.RegisterBoolFlag(cmd, "test-flag", &testFlag, false, "Test flag") + + // Verify the flag was registered + flag := cmd.Flags().Lookup("test-flag") + assert.NotNil(t, flag) + assert.Equal(t, "test-flag", flag.Name) + assert.Equal(t, "false", flag.DefValue) + assert.Equal(t, "Test flag", flag.Usage) + + // Verify the value was stored in the registrar + value, exists := registrar.values["test-flag"] + assert.True(t, exists) + assert.Equal(t, &testFlag, value) +} + +func TestGenericFlagRegistrar_TransferFlags_NoChanges(t *testing.T) { + registrar := NewGenericFlagRegistrar() + cmd := &cobra.Command{Use: "test"} + + var testFlag bool + registrar.RegisterBoolFlag(cmd, "test-flag", &testFlag, false, "Test flag") + + // Create a mock handler + handler := NewMockFlagHandler() + + // Transfer flags (none changed) + registrar.TransferFlags(cmd, handler) + + // Verify the handler was called with the right parameters + assert.Equal(t, &testFlag, handler.handledFlags["test-flag"]) + assert.False(t, handler.changedFlags["test-flag"]) +} + +func TestGenericFlagRegistrar_TransferFlags_WithChanges(t *testing.T) { + registrar := NewGenericFlagRegistrar() + cmd := &cobra.Command{Use: "test"} + + var testFlag bool + registrar.RegisterBoolFlag(cmd, "test-flag", &testFlag, false, "Test flag") + + // Simulate flag being set on command line + err := cmd.Flags().Set("test-flag", "true") + assert.NoError(t, err) + testFlag = true // Value would be updated by cobra + + // Create a mock handler + handler := NewMockFlagHandler() + + // Transfer flags (with changes) + registrar.TransferFlags(cmd, handler) + + // Verify the handler was called with the right parameters + assert.Equal(t, &testFlag, handler.handledFlags["test-flag"]) + assert.True(t, handler.changedFlags["test-flag"]) + assert.True(t, *handler.handledFlags["test-flag"].(*bool)) +} + +func TestGenericFlagRegistrar_TransferFlags_NonHandler(t *testing.T) { + registrar := NewGenericFlagRegistrar() + cmd := &cobra.Command{Use: "test"} + + var testFlag bool + registrar.RegisterBoolFlag(cmd, "test-flag", &testFlag, false, "Test flag") + + // Use a non-handler type + nonHandler := struct{}{} + + // This should not panic + registrar.TransferFlags(cmd, nonHandler) +} + +func TestGenericFlagRegistrar_MultipleFlags(t *testing.T) { + registrar := NewGenericFlagRegistrar() + cmd := &cobra.Command{Use: "test"} + + var boolFlag bool + var boolFlag2 bool + + registrar.RegisterBoolFlag(cmd, "bool-flag", &boolFlag, false, "Boolean flag") + registrar.RegisterBoolFlag(cmd, "bool-flag2", &boolFlag2, true, "Another boolean flag") + + // Set one flag + err := cmd.Flags().Set("bool-flag", "true") + assert.NoError(t, err) + boolFlag = true // Value would be updated by cobra + + // Create a mock handler + handler := NewMockFlagHandler() + + // Transfer flags + registrar.TransferFlags(cmd, handler) + + // Verify both flags were handled correctly + assert.Equal(t, &boolFlag, handler.handledFlags["bool-flag"]) + assert.True(t, handler.changedFlags["bool-flag"]) + assert.True(t, *handler.handledFlags["bool-flag"].(*bool)) + + assert.Equal(t, &boolFlag2, handler.handledFlags["bool-flag2"]) + assert.False(t, handler.changedFlags["bool-flag2"]) + assert.True(t, *handler.handledFlags["bool-flag2"].(*bool)) // Default is true +} diff --git a/pkg/flags/diff.go b/pkg/flags/diff.go new file mode 100644 index 00000000..bdebd804 --- /dev/null +++ b/pkg/flags/diff.go @@ -0,0 +1,22 @@ +package flags + +import "github.com/spf13/cobra" + +// DiffFlagRegistrar handles flags specific to the diff command +type DiffFlagRegistrar struct { + *GenericFlagRegistrar + IncludeCRDs bool +} + +// NewDiffFlagRegistrar creates a new DiffFlagRegistrar +func NewDiffFlagRegistrar() *DiffFlagRegistrar { + return &DiffFlagRegistrar{ + GenericFlagRegistrar: NewGenericFlagRegistrar(), + } +} + +// RegisterFlags registers diff-specific flags +func (r *DiffFlagRegistrar) RegisterFlags(cmd *cobra.Command) { + r.RegisterBoolFlag(cmd, "include-crds", &r.IncludeCRDs, false, "include CRDs in the diffing") + // Diff doesn't have skip-crds +} diff --git a/pkg/flags/initializer.go b/pkg/flags/initializer.go new file mode 100644 index 00000000..c301c9e1 --- /dev/null +++ b/pkg/flags/initializer.go @@ -0,0 +1,32 @@ +package flags + +import ( + "github.com/helmfile/helmfile/pkg/common" +) + +// BoolFlagInitializer ensures a BoolFlag is initialized with a default value if nil +func EnsureBoolFlag(flag *common.BoolFlag, defaultValue bool) { + if *flag == nil { + *flag = common.NewBoolFlag(defaultValue) + } +} + +// StringFlagInitializer ensures a StringFlag is initialized with a default value if nil +func EnsureStringFlag(flag *common.StringFlag, defaultValue string) { + if *flag == nil { + *flag = common.NewStringFlag(defaultValue) + } +} + +// StringArrayFlagInitializer ensures a StringArrayFlag is initialized with default values if nil +func EnsureStringArrayFlag(flag *common.StringArrayFlag, defaultValues []string) { + if *flag == nil { + *flag = common.NewStringArrayFlag(defaultValues) + } +} + +// InitializeOptions initializes all nil flag fields in an options struct +func InitializeOptions(options interface{}) { + // This could be expanded to use reflection to automatically find and initialize + // all flag fields in any options struct +} \ No newline at end of file diff --git a/pkg/flags/sync.go b/pkg/flags/sync.go new file mode 100644 index 00000000..300c04b7 --- /dev/null +++ b/pkg/flags/sync.go @@ -0,0 +1,24 @@ +package flags + +import "github.com/spf13/cobra" + +// SyncFlagRegistrar handles flags specific to the sync command +type SyncFlagRegistrar struct { + *GenericFlagRegistrar + IncludeCRDs bool + SkipCRDs bool +} + +// NewSyncFlagRegistrar creates a new SyncFlagRegistrar +func NewSyncFlagRegistrar() *SyncFlagRegistrar { + return &SyncFlagRegistrar{ + GenericFlagRegistrar: NewGenericFlagRegistrar(), + } +} + +// RegisterFlags registers sync-specific flags +func (r *SyncFlagRegistrar) RegisterFlags(cmd *cobra.Command) { + r.RegisterBoolFlag(cmd, "include-crds", &r.IncludeCRDs, false, "include CRDs in the diffing") + r.RegisterBoolFlag(cmd, "skip-crds", &r.SkipCRDs, false, "if set, no CRDs will be installed on sync. By default, CRDs are installed if not already present") +} + diff --git a/pkg/flags/template.go b/pkg/flags/template.go new file mode 100644 index 00000000..5af173b2 --- /dev/null +++ b/pkg/flags/template.go @@ -0,0 +1,22 @@ +package flags + +import "github.com/spf13/cobra" + +// TemplateFlagRegistrar handles flags specific to the template command +type TemplateFlagRegistrar struct { + *GenericFlagRegistrar + IncludeCRDs bool +} + +// NewTemplateFlagRegistrar creates a new TemplateFlagRegistrar +func NewTemplateFlagRegistrar() *TemplateFlagRegistrar { + return &TemplateFlagRegistrar{ + GenericFlagRegistrar: NewGenericFlagRegistrar(), + } +} + +// RegisterFlags registers template-specific flags +func (r *TemplateFlagRegistrar) RegisterFlags(cmd *cobra.Command) { + r.RegisterBoolFlag(cmd, "include-crds", &r.IncludeCRDs, false, "include CRDs in the diffing") +} + diff --git a/pkg/state/state.go b/pkg/state/state.go index 47cdde12..3feb949e 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1136,7 +1136,7 @@ type ChartPrepareOptions struct { // Validate is a helm-3-only option. When it is set to true, it configures chartify to pass --validate to helm-template run by it. // It's required when one of your chart relies on Capabilities.APIVersions in a template Validate bool - IncludeCRDs *bool + IncludeCRDs bool Wait bool WaitRetries int WaitForJobs bool @@ -1298,33 +1298,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre ) chartifyOpts := chartification.Opts - - if skipDeps { - chartifyOpts.SkipDeps = true - } - - includeCRDs := true - if opts.IncludeCRDs != nil { - includeCRDs = *opts.IncludeCRDs - } - chartifyOpts.IncludeCRDs = includeCRDs - - chartifyOpts.Validate = opts.Validate - - chartifyOpts.KubeVersion = st.getKubeVersion(release, opts.KubeVersion) - chartifyOpts.ApiVersions = st.getApiVersions(release) - - if opts.Values != nil { - chartifyOpts.ValuesFiles = append(opts.Values, chartifyOpts.ValuesFiles...) - } - - // https://github.com/helmfile/helmfile/pull/867 - // https://github.com/helmfile/helmfile/issues/895 - var flags []string - for _, s := range opts.Set { - flags = append(flags, "--set", s) - } - chartifyOpts.SetFlags = append(chartifyOpts.SetFlags, flags...) + st.mergeChartifyOpts(chartifyOpts, opts, skipDeps, release) out, err := c.Chartify(release.Name, chartPath, chartify.WithChartifyOpts(chartifyOpts)) if err != nil { @@ -1454,6 +1428,33 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre return prepareChartInfo, nil } +// mergeChartifyOpts merges the source opts into the destination opts, modifying dest directly +func (st *HelmState) mergeChartifyOpts(dest *chartify.ChartifyOpts, source ChartPrepareOptions, skipDeps bool, release *ReleaseSpec) { + // @TODO why is this not just `chartifyOpts.SkipDeps = skipDeps`?? + if skipDeps { + dest.SkipDeps = true + } + + dest.IncludeCRDs = source.IncludeCRDs + + dest.Validate = source.Validate + + dest.KubeVersion = st.getKubeVersion(release, source.KubeVersion) + dest.ApiVersions = st.getApiVersions(release) + + if source.Values != nil { + dest.ValuesFiles = append(source.Values, dest.ValuesFiles...) + } + + // https://github.com/helmfile/helmfile/pull/867 + // https://github.com/helmfile/helmfile/issues/895 + var flags []string + for _, s := range source.Set { + flags = append(flags, "--set", s) + } + dest.SetFlags = append(dest.SetFlags, flags...) +} + // nolint: unparam func (st *HelmState) runHelmDepBuilds(helm helmexec.Interface, concurrency int, builds []*chartPrepareResult) error { // NOTES: @@ -1852,6 +1853,12 @@ func (st *HelmState) commonDiffFlags(detailedExitCode bool, stripTrailingCR bool flags = append(flags, "--set", s) } } + + // internally we only care about include-crds since skip-crds is misleading and used only in command interface + if opt.IncludeCRDs { + flags = append(flags, "--include-crds") + } + flags = st.appendExtraDiffFlags(flags, opt) return flags @@ -2029,6 +2036,7 @@ type DiffOpts struct { // If this is true, Color has no effect. NoColor bool Set []string + IncludeCRDs bool SkipCleanup bool SkipDiffOnInstall bool DiffArgs string