From 43e50bb0c34ed891eccf41ca379bd58c17a73f3e Mon Sep 17 00:00:00 2001 From: Philip Laine Date: Tue, 2 Jul 2024 20:50:55 +0200 Subject: [PATCH] refactor: remove warnings property from packager (#2687) --- src/pkg/packager/common.go | 22 +++----- src/pkg/packager/common_test.go | 95 +++++++++++---------------------- src/pkg/packager/create.go | 7 +-- src/pkg/packager/deploy.go | 27 +++++----- src/pkg/packager/dev.go | 2 +- src/pkg/packager/inspect.go | 2 +- src/pkg/packager/interactive.go | 7 ++- src/pkg/packager/mirror.go | 14 ++--- src/pkg/packager/prepare.go | 7 +-- src/pkg/packager/publish.go | 4 +- src/pkg/packager/remove.go | 6 +-- 11 files changed, 76 insertions(+), 117 deletions(-) diff --git a/src/pkg/packager/common.go b/src/pkg/packager/common.go index 86c55b3240..fb470d9ca6 100644 --- a/src/pkg/packager/common.go +++ b/src/pkg/packager/common.go @@ -35,7 +35,6 @@ type Packager struct { state *types.ZarfState cluster *cluster.Cluster layout *layout.PackagePaths - warnings []string hpaModified bool connectStrings types.ConnectStrings sbomViewFiles []string @@ -235,26 +234,18 @@ func (p *Packager) validatePackageArchitecture(ctx context.Context) error { } // validateLastNonBreakingVersion validates the Zarf CLI version against a package's LastNonBreakingVersion. -func (p *Packager) validateLastNonBreakingVersion() (err error) { - cliVersion := config.CLIVersion - lastNonBreakingVersion := p.cfg.Pkg.Build.LastNonBreakingVersion - +func validateLastNonBreakingVersion(cliVersion, lastNonBreakingVersion string) ([]string, error) { if lastNonBreakingVersion == "" { - return nil + return nil, nil } - lastNonBreakingSemVer, err := semver.NewVersion(lastNonBreakingVersion) if err != nil { - return fmt.Errorf("unable to parse lastNonBreakingVersion '%s' from Zarf package build data : %w", lastNonBreakingVersion, err) + return nil, fmt.Errorf("unable to parse last non breaking version %s from Zarf package build data: %w", lastNonBreakingVersion, err) } - cliSemVer, err := semver.NewVersion(cliVersion) if err != nil { - warning := fmt.Sprintf(lang.CmdPackageDeployInvalidCLIVersionWarn, config.CLIVersion) - p.warnings = append(p.warnings, warning) - return nil + return []string{fmt.Sprintf(lang.CmdPackageDeployInvalidCLIVersionWarn, cliVersion)}, nil } - if cliSemVer.LessThan(lastNonBreakingSemVer) { warning := fmt.Sprintf( lang.CmdPackageDeployValidateLastNonBreakingVersionWarn, @@ -262,8 +253,7 @@ func (p *Packager) validateLastNonBreakingVersion() (err error) { lastNonBreakingVersion, lastNonBreakingVersion, ) - p.warnings = append(p.warnings, warning) + return []string{warning}, nil } - - return nil + return nil, nil } diff --git a/src/pkg/packager/common_test.go b/src/pkg/packager/common_test.go index b298a1d0de..6927082f9b 100644 --- a/src/pkg/packager/common_test.go +++ b/src/pkg/packager/common_test.go @@ -13,7 +13,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" - "github.com/defenseunicorns/zarf/src/config" "github.com/defenseunicorns/zarf/src/config/lang" "github.com/defenseunicorns/zarf/src/pkg/cluster" "github.com/defenseunicorns/zarf/src/types" @@ -120,98 +119,66 @@ func TestValidatePackageArchitecture(t *testing.T) { func TestValidateLastNonBreakingVersion(t *testing.T) { t.Parallel() - type testCase struct { + tests := []struct { name string cliVersion string lastNonBreakingVersion string - expectedErrorMessage string - expectedWarningMessage string - returnError bool - throwWarning bool - } - - testCases := []testCase{ + expectedErr string + expectedWarnings []string + }{ { - name: "CLI version less than lastNonBreakingVersion", + name: "CLI version less than last non breaking version", cliVersion: "v0.26.4", lastNonBreakingVersion: "v0.27.0", - returnError: false, - throwWarning: true, - expectedWarningMessage: fmt.Sprintf( - lang.CmdPackageDeployValidateLastNonBreakingVersionWarn, - "v0.26.4", - "v0.27.0", - "v0.27.0", - ), + expectedWarnings: []string{ + fmt.Sprintf( + lang.CmdPackageDeployValidateLastNonBreakingVersionWarn, + "v0.26.4", + "v0.27.0", + "v0.27.0", + ), + }, }, { - name: "invalid semantic version (CLI version)", + name: "invalid cli version", cliVersion: "invalidSemanticVersion", lastNonBreakingVersion: "v0.0.1", - returnError: false, - throwWarning: true, - expectedWarningMessage: fmt.Sprintf(lang.CmdPackageDeployInvalidCLIVersionWarn, "invalidSemanticVersion"), + expectedWarnings: []string{fmt.Sprintf(lang.CmdPackageDeployInvalidCLIVersionWarn, "invalidSemanticVersion")}, }, { - name: "invalid semantic version (lastNonBreakingVersion)", + name: "invalid last non breaking version", cliVersion: "v0.0.1", lastNonBreakingVersion: "invalidSemanticVersion", - throwWarning: false, - returnError: true, - expectedErrorMessage: "unable to parse lastNonBreakingVersion", + expectedErr: "unable to parse last non breaking version", }, { - name: "CLI version greater than lastNonBreakingVersion", + name: "CLI version greater than last non breaking version", cliVersion: "v0.28.2", lastNonBreakingVersion: "v0.27.0", - returnError: false, - throwWarning: false, }, { - name: "CLI version equal to lastNonBreakingVersion", + name: "CLI version equal to last non breaking version", cliVersion: "v0.27.0", lastNonBreakingVersion: "v0.27.0", - returnError: false, - throwWarning: false, }, { - name: "empty lastNonBreakingVersion", - cliVersion: "this shouldn't get evaluated when the lastNonBreakingVersion is empty", + name: "empty last non breaking version", + cliVersion: "", lastNonBreakingVersion: "", - returnError: false, - throwWarning: false, }, } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() - for _, testCase := range testCases { - testCase := testCase - - t.Run(testCase.name, func(t *testing.T) { - config.CLIVersion = testCase.cliVersion - - p := &Packager{ - cfg: &types.PackagerConfig{ - Pkg: types.ZarfPackage{ - Build: types.ZarfBuildData{ - LastNonBreakingVersion: testCase.lastNonBreakingVersion, - }, - }, - }, - } - - err := p.validateLastNonBreakingVersion() - - switch { - case testCase.returnError: - require.ErrorContains(t, err, testCase.expectedErrorMessage) - require.Empty(t, p.warnings, "Expected no warnings for test case: %s", testCase.name) - case testCase.throwWarning: - require.Contains(t, p.warnings, testCase.expectedWarningMessage) - require.NoError(t, err, "Expected no error for test case: %s", testCase.name) - default: - require.NoError(t, err, "Expected no error for test case: %s", testCase.name) - require.Empty(t, p.warnings, "Expected no warnings for test case: %s", testCase.name) + warnings, err := validateLastNonBreakingVersion(tt.cliVersion, tt.lastNonBreakingVersion) + if tt.expectedErr != "" { + require.ErrorContains(t, err, tt.expectedErr) + require.Empty(t, warnings) + return } + require.NoError(t, err) + require.ElementsMatch(t, tt.expectedWarnings, warnings) }) } } diff --git a/src/pkg/packager/create.go b/src/pkg/packager/create.go index ab46bdaa30..0423208639 100755 --- a/src/pkg/packager/create.go +++ b/src/pkg/packager/create.go @@ -17,7 +17,7 @@ import ( ) // Create generates a Zarf package tarball for a given PackageConfig and optional base directory. -func (p *Packager) Create(ctx context.Context) (err error) { +func (p *Packager) Create(ctx context.Context) error { cwd, err := os.Getwd() if err != nil { return err @@ -35,12 +35,13 @@ func (p *Packager) Create(ctx context.Context) (err error) { return err } - p.cfg.Pkg, p.warnings, err = pc.LoadPackageDefinition(ctx, p.layout) + pkg, warnings, err := pc.LoadPackageDefinition(ctx, p.layout) if err != nil { return err } + p.cfg.Pkg = pkg - if !p.confirmAction(config.ZarfCreateStage) { + if !p.confirmAction(config.ZarfCreateStage, warnings) { return fmt.Errorf("package creation canceled") } diff --git a/src/pkg/packager/deploy.go b/src/pkg/packager/deploy.go index 716b672c75..094e7cfef7 100644 --- a/src/pkg/packager/deploy.go +++ b/src/pkg/packager/deploy.go @@ -47,8 +47,7 @@ func (p *Packager) resetRegistryHPA(ctx context.Context) { } // Deploy attempts to deploy the given PackageConfig. -func (p *Packager) Deploy(ctx context.Context) (err error) { - +func (p *Packager) Deploy(ctx context.Context) error { isInteractive := !config.CommonOptions.Confirm deployFilter := filters.Combine( @@ -56,38 +55,42 @@ func (p *Packager) Deploy(ctx context.Context) (err error) { filters.ForDeploy(p.cfg.PkgOpts.OptionalComponents, isInteractive), ) + warnings := []string{} if isInteractive { filter := filters.Empty() - - p.cfg.Pkg, p.warnings, err = p.source.LoadPackage(ctx, p.layout, filter, true) + pkg, loadWarnings, err := p.source.LoadPackage(ctx, p.layout, filter, true) if err != nil { return fmt.Errorf("unable to load the package: %w", err) } + p.cfg.Pkg = pkg + warnings = append(warnings, loadWarnings...) } else { - p.cfg.Pkg, p.warnings, err = p.source.LoadPackage(ctx, p.layout, deployFilter, true) + pkg, loadWarnings, err := p.source.LoadPackage(ctx, p.layout, deployFilter, true) if err != nil { return fmt.Errorf("unable to load the package: %w", err) } - + p.cfg.Pkg = pkg + warnings = append(warnings, loadWarnings...) if err := p.populatePackageVariableConfig(); err != nil { return fmt.Errorf("unable to set the active variables: %w", err) } } - if err := p.validateLastNonBreakingVersion(); err != nil { + validateWarnings, err := validateLastNonBreakingVersion(config.CLIVersion, p.cfg.Pkg.Build.LastNonBreakingVersion) + if err != nil { return err } + warnings = append(warnings, validateWarnings...) - var sbomWarnings []string - p.sbomViewFiles, sbomWarnings, err = p.layout.SBOMs.StageSBOMViewFiles() + sbomViewFiles, sbomWarnings, err := p.layout.SBOMs.StageSBOMViewFiles() if err != nil { return err } - - p.warnings = append(p.warnings, sbomWarnings...) + p.sbomViewFiles = sbomViewFiles + warnings = append(warnings, sbomWarnings...) // Confirm the overall package deployment - if !p.confirmAction(config.ZarfDeployStage) { + if !p.confirmAction(config.ZarfDeployStage, warnings) { return fmt.Errorf("deployment cancelled") } diff --git a/src/pkg/packager/dev.go b/src/pkg/packager/dev.go index 5332d10c10..138d175109 100644 --- a/src/pkg/packager/dev.go +++ b/src/pkg/packager/dev.go @@ -44,7 +44,7 @@ func (p *Packager) DevDeploy(ctx context.Context) error { return err } - p.cfg.Pkg, p.warnings, err = pc.LoadPackageDefinition(ctx, p.layout) + p.cfg.Pkg, _, err = pc.LoadPackageDefinition(ctx, p.layout) if err != nil { return err } diff --git a/src/pkg/packager/inspect.go b/src/pkg/packager/inspect.go index d7e4be3e18..d68ae5f5de 100644 --- a/src/pkg/packager/inspect.go +++ b/src/pkg/packager/inspect.go @@ -18,7 +18,7 @@ import ( func (p *Packager) Inspect(ctx context.Context) (err error) { wantSBOM := p.cfg.InspectOpts.ViewSBOM || p.cfg.InspectOpts.SBOMOutputDir != "" - p.cfg.Pkg, p.warnings, err = p.source.LoadPackageMetadata(ctx, p.layout, wantSBOM, true) + p.cfg.Pkg, _, err = p.source.LoadPackageMetadata(ctx, p.layout, wantSBOM, true) if err != nil { return err } diff --git a/src/pkg/packager/interactive.go b/src/pkg/packager/interactive.go index 2326a4910a..d481b06d63 100644 --- a/src/pkg/packager/interactive.go +++ b/src/pkg/packager/interactive.go @@ -18,8 +18,7 @@ import ( "github.com/pterm/pterm" ) -func (p *Packager) confirmAction(stage string) (confirm bool) { - +func (p *Packager) confirmAction(stage string, warnings []string) (confirm bool) { pterm.Println() message.HeaderInfof("📦 PACKAGE DEFINITION") utils.ColorPrintYAML(p.cfg.Pkg, p.getPackageYAMLHints(stage), true) @@ -54,10 +53,10 @@ func (p *Packager) confirmAction(stage string) (confirm bool) { } } - if len(p.warnings) > 0 { + if len(warnings) > 0 { message.HorizontalRule() message.Title("Package Warnings", "the following warnings were flagged while reading the package") - for _, warning := range p.warnings { + for _, warning := range warnings { message.Warn(warning) } } diff --git a/src/pkg/packager/mirror.go b/src/pkg/packager/mirror.go index f19536443c..3e1782755f 100644 --- a/src/pkg/packager/mirror.go +++ b/src/pkg/packager/mirror.go @@ -17,27 +17,27 @@ import ( ) // Mirror pulls resources from a package (images, git repositories, etc) and pushes them to remotes in the air gap without deploying them -func (p *Packager) Mirror(ctx context.Context) (err error) { +func (p *Packager) Mirror(ctx context.Context) error { filter := filters.Combine( filters.ByLocalOS(runtime.GOOS), filters.BySelectState(p.cfg.PkgOpts.OptionalComponents), ) - p.cfg.Pkg, p.warnings, err = p.source.LoadPackage(ctx, p.layout, filter, true) + pkg, warnings, err := p.source.LoadPackage(ctx, p.layout, filter, true) if err != nil { return fmt.Errorf("unable to load the package: %w", err) } + p.cfg.Pkg = pkg - var sbomWarnings []string - p.sbomViewFiles, sbomWarnings, err = p.layout.SBOMs.StageSBOMViewFiles() + sbomViewFiles, sbomWarnings, err := p.layout.SBOMs.StageSBOMViewFiles() if err != nil { return err } - - p.warnings = append(p.warnings, sbomWarnings...) + p.sbomViewFiles = sbomViewFiles + warnings = append(warnings, sbomWarnings...) // Confirm the overall package mirror - if !p.confirmAction(config.ZarfMirrorStage) { + if !p.confirmAction(config.ZarfMirrorStage, warnings) { return fmt.Errorf("mirror cancelled") } diff --git a/src/pkg/packager/prepare.go b/src/pkg/packager/prepare.go index 4bd9c88e54..9a4f7a1280 100644 --- a/src/pkg/packager/prepare.go +++ b/src/pkg/packager/prepare.go @@ -38,7 +38,7 @@ import ( type imageMap map[string]bool // FindImages iterates over a Zarf.yaml and attempts to parse any images. -func (p *Packager) FindImages(ctx context.Context) (imgMap map[string][]string, err error) { +func (p *Packager) FindImages(ctx context.Context) (map[string][]string, error) { cwd, err := os.Getwd() if err != nil { return nil, err @@ -60,12 +60,13 @@ func (p *Packager) FindImages(ctx context.Context) (imgMap map[string][]string, return nil, err } - p.cfg.Pkg, p.warnings, err = c.LoadPackageDefinition(ctx, p.layout) + pkg, warnings, err := c.LoadPackageDefinition(ctx, p.layout) if err != nil { return nil, err } + p.cfg.Pkg = pkg - for _, warning := range p.warnings { + for _, warning := range warnings { message.Warn(warning) } diff --git a/src/pkg/packager/publish.go b/src/pkg/packager/publish.go index e889ea89b6..6f5e90843c 100644 --- a/src/pkg/packager/publish.go +++ b/src/pkg/packager/publish.go @@ -58,7 +58,7 @@ func (p *Packager) Publish(ctx context.Context) (err error) { return err } - p.cfg.Pkg, p.warnings, err = sc.LoadPackageDefinition(ctx, p.layout) + p.cfg.Pkg, _, err = sc.LoadPackageDefinition(ctx, p.layout) if err != nil { return err } @@ -72,7 +72,7 @@ func (p *Packager) Publish(ctx context.Context) (err error) { } } else { filter := filters.Empty() - p.cfg.Pkg, p.warnings, err = p.source.LoadPackage(ctx, p.layout, filter, false) + p.cfg.Pkg, _, err = p.source.LoadPackage(ctx, p.layout, filter, false) if err != nil { return fmt.Errorf("unable to load the package: %w", err) } diff --git a/src/pkg/packager/remove.go b/src/pkg/packager/remove.go index 091f446ae2..be34b6dda7 100644 --- a/src/pkg/packager/remove.go +++ b/src/pkg/packager/remove.go @@ -37,15 +37,13 @@ func (p *Packager) Remove(ctx context.Context) (err error) { spinner := message.NewProgressSpinner("Removing Zarf package %s", p.cfg.PkgOpts.PackageSource) defer spinner.Stop() - var packageName string - // we do not want to allow removal of signed packages without a signature if there are remove actions // as this is arbitrary code execution from an untrusted source - p.cfg.Pkg, p.warnings, err = p.source.LoadPackageMetadata(ctx, p.layout, false, false) + p.cfg.Pkg, _, err = p.source.LoadPackageMetadata(ctx, p.layout, false, false) if err != nil { return err } - packageName = p.cfg.Pkg.Metadata.Name + packageName := p.cfg.Pkg.Metadata.Name // Build a list of components to remove and determine if we need a cluster connection componentsToRemove := []string{}