From 7219273d3bc1924339ed6ab777f39a2fef24b930 Mon Sep 17 00:00:00 2001 From: Hubertbits <170125456+hubertbits@users.noreply.github.com> Date: Fri, 11 Apr 2025 23:11:53 +0200 Subject: [PATCH] Fix tests not using New func after introducing common flags Signed-off-by: yxxhero --- pkg/app/app_apply_hooks_test.go | 4 +- pkg/app/app_apply_nokubectx_test.go | 4 +- pkg/app/app_apply_test.go | 4 +- pkg/app/app_diff_test.go | 8 +- pkg/app/app_lint_test.go | 4 +- pkg/app/app_list_test.go | 8 +- pkg/app/app_sync_test.go | 4 +- pkg/app/app_template_test.go | 12 +-- pkg/app/app_test.go | 111 ++++++++++++++++++++++------ pkg/app/diff_nokubectx_test.go | 4 +- pkg/app/diff_test.go | 46 +++++++++++- pkg/helmexec/exec_test.go | 2 +- 12 files changed, 157 insertions(+), 54 deletions(-) diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index 71aff79c..11f76408 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -80,7 +80,7 @@ func TestApply_hooks(t *testing.T) { app.Selectors = tc.selectors } - syncErr := app.Apply(applyConfig{ + syncErr := app.Apply(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, @@ -88,7 +88,7 @@ func TestApply_hooks(t *testing.T) { skipNeeds: tc.fields.skipNeeds, includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, - }) + })) var gotErr string if syncErr != nil { diff --git a/pkg/app/app_apply_nokubectx_test.go b/pkg/app/app_apply_nokubectx_test.go index c5ca3bbe..bb55aac3 100644 --- a/pkg/app/app_apply_nokubectx_test.go +++ b/pkg/app/app_apply_nokubectx_test.go @@ -81,7 +81,7 @@ func TestApply_3(t *testing.T) { app.Selectors = tc.selectors } - syncErr := app.Apply(applyConfig{ + syncErr := app.Apply(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, @@ -89,7 +89,7 @@ func TestApply_3(t *testing.T) { skipNeeds: tc.fields.skipNeeds, includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, - }) + })) var gotErr string if syncErr != nil { diff --git a/pkg/app/app_apply_test.go b/pkg/app/app_apply_test.go index 06caf828..2d8b831a 100644 --- a/pkg/app/app_apply_test.go +++ b/pkg/app/app_apply_test.go @@ -81,7 +81,7 @@ func TestApply_2(t *testing.T) { app.Selectors = tc.selectors } - syncErr := app.Apply(applyConfig{ + syncErr := app.Apply(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, @@ -89,7 +89,7 @@ func TestApply_2(t *testing.T) { skipNeeds: tc.fields.skipNeeds, includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, - }) + })) var gotErr string if syncErr != nil { diff --git a/pkg/app/app_diff_test.go b/pkg/app/app_diff_test.go index ccf499f5..4f34ccb1 100644 --- a/pkg/app/app_diff_test.go +++ b/pkg/app/app_diff_test.go @@ -121,7 +121,7 @@ releases: app.Selectors = tc.selectors } - diffErr := app.Diff(applyConfig{ + diffErr := app.Diff(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, @@ -129,7 +129,7 @@ releases: includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, noHooks: tc.fields.noHooks, - }) + })) var gotErr string if diffErr != nil { @@ -352,12 +352,12 @@ func TestDiffWithInstalled(t *testing.T) { app.Selectors = tc.selectors } - diffErr := app.Diff(applyConfig{ + diffErr := app.Diff(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, skipNeeds: true, - }) + })) var gotErr string if diffErr != nil { diff --git a/pkg/app/app_lint_test.go b/pkg/app/app_lint_test.go index aa328063..82987bf5 100644 --- a/pkg/app/app_lint_test.go +++ b/pkg/app/app_lint_test.go @@ -123,14 +123,14 @@ releases: app.Selectors = tc.selectors } - lintErr := app.Lint(applyConfig{ + lintErr := app.Lint(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, skipNeeds: tc.fields.skipNeeds, includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, - }) + })) var gotErr string if lintErr != nil { diff --git a/pkg/app/app_list_test.go b/pkg/app/app_list_test.go index d7775a83..d8e5b747 100644 --- a/pkg/app/app_list_test.go +++ b/pkg/app/app_list_test.go @@ -223,10 +223,10 @@ database my-app true true chart:postgres,name:da func TestListWithEnvironment(t *testing.T) { t.Run("with skipCharts=false", func(t *testing.T) { - testListWithEnvironment(t, configImpl{skipCharts: false}) + testListWithEnvironment(t, *NewConfigImplWithDefaults(&configImpl{skipCharts: false})) }) t.Run("with skipCharts=true", func(t *testing.T) { - testListWithEnvironment(t, configImpl{skipCharts: true}) + testListWithEnvironment(t, *NewConfigImplWithDefaults(&configImpl{skipCharts: true})) }) } @@ -292,9 +292,9 @@ releases: func TestListWithJSONOutput(t *testing.T) { t.Run("with skipCharts=false", func(t *testing.T) { - testListWithJSONOutput(t, configImpl{skipCharts: false}) + testListWithJSONOutput(t, *NewConfigImplWithDefaults(&configImpl{skipCharts: false})) }) t.Run("with skipCharts=true", func(t *testing.T) { - testListWithJSONOutput(t, configImpl{skipCharts: true}) + testListWithJSONOutput(t, *NewConfigImplWithDefaults(&configImpl{skipCharts: true})) }) } diff --git a/pkg/app/app_sync_test.go b/pkg/app/app_sync_test.go index 5115db1c..d1c23136 100644 --- a/pkg/app/app_sync_test.go +++ b/pkg/app/app_sync_test.go @@ -79,7 +79,7 @@ func TestSync(t *testing.T) { app.Selectors = tc.selectors } - syncErr := app.Sync(applyConfig{ + syncErr := app.Sync(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, @@ -87,7 +87,7 @@ func TestSync(t *testing.T) { skipNeeds: tc.fields.skipNeeds, includeNeeds: tc.fields.includeNeeds, includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, - }) + })) var gotErr string if syncErr != nil { diff --git a/pkg/app/app_template_test.go b/pkg/app/app_template_test.go index e42122e1..29eb8bc5 100644 --- a/pkg/app/app_template_test.go +++ b/pkg/app/app_template_test.go @@ -126,7 +126,7 @@ releases: app.Selectors = tc.selectors } - tmplErr := app.Template(applyConfig{ + tmplErr := app.Template(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, @@ -135,7 +135,7 @@ releases: includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, showOnly: tc.fields.showOnly, noHooks: tc.fields.noHooks, - }) + })) var gotErr string if tmplErr != nil { @@ -394,11 +394,11 @@ releases: app.Namespace = tc.ns } - tmplErr := app.Template(applyConfig{ + tmplErr := app.Template(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, - }) + })) var gotErr string if tmplErr != nil { @@ -499,11 +499,11 @@ releases: app.Namespace = tc.ns } - tmplErr := app.Template(applyConfig{ + tmplErr := app.Template(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: 1, logger: logger, - }) + })) var gotErr string if tmplErr != nil { diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 67e6f8a2..e1e0e82f 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -21,9 +21,11 @@ import ( "go.uber.org/zap" "helm.sh/helm/v3/pkg/chart" + "github.com/helmfile/helmfile/pkg/common" "github.com/helmfile/helmfile/pkg/envvar" "github.com/helmfile/helmfile/pkg/exectest" ffs "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/flags" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/remote" "github.com/helmfile/helmfile/pkg/runtime" @@ -2096,9 +2098,9 @@ type configImpl struct { set []string output string noHooks bool - includeCRDs bool + skipCRDs common.BoolFlag + includeCRDs common.BoolFlag skipCleanup bool - skipCRDs bool skipDeps bool skipTests bool skipSchemaValidation bool @@ -2111,6 +2113,29 @@ type configImpl struct { kubeVersion string } +func NewConfigImpl() *configImpl { + return &configImpl{ + skipCRDs: common.NewBoolFlag(false), + includeCRDs: common.NewBoolFlag(false), + } +} + +// NewConfigImplWithDefaults creates a new configImpl with default values. +// If an existing config is provided, it ensures all fields are initialized. +// If the existing config is nil, a new config is created. +// The existing config is modified in place, so it should not be nil if you want to reuse it. +func NewConfigImplWithDefaults(existing *configImpl) *configImpl { + if existing == nil { + return NewConfigImpl() + } + + // Initialize any nil fields in the existing config + flags.EnsureBoolFlag(&existing.skipCRDs, false) + flags.EnsureBoolFlag(&existing.includeCRDs, false) + + return existing +} + func (c configImpl) Selectors() []string { return c.selectors } @@ -2135,14 +2160,25 @@ func (c configImpl) SkipCleanup() bool { return c.skipCleanup } -func (c configImpl) SkipCRDs() bool { - return c.skipCRDs -} - func (c configImpl) SkipDeps() bool { return c.skipDeps } +func (c configImpl) SkipCRDs() bool { + return c.skipCRDs.Value() +} + +func (c configImpl) IncludeCRDs() bool { + return c.includeCRDs.Value() +} + +func (c configImpl) ShouldIncludeCRDs() bool { + includeCRDsExplicit := c.includeCRDs.WasExplicitlySet() && c.includeCRDs.Value() + skipCRDsExplicit := c.skipCRDs.WasExplicitlySet() && !c.skipCRDs.Value() + + return includeCRDsExplicit || skipCRDsExplicit +} + func (c configImpl) SkipRefresh() bool { return c.skipRefresh } @@ -2171,10 +2207,6 @@ func (c configImpl) OutputDirTemplate() string { return "" } -func (c configImpl) IncludeCRDs() bool { - return c.includeCRDs -} - func (c configImpl) NoHooks() bool { return c.noHooks } @@ -2223,7 +2255,8 @@ type applyConfig struct { set []string validate bool skipCleanup bool - skipCRDs bool + skipCRDs common.BoolFlag + includeCRDs common.BoolFlag skipDeps bool skipRefresh bool skipNeeds bool @@ -2262,8 +2295,33 @@ type applyConfig struct { syncReleaseLabels bool // template-only options - includeCRDs, skipTests bool - outputDir, outputDirTemplate string + skipTests bool + outputDir string + outputDirTemplate string +} + +// New creates a new applyConfig with default values +func NewApplyConfig() *applyConfig { + return &applyConfig{ + skipCRDs: common.NewBoolFlag(false), + includeCRDs: common.NewBoolFlag(false), + } +} + +// NewApplyConfigWithDefaults creates a new applyConfig with default values. +// If an existing config is provided, it ensures all fields are initialized. +// If the existing config is nil, a new config is created. +// The existing config is modified in place, so it should not be nil if you want to reuse it. +func NewApplyConfigWithDefaults(existing *applyConfig) *applyConfig { + if existing == nil { + return NewApplyConfig() + } + + // Initialize any nil fields in the existing config + flags.EnsureBoolFlag(&existing.skipCRDs, false) + flags.EnsureBoolFlag(&existing.includeCRDs, false) + + return existing } func (a applyConfig) Args() string { @@ -2303,7 +2361,18 @@ func (a applyConfig) SkipCleanup() bool { } func (a applyConfig) SkipCRDs() bool { - return a.skipCRDs + return a.skipCRDs.Value() +} + +func (a applyConfig) IncludeCRDs() bool { + return a.includeCRDs.Value() +} + +func (c applyConfig) ShouldIncludeCRDs() bool { + includeCRDsExplicit := c.includeCRDs.WasExplicitlySet() && c.includeCRDs.Value() + skipCRDsExplicit := c.skipCRDs.WasExplicitlySet() && !c.skipCRDs.Value() + + return includeCRDsExplicit || skipCRDsExplicit } func (a applyConfig) SkipDeps() bool { @@ -2399,10 +2468,6 @@ func (a applyConfig) DiffArgs() string { } // helmfile-template-only flags -func (a applyConfig) IncludeCRDs() bool { - return a.includeCRDs -} - func (a applyConfig) SkipTests() bool { return a.skipTests } @@ -2659,7 +2724,7 @@ releases: }, }, files) - if err := app.Template(configImpl{set: []string{"foo=a", "bar=b"}, skipDeps: false}); err != nil { + if err := app.Template(NewConfigImplWithDefaults(&configImpl{set: []string{"foo=a", "bar=b"}, skipDeps: false})); err != nil { t.Fatalf("%v", err) } @@ -2729,7 +2794,7 @@ releases: fmt.Printf("CRAFTED APP WITH %p\n", app.fs.DirectoryExistsAt) - if err := app.Template(configImpl{}); err != nil { + if err := app.Template(NewConfigImpl()); err != nil { t.Fatalf("%v", err) } @@ -3728,13 +3793,13 @@ releases: app.Selectors = tc.selectors } - applyErr := app.Apply(applyConfig{ + applyErr := app.Apply(NewApplyConfigWithDefaults(&applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, skipDiffOnInstall: tc.skipDiffOnInstall, skipNeeds: tc.fields.skipNeeds, - }) + })) switch { case tc.error == "" && applyErr != nil: t.Fatalf("unexpected error for data defined at %s: %v", tc.loc, applyErr) @@ -4006,7 +4071,7 @@ releases: expectNoCallsToHelm(app) out, err := testutil.CaptureStdout(func() { - err := app.ListReleases(configImpl{}) + err := app.ListReleases(NewConfigImpl()) assert.Nil(t, err) }) assert.NoError(t, err) diff --git a/pkg/app/diff_nokubectx_test.go b/pkg/app/diff_nokubectx_test.go index 9b2d36c8..24566b35 100644 --- a/pkg/app/diff_nokubectx_test.go +++ b/pkg/app/diff_nokubectx_test.go @@ -826,14 +826,14 @@ releases: app.Selectors = tc.selectors } - diffErr := app.Diff(diffConfig{ + diffErr := app.Diff(NewDiffConfigWithDefaults(&diffConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, detailedExitcode: tc.detailedExitcode, skipNeeds: tc.flags.skipNeeds, includeNeeds: tc.flags.includeNeeds, - }) + })) var diffErrStr string if diffErr != nil { diff --git a/pkg/app/diff_test.go b/pkg/app/diff_test.go index 4dd63f94..e7438312 100644 --- a/pkg/app/diff_test.go +++ b/pkg/app/diff_test.go @@ -9,8 +9,10 @@ import ( "github.com/stretchr/testify/assert" "go.uber.org/zap" + "github.com/helmfile/helmfile/pkg/common" "github.com/helmfile/helmfile/pkg/exectest" ffs "github.com/helmfile/helmfile/pkg/filesystem" + "github.com/helmfile/helmfile/pkg/flags" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/testhelper" ) @@ -22,7 +24,8 @@ type diffConfig struct { retainValuesFiles bool set []string validate bool - skipCRDs bool + skipCRDs common.BoolFlag + includeCRDs common.BoolFlag skipDeps bool skipRefresh bool includeTests bool @@ -48,6 +51,30 @@ type diffConfig struct { logger *zap.SugaredLogger } +// New creates a new applyConfig with default values +func NewDiffConfig() *diffConfig { + return &diffConfig{ + skipCRDs: common.NewBoolFlag(false), + includeCRDs: common.NewBoolFlag(false), + } +} + +// NewDiffConfigWithDefaults creates a new diffConfig with default values. +// If an existing config is provided, it ensures all fields are initialized. +// If the existing config is nil, a new config is created. +// The existing config is modified in place, so it should not be nil if you want to reuse it. +func NewDiffConfigWithDefaults(existing *diffConfig) *diffConfig { + if existing == nil { + return NewDiffConfig() + } + + // Initialize any nil fields in the existing config + flags.EnsureBoolFlag(&existing.skipCRDs, false) + flags.EnsureBoolFlag(&existing.includeCRDs, false) + + return existing +} + func (a diffConfig) Args() string { return a.args } @@ -69,7 +96,18 @@ func (a diffConfig) Validate() bool { } func (a diffConfig) SkipCRDs() bool { - return a.skipCRDs + return a.skipCRDs.Value() +} + +func (a diffConfig) IncludeCRDs() bool { + return a.includeCRDs.Value() +} + +func (a diffConfig) ShouldIncludeCRDs() bool { + includeCRDsExplicit := a.includeCRDs.WasExplicitlySet() && a.includeCRDs.Value() + skipCRDsExplicit := a.skipCRDs.WasExplicitlySet() && !a.skipCRDs.Value() + + return includeCRDsExplicit || skipCRDsExplicit } func (a diffConfig) SkipDeps() bool { @@ -1181,7 +1219,7 @@ releases: app.Selectors = tc.selectors } - diffErr := app.Diff(diffConfig{ + diffErr := app.Diff(NewDiffConfigWithDefaults(&diffConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. concurrency: tc.concurrency, logger: logger, @@ -1189,7 +1227,7 @@ releases: skipNeeds: tc.flags.skipNeeds, includeNeeds: tc.flags.includeNeeds, noHooks: tc.flags.noHooks, - }) + })) var diffErrStr string if diffErr != nil { diff --git a/pkg/helmexec/exec_test.go b/pkg/helmexec/exec_test.go index b68c3e76..ab9b178f 100644 --- a/pkg/helmexec/exec_test.go +++ b/pkg/helmexec/exec_test.go @@ -13,7 +13,7 @@ import ( "github.com/Masterminds/semver/v3" "github.com/google/go-cmp/cmp" - "github.com/tj/assert" + "github.com/stretchr/testify/assert" "go.uber.org/zap" )