From 26d11bf13a543f6a70b342761f916658f4a7fb96 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 10:07:48 -0700 Subject: [PATCH 01/16] fix: remove unnecessary named returns in pkg/variables Signed-off-by: Kit Patella --- src/pkg/variables/variables.go | 4 ++-- src/pkg/variables/variables_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pkg/variables/variables.go b/src/pkg/variables/variables.go index 929cb13182..353040eab4 100644 --- a/src/pkg/variables/variables.go +++ b/src/pkg/variables/variables.go @@ -15,8 +15,8 @@ import ( type SetVariableMap map[string]*v1alpha1.SetVariable // GetSetVariable gets a variable set within a VariableConfig by its name -func (vc *VariableConfig) GetSetVariable(name string) (variable *v1alpha1.SetVariable, ok bool) { - variable, ok = vc.setVariableMap[name] +func (vc *VariableConfig) GetSetVariable(name string) (*v1alpha1.SetVariable, bool) { + variable, ok := vc.setVariableMap[name] return variable, ok } diff --git a/src/pkg/variables/variables_test.go b/src/pkg/variables/variables_test.go index 07442e97f5..f0aeea78c5 100644 --- a/src/pkg/variables/variables_test.go +++ b/src/pkg/variables/variables_test.go @@ -20,7 +20,7 @@ func TestPopulateVariables(t *testing.T) { wantVars SetVariableMap } - prompt := func(_ v1alpha1.InteractiveVariable) (value string, err error) { return "Prompt", nil } + prompt := func(_ v1alpha1.InteractiveVariable) (string, error) { return "Prompt", nil } tests := []test{ { From 3fff246dfcc8c38e9874ab8fa920e6bf510e2009 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 10:46:03 -0700 Subject: [PATCH 02/16] fix: remove unused named return and delay declaration until its used Signed-off-by: Kit Patella --- src/pkg/interactive/components.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pkg/interactive/components.go b/src/pkg/interactive/components.go index 719228cb5c..28ff933b79 100644 --- a/src/pkg/interactive/components.go +++ b/src/pkg/interactive/components.go @@ -15,7 +15,7 @@ import ( ) // SelectOptionalComponent prompts to confirm optional components -func SelectOptionalComponent(component v1alpha1.ZarfComponent) (confirm bool, err error) { +func SelectOptionalComponent(component v1alpha1.ZarfComponent) (bool, error) { message.HorizontalRule() displayComponent := component @@ -30,6 +30,8 @@ func SelectOptionalComponent(component v1alpha1.ZarfComponent) (confirm bool, er Default: component.Default, } + // REVIEW(mkcp): Can confirm be true here? It's not clear to me if survey.AskOne will ever flip a boolean + var confirm bool return confirm, survey.AskOne(prompt, &confirm) } From 005bc443f69d44088ac11c971a4a552413b44005 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 11:09:03 -0700 Subject: [PATCH 03/16] fix: pkg/transform/image.go remove shadowed named returns and pass back explicit empty struct until needed Signed-off-by: Kit Patella --- src/pkg/transform/image.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/pkg/transform/image.go b/src/pkg/transform/image.go index ca6fcdc820..c12bb1d232 100644 --- a/src/pkg/transform/image.go +++ b/src/pkg/transform/image.go @@ -62,32 +62,36 @@ func ImageTransformHostWithoutChecksum(targetHost, srcReference string) (string, } // ParseImageRef parses a source reference into an Image struct -func ParseImageRef(srcReference string) (out Image, err error) { +func ParseImageRef(srcReference string) (Image, error) { srcReference = strings.TrimPrefix(srcReference, helpers.OCIURLPrefix) ref, err := reference.ParseAnyReference(srcReference) if err != nil { - return out, err + return Image{}, err } // Parse the reference into its components - if named, ok := ref.(reference.Named); ok { - out.Name = named.Name() - out.Path = reference.Path(named) - out.Host = reference.Domain(named) - out.Reference = ref.String() - } else { - return out, fmt.Errorf("unable to parse image name from %s", srcReference) + named, ok := ref.(reference.Named) + if !ok { + return Image{}, fmt.Errorf("unable to parse image name from %s", srcReference) } + out := Image{ + Name: named.Name(), + Path: reference.Path(named), + Host: reference.Domain(named), + Reference: ref.String(), + } + + // TODO(mkcp): This rewriting tag and digest code could probably be consolidated with types // Parse the tag and add it to digestOrReference - if tagged, ok := ref.(reference.Tagged); ok { + if tagged, tagOK := ref.(reference.Tagged); tagOK { out.Tag = tagged.Tag() out.TagOrDigest = fmt.Sprintf(":%s", tagged.Tag()) } // Parse the digest and override digestOrReference - if digested, ok := ref.(reference.Digested); ok { + if digested, digOK := ref.(reference.Digested); digOK { out.Digest = digested.Digest().String() out.TagOrDigest = fmt.Sprintf("@%s", digested.Digest().String()) } From ce2c530c715340813783d84f2f2559c5c59b892b Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 11:27:09 -0700 Subject: [PATCH 04/16] fix: pkg/interactive/prompt remove unused named returns and make AskOne error handling explicit Signed-off-by: Kit Patella --- src/pkg/interactive/prompt.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/pkg/interactive/prompt.go b/src/pkg/interactive/prompt.go index 5af5f9c451..b6b6e69c94 100644 --- a/src/pkg/interactive/prompt.go +++ b/src/pkg/interactive/prompt.go @@ -19,11 +19,15 @@ func PromptSigPassword() ([]byte, error) { prompt := &survey.Password{ Message: "Private key password (empty for no password): ", } - return []byte(password), survey.AskOne(prompt, &password) + err := survey.AskOne(prompt, &password) + if err != nil { + return []byte{}, err + } + return []byte(password), nil } // PromptVariable prompts the user for a value for a variable -func PromptVariable(variable v1alpha1.InteractiveVariable) (value string, err error) { +func PromptVariable(variable v1alpha1.InteractiveVariable) (string, error) { if variable.Description != "" { message.Question(variable.Description) } @@ -33,5 +37,10 @@ func PromptVariable(variable v1alpha1.InteractiveVariable) (value string, err er Default: variable.Default, } - return value, survey.AskOne(prompt, &value) + var value string + err := survey.AskOne(prompt, &value) + if err != nil { + return "", err + } + return value, nil } From 87cf57eebb9ba829970e1ea7b3946441674975d3 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 12:03:05 -0700 Subject: [PATCH 05/16] chore,pkg/utils: remove named return, refactor, add tests for ByteFormat Signed-off-by: Kit Patella --- src/pkg/utils/bytes.go | 70 +++++++++++++++++++++++---------- src/pkg/utils/bytes_test.go | 78 +++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 21 deletions(-) create mode 100644 src/pkg/utils/bytes_test.go diff --git a/src/pkg/utils/bytes.go b/src/pkg/utils/bytes.go index 7dd159b91f..6c0bed95b6 100644 --- a/src/pkg/utils/bytes.go +++ b/src/pkg/utils/bytes.go @@ -16,45 +16,73 @@ import ( "github.com/zarf-dev/zarf/src/pkg/message" ) +type Unit struct { + name string + size float64 +} + +var ( + gigabyte = Unit{ + name: "GB", + size: 1000000000, + } + megabyte = Unit{ + name: "MB", + size: 1000000, + } + kilobyte = Unit{ + name: "KB", + size: 1000, + } + unitByte = Unit{ + name: "Byte", + } +) + // RoundUp rounds a float64 to the given number of decimal places. -func RoundUp(input float64, places int) (newVal float64) { - var round float64 +func RoundUp(input float64, places int) float64 { pow := math.Pow(10, float64(places)) digit := pow * input - round = math.Ceil(digit) - newVal = round / pow - return + round := math.Ceil(digit) + return round / pow } // ByteFormat formats a number of bytes into a human readable string. -func ByteFormat(inputNum float64, precision int) string { +func ByteFormat(in float64, precision int) string { if precision <= 0 { precision = 1 } var unit string - var returnVal float64 + var val float64 // https://www.techtarget.com/searchstorage/definition/mebibyte-MiB - if inputNum >= 1000000000 { - returnVal = RoundUp(inputNum/1000000000, precision) - unit = " GB" // gigabyte - } else if inputNum >= 1000000 { - returnVal = RoundUp(inputNum/1000000, precision) - unit = " MB" // megabyte - } else if inputNum >= 1000 { - returnVal = RoundUp(inputNum/1000, precision) - unit = " KB" // kilobyte - } else { - returnVal = inputNum - unit = " Byte" // byte + switch { + case gigabyte.size <= in: + val = RoundUp(in/gigabyte.size, precision) + unit = gigabyte.name + break + case 1000000 <= in: + val = RoundUp(in/1000000, precision) + unit = megabyte.name + break + case 1000 <= in: + val = RoundUp(in/1000, precision) + unit = kilobyte.name + break + default: + val = in + unit = unitByte.name + break } - if returnVal > 1 { + // NOTE(mkcp): Negative bytes are nonsense, but it's more robust for inputs without erroring. + if val < -1 || 1 < val { unit += "s" } - return strconv.FormatFloat(returnVal, 'f', precision, 64) + unit + vFmt := strconv.FormatFloat(val, 'f', precision, 64) + return vFmt + " " + unit } // RenderProgressBarForLocalDirWrite creates a progress bar that continuously tracks the progress of writing files to a local directory and all of its subdirectories. diff --git a/src/pkg/utils/bytes_test.go b/src/pkg/utils/bytes_test.go new file mode 100644 index 0000000000..55c30b7643 --- /dev/null +++ b/src/pkg/utils/bytes_test.go @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2024-Present The Zarf Authors + +// Package utils provides generic utility functions. +package utils + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestByteFormat(t *testing.T) { + t.Parallel() + tt := []struct { + name string + in float64 + precision int + expect string + }{ + { + name: "accepts empty", + expect: "0.0 Byte", + }, + { + name: "accepts empty bytes with precision", + precision: 1, + expect: "0.0 Byte", + }, + { + name: "accepts empty bytes with meaningful precision", + precision: 3, + expect: "0.000 Byte", + }, + { + name: "formats negative byte with empty precision", + in: -1, + expect: "-1.0 Byte", + }, + { + name: "formats negative bytes with empty precision", + in: -2, + expect: "-2.0 Bytes", + }, + { + name: "formats kilobyte", + in: 1000, + expect: "1.0 KB", + }, + { + name: "formats kilobytes", + in: 1100, + expect: "1.1 KBs", + }, + { + name: "formats megabytes", + in: 10000000, + expect: "10.0 MBs", + }, + { + name: "formats gigabytes", + in: 100000000000, + expect: "100.0 GBs", + }, + { + name: "formats arbitrary in", + in: 4238970784923, + precision: 99, + expect: "4238.970784922999882837757468223571777343750000000000000000000000000000000000000000000000000000000000000 GBs", + }, + } + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + actual := ByteFormat(tc.in, tc.precision) + require.Equal(t, tc.expect, actual) + }) + } +} From 8cc968d25bd4f12f7b7dec2d99668ede08f7c30c Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 12:07:22 -0700 Subject: [PATCH 06/16] chore: make interactive/SelectOptionalComponent error handling explicit Signed-off-by: Kit Patella --- src/pkg/interactive/components.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/pkg/interactive/components.go b/src/pkg/interactive/components.go index 28ff933b79..b742aeed4c 100644 --- a/src/pkg/interactive/components.go +++ b/src/pkg/interactive/components.go @@ -30,9 +30,12 @@ func SelectOptionalComponent(component v1alpha1.ZarfComponent) (bool, error) { Default: component.Default, } - // REVIEW(mkcp): Can confirm be true here? It's not clear to me if survey.AskOne will ever flip a boolean var confirm bool - return confirm, survey.AskOne(prompt, &confirm) + err := survey.AskOne(prompt, &confirm) + if err != nil { + return false, err + } + return confirm, nil } // SelectChoiceGroup prompts to select component groups From f8e373ed6b29b09dd4e21257055998ac40b47061 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 12:18:07 -0700 Subject: [PATCH 07/16] chore: finish refactor of utils/ByteFormat and make linter happy Signed-off-by: Kit Patella --- src/pkg/utils/bytes.go | 50 +++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/src/pkg/utils/bytes.go b/src/pkg/utils/bytes.go index 6c0bed95b6..b091521381 100644 --- a/src/pkg/utils/bytes.go +++ b/src/pkg/utils/bytes.go @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: 2021-Present The Zarf Authors +// SPDX-FileCopyrightText: 2024-Present The Zarf Authors // forked from https://www.socketloop.com/tutorials/golang-byte-format-example @@ -16,25 +16,25 @@ import ( "github.com/zarf-dev/zarf/src/pkg/message" ) -type Unit struct { +type unit struct { name string size float64 } var ( - gigabyte = Unit{ + gigabyte = unit{ name: "GB", size: 1000000000, } - megabyte = Unit{ + megabyte = unit{ name: "MB", size: 1000000, } - kilobyte = Unit{ + kilobyte = unit{ name: "KB", size: 1000, } - unitByte = Unit{ + unitByte = unit{ name: "Byte", } ) @@ -47,42 +47,38 @@ func RoundUp(input float64, places int) float64 { return round / pow } -// ByteFormat formats a number of bytes into a human readable string. +// ByteFormat formats a number of bytes into a human-readable string. func ByteFormat(in float64, precision int) string { if precision <= 0 { precision = 1 } - var unit string - var val float64 + var v float64 + var u string // https://www.techtarget.com/searchstorage/definition/mebibyte-MiB switch { case gigabyte.size <= in: - val = RoundUp(in/gigabyte.size, precision) - unit = gigabyte.name - break - case 1000000 <= in: - val = RoundUp(in/1000000, precision) - unit = megabyte.name - break - case 1000 <= in: - val = RoundUp(in/1000, precision) - unit = kilobyte.name - break + v = RoundUp(in/gigabyte.size, precision) + u = gigabyte.name + case megabyte.size <= in: + v = RoundUp(in/megabyte.size, precision) + u = megabyte.name + case kilobyte.size <= in: + v = RoundUp(in/kilobyte.size, precision) + u = kilobyte.name default: - val = in - unit = unitByte.name - break + v = in + u = unitByte.name } // NOTE(mkcp): Negative bytes are nonsense, but it's more robust for inputs without erroring. - if val < -1 || 1 < val { - unit += "s" + if v < -1 || 1 < v { + u += "s" } - vFmt := strconv.FormatFloat(val, 'f', precision, 64) - return vFmt + " " + unit + vFmt := strconv.FormatFloat(v, 'f', precision, 64) + return vFmt + " " + u } // RenderProgressBarForLocalDirWrite creates a progress bar that continuously tracks the progress of writing files to a local directory and all of its subdirectories. From bc93e127bfb4ecb67ac8e85742a465de792b1f18 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 12:20:35 -0700 Subject: [PATCH 08/16] chore: pkg/utils ok now the linter should be happy with the copyright date Signed-off-by: Kit Patella --- src/pkg/utils/bytes.go | 2 +- src/pkg/utils/bytes_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pkg/utils/bytes.go b/src/pkg/utils/bytes.go index b091521381..22b3322614 100644 --- a/src/pkg/utils/bytes.go +++ b/src/pkg/utils/bytes.go @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: 2024-Present The Zarf Authors +// SPDX-FileCopyrightText: 2021-Present The Zarf Authors // forked from https://www.socketloop.com/tutorials/golang-byte-format-example diff --git a/src/pkg/utils/bytes_test.go b/src/pkg/utils/bytes_test.go index 55c30b7643..492048f788 100644 --- a/src/pkg/utils/bytes_test.go +++ b/src/pkg/utils/bytes_test.go @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: 2024-Present The Zarf Authors +// SPDX-FileCopyrightText: 2021-Present The Zarf Authors // Package utils provides generic utility functions. package utils From 0e156bad8d6d35e7264df424059e3ad7e56cdf71 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 13:25:24 -0700 Subject: [PATCH 09/16] refactor(pkg/utils/cosign): extract constants, no partial results on error, and clean up fn signatures Signed-off-by: Kit Patella --- src/pkg/utils/cosign.go | 75 ++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/src/pkg/utils/cosign.go b/src/pkg/utils/cosign.go index 21bfc282bd..f81183741a 100644 --- a/src/pkg/utils/cosign.go +++ b/src/pkg/utils/cosign.go @@ -33,6 +33,12 @@ import ( "github.com/zarf-dev/zarf/src/pkg/message" ) +const ( + cosignB64Enabled = true + cosignOutputCertificate = "" + cosignTLogUpload = false +) + // Sget performs a cosign signature verification on a given image using the specified public key. // // Forked from https://github.com/sigstore/cosign/blob/v1.7.1/pkg/sget/sget.go @@ -171,7 +177,7 @@ func Sget(ctx context.Context, image, key string, out io.Writer) error { } // CosignVerifyBlob verifies the zarf.yaml.sig was signed with the key provided by the flag -func CosignVerifyBlob(ctx context.Context, blobRef string, sigRef string, keyPath string) error { +func CosignVerifyBlob(ctx context.Context, blobRef, sigRef, keyPath string) error { keyOptions := options.KeyOpts{KeyRef: keyPath} cmd := &verify.VerifyBlobCmd{ KeyOpts: keyOptions, @@ -181,74 +187,83 @@ func CosignVerifyBlob(ctx context.Context, blobRef string, sigRef string, keyPat IgnoreTlog: true, } err := cmd.Exec(ctx, blobRef) - if err == nil { - message.Successf("Package signature validated!") + if err != nil { + return err } - return err + message.Successf("Package signature validated!") + return nil } // CosignSignBlob signs the provide binary and returns the signature -func CosignSignBlob(blobPath string, outputSigPath string, keyPath string, passwordFunc func(bool) ([]byte, error)) ([]byte, error) { - rootOptions := &options.RootOptions{Verbose: false, Timeout: options.DefaultTimeout} +func CosignSignBlob(blobPath, outputSigPath, keyPath string, passFn cosign.PassFunc) ([]byte, error) { + rootOptions := &options.RootOptions{ + Verbose: false, + Timeout: options.DefaultTimeout, + } - keyOptions := options.KeyOpts{KeyRef: keyPath, - PassFunc: passwordFunc} - b64 := true - outputCertificate := "" - tlogUpload := false + keyOptions := options.KeyOpts{ + KeyRef: keyPath, + PassFunc: passFn, + } - sig, err := sign.SignBlobCmd(rootOptions, + sig, err := sign.SignBlobCmd( + rootOptions, keyOptions, blobPath, - b64, + cosignB64Enabled, outputSigPath, - outputCertificate, - tlogUpload) + cosignOutputCertificate, + cosignTLogUpload) + if err != nil { + return []byte{}, err + } - return sig, err + return sig, nil } // GetCosignArtifacts returns signatures and attestations for the given image -func GetCosignArtifacts(image string) (cosignList []string, err error) { - var cosignArtifactList []string +func GetCosignArtifacts(image string) ([]string, error) { var nameOpts []name.Option - ref, err := name.ParseReference(image, nameOpts...) + ref, err := name.ParseReference(image, nameOpts...) if err != nil { - return cosignArtifactList, err + return []string{}, err } var remoteOpts []ociremote.Option simg, _ := ociremote.SignedEntity(ref, remoteOpts...) if simg == nil { - return cosignArtifactList, nil + return []string{}, nil } + // Errors are dogsled because these functions always return a name.Tag which we can check for layers sigRef, _ := ociremote.SignatureTag(ref, remoteOpts...) attRef, _ := ociremote.AttestationTag(ref, remoteOpts...) - sigs, err := simg.Signatures() + ss, err := simg.Signatures() if err != nil { - return cosignArtifactList, err + return []string{}, err } - layers, err := sigs.Layers() + ssLayers, err := ss.Layers() if err != nil { - return cosignArtifactList, err + return []string{}, err } - if len(layers) > 0 { + + var cosignArtifactList = make([]string, 0) + if 0 < len(ssLayers) { cosignArtifactList = append(cosignArtifactList, sigRef.String()) } atts, err := simg.Attestations() if err != nil { - return cosignArtifactList, err + return []string{}, err } - layers, err = atts.Layers() + aLayers, err := atts.Layers() if err != nil { - return cosignArtifactList, err + return []string{}, err } - if len(layers) > 0 { + if 0 < len(aLayers) { cosignArtifactList = append(cosignArtifactList, attRef.String()) } return cosignArtifactList, nil From fe518f37bd7e37f7fb55fce5969fadedabd02bbd Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 14:02:06 -0700 Subject: [PATCH 10/16] chore(pkg/utils): remove named return in network.go and trim unreachable err Signed-off-by: Kit Patella --- src/pkg/utils/network.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/pkg/utils/network.go b/src/pkg/utils/network.go index be0b80a2ed..ffe5490600 100644 --- a/src/pkg/utils/network.go +++ b/src/pkg/utils/network.go @@ -39,7 +39,7 @@ func parseChecksum(src string) (string, string, error) { } // DownloadToFile downloads a given URL to the target filepath (including the cosign key if necessary). -func DownloadToFile(ctx context.Context, src string, dst string, cosignKeyPath string) (err error) { +func DownloadToFile(ctx context.Context, src, dst, cosignKeyPath string) error { // check if the parsed URL has a checksum // if so, remove it and use the checksum to validate the file src, checksum, err := parseChecksum(src) @@ -69,9 +69,6 @@ func DownloadToFile(ctx context.Context, src string, dst string, cosignKeyPath s if err != nil { return fmt.Errorf("unable to download file with sget: %s: %w", src, err) } - if err != nil { - return err - } } else { err = httpGetFile(src, file) if err != nil { @@ -80,7 +77,7 @@ func DownloadToFile(ctx context.Context, src string, dst string, cosignKeyPath s } // If the file has a checksum, validate it - if len(checksum) > 0 { + if 0 < len(checksum) { received, err := helpers.GetSHA256OfFile(dst) if err != nil { return err From c18256a4cf65f3e754926a4ecc6acc67d8d9ef6b Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 15:17:56 -0700 Subject: [PATCH 11/16] chore(pkg/zoci): remove named returns Signed-off-by: Kit Patella --- src/pkg/zoci/fetch.go | 18 +++++++++++++----- src/pkg/zoci/pull.go | 7 +++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/pkg/zoci/fetch.go b/src/pkg/zoci/fetch.go index 923e3d7c24..ca46d8e996 100644 --- a/src/pkg/zoci/fetch.go +++ b/src/pkg/zoci/fetch.go @@ -14,19 +14,27 @@ import ( ) // FetchZarfYAML fetches the zarf.yaml file from the remote repository. -func (r *Remote) FetchZarfYAML(ctx context.Context) (pkg v1alpha1.ZarfPackage, err error) { +func (r *Remote) FetchZarfYAML(ctx context.Context) (v1alpha1.ZarfPackage, error) { manifest, err := r.FetchRoot(ctx) if err != nil { - return pkg, err + return v1alpha1.ZarfPackage{}, err } - return oci.FetchYAMLFile[v1alpha1.ZarfPackage](ctx, r.FetchLayer, manifest, layout.ZarfYAML) + result, err := oci.FetchYAMLFile[v1alpha1.ZarfPackage](ctx, r.FetchLayer, manifest, layout.ZarfYAML) + if err != nil { + return v1alpha1.ZarfPackage{}, err + } + return result, nil } // FetchImagesIndex fetches the images/index.json file from the remote repository. -func (r *Remote) FetchImagesIndex(ctx context.Context) (index *ocispec.Index, err error) { +func (r *Remote) FetchImagesIndex(ctx context.Context) (*ocispec.Index, error) { manifest, err := r.FetchRoot(ctx) if err != nil { return nil, err } - return oci.FetchJSONFile[*ocispec.Index](ctx, r.FetchLayer, manifest, layout.IndexPath) + result, err := oci.FetchJSONFile[*ocispec.Index](ctx, r.FetchLayer, manifest, layout.IndexPath) + if err != nil { + return nil, err + } + return result, nil } diff --git a/src/pkg/zoci/pull.go b/src/pkg/zoci/pull.go index 9fd76e9ccc..44c16b5646 100644 --- a/src/pkg/zoci/pull.go +++ b/src/pkg/zoci/pull.go @@ -76,7 +76,9 @@ func (r *Remote) PullPackage(ctx context.Context, destinationDir string, concurr // LayersFromRequestedComponents returns the descriptors for the given components from the root manifest. // // It also retrieves the descriptors for all image layers that are required by the components. -func (r *Remote) LayersFromRequestedComponents(ctx context.Context, requestedComponents []v1alpha1.ZarfComponent) (layers []ocispec.Descriptor, err error) { +func (r *Remote) LayersFromRequestedComponents(ctx context.Context, requestedComponents []v1alpha1.ZarfComponent) ([]ocispec.Descriptor, error) { + layers := make([]ocispec.Descriptor, 0) + root, err := r.FetchRoot(ctx) if err != nil { return nil, err @@ -98,7 +100,8 @@ func (r *Remote) LayersFromRequestedComponents(ctx context.Context, requestedCom for _, image := range component.Images { images[image] = true } - layers = append(layers, root.Locate(filepath.Join(layout.ComponentsDir, fmt.Sprintf(tarballFormat, component.Name)))) + desc := root.Locate(filepath.Join(layout.ComponentsDir, fmt.Sprintf(tarballFormat, component.Name))) + layers = append(layers, desc) } // Append the sboms.tar layer if it exists // From ace404d9926aa68df3a7d3492ae4c1e1ba875473 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 15:31:44 -0700 Subject: [PATCH 12/16] chore(pkg/lint): remove named returns and return error paths early Signed-off-by: Kit Patella --- src/pkg/lint/validate.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/pkg/lint/validate.go b/src/pkg/lint/validate.go index 2de0ba8e91..0083c8a6b7 100644 --- a/src/pkg/lint/validate.go +++ b/src/pkg/lint/validate.go @@ -234,7 +234,7 @@ func validateAction(action v1alpha1.ZarfComponentAction) error { // validateReleaseName validates a release name against DNS 1035 spec, using chartName as fallback. // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#rfc-1035-label-names -func validateReleaseName(chartName, releaseName string) (err error) { +func validateReleaseName(chartName, releaseName string) error { // Fallback to chartName if releaseName is empty // NOTE: Similar fallback mechanism happens in src/internal/packager/helm/chart.go:InstallOrUpgradeChart if releaseName == "" { @@ -243,16 +243,15 @@ func validateReleaseName(chartName, releaseName string) (err error) { // Check if the final releaseName is empty and return an error if so if releaseName == "" { - err = errors.New(errChartReleaseNameEmpty) - return + return errors.New(errChartReleaseNameEmpty) } // Validate the releaseName against DNS 1035 label spec if errs := validation.IsDNS1035Label(releaseName); len(errs) > 0 { - err = fmt.Errorf("invalid release name '%s': %s", releaseName, strings.Join(errs, "; ")) + return fmt.Errorf("invalid release name '%s': %s", releaseName, strings.Join(errs, "; ")) } - return + return nil } // validateChart runs all validation checks on a chart. From dd521ffca92782b74b8ab5de4596ff0ded100aed Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 15:38:37 -0700 Subject: [PATCH 13/16] chore(pkg/message): remove unused named return Signed-off-by: Kit Patella --- src/pkg/message/pausable.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pkg/message/pausable.go b/src/pkg/message/pausable.go index b9e8fae1c7..3a61f3cb59 100644 --- a/src/pkg/message/pausable.go +++ b/src/pkg/message/pausable.go @@ -29,6 +29,6 @@ func (pw *PausableWriter) Resume() { } // Write writes the data to the underlying output writer -func (pw *PausableWriter) Write(p []byte) (n int, err error) { +func (pw *PausableWriter) Write(p []byte) (int, error) { return pw.out.Write(p) } From 9c8e3e18eccc3a925509f4a6e335055ca32b5347 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 16:41:03 -0700 Subject: [PATCH 14/16] refactor(pkg/cluster): simplify PackageSecretNeedsWait and remove unnecessary named returns Signed-off-by: Kit Patella --- src/pkg/cluster/state.go | 4 +++- src/pkg/cluster/zarf.go | 32 +++++++++++++++++--------------- src/pkg/cluster/zarf_test.go | 12 +----------- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/pkg/cluster/state.go b/src/pkg/cluster/state.go index 3c31ccf128..f2279b0221 100644 --- a/src/pkg/cluster/state.go +++ b/src/pkg/cluster/state.go @@ -193,12 +193,14 @@ func (c *Cluster) InitZarfState(ctx context.Context, initOptions types.ZarfInitO } // LoadZarfState returns the current zarf/zarf-state secret data or an empty ZarfState. -func (c *Cluster) LoadZarfState(ctx context.Context) (state *types.ZarfState, err error) { +func (c *Cluster) LoadZarfState(ctx context.Context) (*types.ZarfState, error) { stateErr := errors.New("failed to load the Zarf State from the cluster, has Zarf been initiated?") secret, err := c.Clientset.CoreV1().Secrets(ZarfNamespaceName).Get(ctx, ZarfStateSecretName, metav1.GetOptions{}) if err != nil { return nil, fmt.Errorf("%w: %w", stateErr, err) } + + state := &types.ZarfState{} err = json.Unmarshal(secret.Data[ZarfStateDataKey], &state) if err != nil { return nil, fmt.Errorf("%w: %w", stateErr, err) diff --git a/src/pkg/cluster/zarf.go b/src/pkg/cluster/zarf.go index 3557544e14..a9851a29f3 100644 --- a/src/pkg/cluster/zarf.go +++ b/src/pkg/cluster/zarf.go @@ -109,28 +109,29 @@ func (c *Cluster) StripZarfLabelsAndSecretsFromNamespaces(ctx context.Context) { spinner.Success() } -// PackageSecretNeedsWait checks if a package component has a running webhook that needs to be waited on. -func (c *Cluster) PackageSecretNeedsWait(deployedPackage *types.DeployedPackage, component v1alpha1.ZarfComponent, skipWebhooks bool) (needsWait bool, waitSeconds int, hookName string) { +// PackageSecretNeedsWait checks if a package component has a running webhook that needs to be waited on. Returns the +// number of seconds remaining to wait and the name of the webhook. If seconds is zero there's no need to wait. +func (c *Cluster) PackageSecretNeedsWait(deployedPackage *types.DeployedPackage, component v1alpha1.ZarfComponent, skipWebhooks bool) (int, string) { // Skip checking webhook status when '--skip-webhooks' flag is provided and for YOLO packages if skipWebhooks || deployedPackage == nil || deployedPackage.Data.Metadata.YOLO { - return false, 0, "" + return 0, "" } // Look for the specified component hookMap, componentExists := deployedPackage.ComponentWebhooks[component.Name] if !componentExists { - return false, 0, "" // Component not found, no need to wait + return 0, "" // Component not found, no need to wait } // Check if there are any "Running" webhooks for the component that we need to wait for for hookName, webhook := range hookMap { if webhook.Status == types.WebhookStatusRunning { - return true, webhook.WaitDurationSeconds, hookName + return webhook.WaitDurationSeconds, hookName } } // If we get here, the component doesn't need to wait for a webhook to run - return false, 0, "" + return 0, "" } // RecordPackageDeploymentAndWait records the deployment of a package to the cluster and waits for any webhooks to complete. @@ -140,9 +141,9 @@ func (c *Cluster) RecordPackageDeploymentAndWait(ctx context.Context, pkg v1alph return nil, err } - packageNeedsWait, waitSeconds, hookName := c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) + waitSeconds, hookName := c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) // If no webhooks need to complete, we can return immediately. - if !packageNeedsWait { + if waitSeconds == 0 { return deployedPackage, nil } @@ -160,8 +161,8 @@ func (c *Cluster) RecordPackageDeploymentAndWait(ctx context.Context, pkg v1alph if err != nil { return nil, err } - packageNeedsWait, _, _ = c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) - if !packageNeedsWait { + waitSeconds, _ = c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) + if waitSeconds == 0 { return deployedPackage, nil } return deployedPackage, nil @@ -174,7 +175,7 @@ func (c *Cluster) RecordPackageDeploymentAndWait(ctx context.Context, pkg v1alph } // RecordPackageDeployment saves metadata about a package that has been deployed to the cluster. -func (c *Cluster) RecordPackageDeployment(ctx context.Context, pkg v1alpha1.ZarfPackage, components []types.DeployedComponent, generation int) (deployedPackage *types.DeployedPackage, err error) { +func (c *Cluster) RecordPackageDeployment(ctx context.Context, pkg v1alpha1.ZarfPackage, components []types.DeployedComponent, generation int) (*types.DeployedPackage, error) { packageName := pkg.Metadata.Name // Attempt to load information about webhooks for the package @@ -187,7 +188,7 @@ func (c *Cluster) RecordPackageDeployment(ctx context.Context, pkg v1alpha1.Zarf componentWebhooks = existingPackageSecret.ComponentWebhooks } - // TODO: This is done for backwards compartibility and could be removed in the future. + // TODO: This is done for backwards compatibility and could be removed in the future. connectStrings := types.ConnectStrings{} for _, comp := range components { for _, chart := range comp.InstalledCharts { @@ -197,7 +198,7 @@ func (c *Cluster) RecordPackageDeployment(ctx context.Context, pkg v1alpha1.Zarf } } - deployedPackage = &types.DeployedPackage{ + deployedPackage := &types.DeployedPackage{ Name: packageName, CLIVersion: config.CLIVersion, Data: pkg, @@ -285,12 +286,13 @@ func (c *Cluster) DisableRegHPAScaleDown(ctx context.Context) error { } // GetInstalledChartsForComponent returns any installed Helm Charts for the provided package component. -func (c *Cluster) GetInstalledChartsForComponent(ctx context.Context, packageName string, component v1alpha1.ZarfComponent) (installedCharts []types.InstalledChart, err error) { +func (c *Cluster) GetInstalledChartsForComponent(ctx context.Context, packageName string, component v1alpha1.ZarfComponent) ([]types.InstalledChart, error) { deployedPackage, err := c.GetDeployedPackage(ctx, packageName) if err != nil { - return installedCharts, err + return nil, err } + installedCharts := make([]types.InstalledChart, 0) for _, deployedComponent := range deployedPackage.DeployedComponents { if deployedComponent.Name == component.Name { installedCharts = append(installedCharts, deployedComponent.InstalledCharts...) diff --git a/src/pkg/cluster/zarf_test.go b/src/pkg/cluster/zarf_test.go index d2f3aefcde..89b9c26e1a 100644 --- a/src/pkg/cluster/zarf_test.go +++ b/src/pkg/cluster/zarf_test.go @@ -30,7 +30,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { deployedPackage *types.DeployedPackage component v1alpha1.ZarfComponent skipWebhooks bool - needsWait bool waitSeconds int hookName string } @@ -49,7 +48,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { Name: packageName, ComponentWebhooks: map[string]map[string]types.Webhook{}, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -67,7 +65,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: true, waitSeconds: 10, hookName: webhookName, }, @@ -86,7 +83,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -103,7 +99,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -120,7 +115,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -137,7 +131,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -160,7 +153,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -179,7 +171,6 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, - needsWait: false, waitSeconds: 0, hookName: "", }, @@ -193,9 +184,8 @@ func TestPackageSecretNeedsWait(t *testing.T) { c := &Cluster{} - needsWait, waitSeconds, hookName := c.PackageSecretNeedsWait(testCase.deployedPackage, testCase.component, testCase.skipWebhooks) + waitSeconds, hookName := c.PackageSecretNeedsWait(testCase.deployedPackage, testCase.component, testCase.skipWebhooks) - require.Equal(t, testCase.needsWait, needsWait) require.Equal(t, testCase.waitSeconds, waitSeconds) require.Equal(t, testCase.hookName, hookName) }) From f8216e52843c31e5f1e0ebfd18b02a0fa8d2a4e0 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Mon, 9 Sep 2024 17:07:53 -0700 Subject: [PATCH 15/16] chore(pkg/layout): remove unnecessary named returns Signed-off-by: Kit Patella --- src/pkg/layout/component.go | 26 +++++++++++++------------- src/pkg/layout/package.go | 7 +++++-- src/pkg/layout/sbom.go | 17 +++++++++++------ 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/pkg/layout/component.go b/src/pkg/layout/component.go index fee2d90082..c3ce6ae930 100644 --- a/src/pkg/layout/component.go +++ b/src/pkg/layout/component.go @@ -39,7 +39,7 @@ type Components struct { var ErrNotLoaded = fmt.Errorf("not loaded") // Archive archives a component. -func (c *Components) Archive(component v1alpha1.ZarfComponent, cleanupTemp bool) (err error) { +func (c *Components) Archive(component v1alpha1.ZarfComponent, cleanupTemp bool) error { name := component.Name if _, ok := c.Dirs[name]; !ok { return &fs.PathError{ @@ -75,7 +75,7 @@ func (c *Components) Archive(component v1alpha1.ZarfComponent, cleanupTemp bool) } // Unarchive unarchives a component. -func (c *Components) Unarchive(component v1alpha1.ZarfComponent) (err error) { +func (c *Components) Unarchive(component v1alpha1.ZarfComponent) error { name := component.Name tb, ok := c.Tarballs[name] if !ok { @@ -138,7 +138,7 @@ func (c *Components) Unarchive(component v1alpha1.ZarfComponent) (err error) { } // Create creates a new component directory structure. -func (c *Components) Create(component v1alpha1.ZarfComponent) (cp *ComponentPaths, err error) { +func (c *Components) Create(component v1alpha1.ZarfComponent) (*ComponentPaths, error) { name := component.Name _, ok := c.Tarballs[name] @@ -150,41 +150,41 @@ func (c *Components) Create(component v1alpha1.ZarfComponent) (cp *ComponentPath } } - if err = helpers.CreateDirectory(c.Base, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(c.Base, helpers.ReadWriteExecuteUser); err != nil { return nil, err } base := filepath.Join(c.Base, name) - if err = helpers.CreateDirectory(base, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(base, helpers.ReadWriteExecuteUser); err != nil { return nil, err } - cp = &ComponentPaths{ + cp := &ComponentPaths{ Base: base, } cp.Temp = filepath.Join(base, TempDir) - if err = helpers.CreateDirectory(cp.Temp, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Temp, helpers.ReadWriteExecuteUser); err != nil { return nil, err } if len(component.Files) > 0 { cp.Files = filepath.Join(base, FilesDir) - if err = helpers.CreateDirectory(cp.Files, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Files, helpers.ReadWriteExecuteUser); err != nil { return nil, err } } if len(component.Charts) > 0 { cp.Charts = filepath.Join(base, ChartsDir) - if err = helpers.CreateDirectory(cp.Charts, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Charts, helpers.ReadWriteExecuteUser); err != nil { return nil, err } for _, chart := range component.Charts { cp.Values = filepath.Join(base, ValuesDir) if len(chart.ValuesFiles) > 0 { - if err = helpers.CreateDirectory(cp.Values, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Values, helpers.ReadWriteExecuteUser); err != nil { return nil, err } break @@ -194,21 +194,21 @@ func (c *Components) Create(component v1alpha1.ZarfComponent) (cp *ComponentPath if len(component.Repos) > 0 { cp.Repos = filepath.Join(base, ReposDir) - if err = helpers.CreateDirectory(cp.Repos, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Repos, helpers.ReadWriteExecuteUser); err != nil { return nil, err } } if len(component.Manifests) > 0 { cp.Manifests = filepath.Join(base, ManifestsDir) - if err = helpers.CreateDirectory(cp.Manifests, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.Manifests, helpers.ReadWriteExecuteUser); err != nil { return nil, err } } if len(component.DataInjections) > 0 { cp.DataInjections = filepath.Join(base, DataInjectionsDir) - if err = helpers.CreateDirectory(cp.DataInjections, helpers.ReadWriteExecuteUser); err != nil { + if err := helpers.CreateDirectory(cp.DataInjections, helpers.ReadWriteExecuteUser); err != nil { return nil, err } } diff --git a/src/pkg/layout/package.go b/src/pkg/layout/package.go index a38ec599a1..532f46653d 100644 --- a/src/pkg/layout/package.go +++ b/src/pkg/layout/package.go @@ -52,11 +52,14 @@ func New(baseDir string) *PackagePaths { // ReadZarfYAML reads a zarf.yaml file into memory, // checks if it's using the legacy layout, and migrates deprecated component configs. -func (pp *PackagePaths) ReadZarfYAML() (pkg v1alpha1.ZarfPackage, warnings []string, err error) { +func (pp *PackagePaths) ReadZarfYAML() (v1alpha1.ZarfPackage, []string, error) { + var pkg v1alpha1.ZarfPackage + if err := utils.ReadYaml(pp.ZarfYAML, &pkg); err != nil { return v1alpha1.ZarfPackage{}, nil, fmt.Errorf("unable to read zarf.yaml: %w", err) } + warnings := make([]string, 0) if pp.IsLegacyLayout() { warnings = append(warnings, "Detected deprecated package layout, migrating to new layout - support for this package will be dropped in v1.0.0") } @@ -74,7 +77,7 @@ func (pp *PackagePaths) ReadZarfYAML() (pkg v1alpha1.ZarfPackage, warnings []str } // MigrateLegacy migrates a legacy package layout to the new layout. -func (pp *PackagePaths) MigrateLegacy() (err error) { +func (pp *PackagePaths) MigrateLegacy() error { var pkg v1alpha1.ZarfPackage base := pp.Base diff --git a/src/pkg/layout/sbom.go b/src/pkg/layout/sbom.go index 7ac39c02a7..fcfb300be6 100644 --- a/src/pkg/layout/sbom.go +++ b/src/pkg/layout/sbom.go @@ -26,7 +26,7 @@ type SBOMs struct { } // Unarchive unarchives the package's SBOMs. -func (s *SBOMs) Unarchive() (err error) { +func (s *SBOMs) Unarchive() error { if s.Path == "" || helpers.InvalidPath(s.Path) { return &fs.PathError{ Op: "stat", @@ -47,7 +47,7 @@ func (s *SBOMs) Unarchive() (err error) { } // Archive archives the package's SBOMs. -func (s *SBOMs) Archive() (err error) { +func (s *SBOMs) Archive() error { if s.Path == "" || helpers.InvalidPath(s.Path) { return &fs.PathError{ Op: "stat", @@ -68,18 +68,23 @@ func (s *SBOMs) Archive() (err error) { return os.RemoveAll(dir) } -// StageSBOMViewFiles copies SBOM viewer HTML files to the Zarf SBOM directory. -func (s *SBOMs) StageSBOMViewFiles() (sbomViewFiles, warnings []string, err error) { +// StageSBOMViewFiles copies SBOM viewer HTML files to the Zarf SBOM directory. Returns sbomViewFiles, warnings, and an +// error. +func (s *SBOMs) StageSBOMViewFiles() ([]string, []string, error) { + sbomViewFiles := make([]string, 0) + warnings := make([]string, 0) + if s.IsTarball() { return nil, nil, fmt.Errorf("unable to process the SBOM files for this package: %s is a tarball", s.Path) } // If SBOMs were loaded, temporarily place them in the deploy directory if !helpers.InvalidPath(s.Path) { - sbomViewFiles, err = filepath.Glob(filepath.Join(s.Path, "sbom-viewer-*")) + files, err := filepath.Glob(filepath.Join(s.Path, "sbom-viewer-*")) if err != nil { return nil, nil, err } + sbomViewFiles = files if _, err := s.OutputSBOMFiles(SBOMDir, ""); err != nil { // Don't stop the deployment, let the user decide if they want to continue the deployment @@ -107,6 +112,6 @@ func (s *SBOMs) OutputSBOMFiles(outputDir, packageName string) (string, error) { } // IsTarball returns true if the SBOMs are a tarball. -func (s SBOMs) IsTarball() bool { +func (s *SBOMs) IsTarball() bool { return !helpers.IsDir(s.Path) && filepath.Ext(s.Path) == ".tar" } From c4b138d498107d5803efe2b59d05b08a6cce5de0 Mon Sep 17 00:00:00 2001 From: Kit Patella Date: Tue, 10 Sep 2024 10:42:09 -0700 Subject: [PATCH 16/16] fix(pkg/cluster): partially revert 9c8e3e18eccc3a925509f4a6e335055ca32b5347 to remove changes to PackageSecretNeedsWait Signed-off-by: Kit Patella --- src/pkg/cluster/zarf.go | 21 ++++++++++----------- src/pkg/cluster/zarf_test.go | 12 +++++++++++- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/pkg/cluster/zarf.go b/src/pkg/cluster/zarf.go index a9851a29f3..eebbd6d295 100644 --- a/src/pkg/cluster/zarf.go +++ b/src/pkg/cluster/zarf.go @@ -109,29 +109,28 @@ func (c *Cluster) StripZarfLabelsAndSecretsFromNamespaces(ctx context.Context) { spinner.Success() } -// PackageSecretNeedsWait checks if a package component has a running webhook that needs to be waited on. Returns the -// number of seconds remaining to wait and the name of the webhook. If seconds is zero there's no need to wait. -func (c *Cluster) PackageSecretNeedsWait(deployedPackage *types.DeployedPackage, component v1alpha1.ZarfComponent, skipWebhooks bool) (int, string) { +// PackageSecretNeedsWait checks if a package component has a running webhook that needs to be waited on. +func (c *Cluster) PackageSecretNeedsWait(deployedPackage *types.DeployedPackage, component v1alpha1.ZarfComponent, skipWebhooks bool) (needsWait bool, waitSeconds int, hookName string) { // Skip checking webhook status when '--skip-webhooks' flag is provided and for YOLO packages if skipWebhooks || deployedPackage == nil || deployedPackage.Data.Metadata.YOLO { - return 0, "" + return false, 0, "" } // Look for the specified component hookMap, componentExists := deployedPackage.ComponentWebhooks[component.Name] if !componentExists { - return 0, "" // Component not found, no need to wait + return false, 0, "" // Component not found, no need to wait } // Check if there are any "Running" webhooks for the component that we need to wait for for hookName, webhook := range hookMap { if webhook.Status == types.WebhookStatusRunning { - return webhook.WaitDurationSeconds, hookName + return true, webhook.WaitDurationSeconds, hookName } } // If we get here, the component doesn't need to wait for a webhook to run - return 0, "" + return false, 0, "" } // RecordPackageDeploymentAndWait records the deployment of a package to the cluster and waits for any webhooks to complete. @@ -141,9 +140,9 @@ func (c *Cluster) RecordPackageDeploymentAndWait(ctx context.Context, pkg v1alph return nil, err } - waitSeconds, hookName := c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) + packageNeedsWait, waitSeconds, hookName := c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) // If no webhooks need to complete, we can return immediately. - if waitSeconds == 0 { + if !packageNeedsWait { return deployedPackage, nil } @@ -161,8 +160,8 @@ func (c *Cluster) RecordPackageDeploymentAndWait(ctx context.Context, pkg v1alph if err != nil { return nil, err } - waitSeconds, _ = c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) - if waitSeconds == 0 { + packageNeedsWait, _, _ = c.PackageSecretNeedsWait(deployedPackage, component, skipWebhooks) + if !packageNeedsWait { return deployedPackage, nil } return deployedPackage, nil diff --git a/src/pkg/cluster/zarf_test.go b/src/pkg/cluster/zarf_test.go index 89b9c26e1a..d2f3aefcde 100644 --- a/src/pkg/cluster/zarf_test.go +++ b/src/pkg/cluster/zarf_test.go @@ -30,6 +30,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { deployedPackage *types.DeployedPackage component v1alpha1.ZarfComponent skipWebhooks bool + needsWait bool waitSeconds int hookName string } @@ -48,6 +49,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { Name: packageName, ComponentWebhooks: map[string]map[string]types.Webhook{}, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -65,6 +67,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: true, waitSeconds: 10, hookName: webhookName, }, @@ -83,6 +86,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -99,6 +103,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -115,6 +120,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -131,6 +137,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -153,6 +160,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -171,6 +179,7 @@ func TestPackageSecretNeedsWait(t *testing.T) { }, }, }, + needsWait: false, waitSeconds: 0, hookName: "", }, @@ -184,8 +193,9 @@ func TestPackageSecretNeedsWait(t *testing.T) { c := &Cluster{} - waitSeconds, hookName := c.PackageSecretNeedsWait(testCase.deployedPackage, testCase.component, testCase.skipWebhooks) + needsWait, waitSeconds, hookName := c.PackageSecretNeedsWait(testCase.deployedPackage, testCase.component, testCase.skipWebhooks) + require.Equal(t, testCase.needsWait, needsWait) require.Equal(t, testCase.waitSeconds, waitSeconds) require.Equal(t, testCase.hookName, hookName) })