handle source.spec.helm.ignoreMissingValuesFiles = true

[sl1pm4t]

If the helm source has ignoreMissingValuesFiles = true kubechecks should ignore values file copy errors where the file is not found.

In one of our multi source appsets we reference a bunch of optional values files that allow overriding values at different levels / layers / environments / tiers / providers / whatever.

failed to generate manifests: failed to package application: failed to copy referenced value file: "$values/apps/aws-provider/default.values.yaml": open /tmp/kubechecks-repo-4238925250/apps/aws-provider/default.values.yaml: no such file or directory

[djeebus]

Good call, thanks!

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/argo_client/manifests.go

@@ -334,7 +334,12 @@ func packageApp(ctx context.Context, source v1alpha1.ApplicationSource, refs []v
 				src := filepath.Join(refRepo.Directory, refPath)
 				dst := filepath.Join(tempDir, refPath)
 				if err = copyFile(src, dst); err != nil {
-					return "", errors.Wrapf(err, "failed to copy referenced value file: %q", valueFile)
+					// handle source.spec.helm.ignoreMissingValues = true
+					if errors.Is(err, os.ErrNotExist) && source.Helm.IgnoreMissingValueFiles {
+						log.Debug().Str("valueFile", valueFile).Msg("ignore missing values file, because source.Helm.IgnoreMissingValueFiles is true")
+					} else {
+						return "", errors.Wrapf(err, "failed to copy referenced value file: %q", valueFile)
+					}
 				}
 
 				relPath, err := filepath.Rel(tempAppDir, dst)

Feedback & Suggestions:

1. Error Handling Improvement: The added condition to handle missing value files when IgnoreMissingValueFiles is true is a good addition. However, consider logging at a higher level (e.g., Info) if this is an expected behavior that should be visible in production logs. If it's purely for debugging, Debug is appropriate.

2. Code Clarity: The comment // handle source.spec.helm.ignoreMissingValues = true could be more descriptive. Consider rephrasing it to explain the logic, such as // Skip missing value files if IgnoreMissingValueFiles is set to true.

3. Performance Consideration: If the IgnoreMissingValueFiles flag is frequently used, consider measuring the performance impact of logging each missing file. If it becomes a bottleneck, you might want to batch log these messages or provide a summary at the end.

4. Security Consideration: Ensure that the valueFile being logged does not contain sensitive information. If there's any chance it might, consider sanitizing the output.

Overall, the changes are well-implemented for the intended functionality. 🛠️

---

---

Dependency Review

Click to read mergecats review!

No suggestions found