feat: improve kustomization walk for helmCharts section

[Greyeye]

current kustomization.yaml walk does not parse the helmCharts section.
This PR is to add ability to add these values file per chart and send to the argo-cd-repo server.

[github-actions]

Temporary image deleted.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/kustomize/process.go

@@ -44,7 +44,7 @@ func processDir(sourceFS fs.FS, relBase string) (files []string, dirs []string,
 	var filesOrDirectories []string
 	filesOrDirectories = append(filesOrDirectories, kust.Bases...) // nolint:staticcheck // deprecated doesn't mean unused
 	filesOrDirectories = append(filesOrDirectories, kust.Resources...)
-
+	filesOrDirectories = append(filesOrDirectories, getValuesFromKustomizationHelm(&kust)...)
 	var directories []string
 	directories = append(directories, kust.Components...)
 
@@ -154,3 +154,11 @@ func isRemoteResource(resource string) bool {
 
 	return false
 }
+
+// getValuesFromKustomizationHelm will parse the helmCharts sections' valueFile field and return them as a slice of strings.
+func getValuesFromKustomizationHelm(kust *types.Kustomization) (files []string) {
+	for _, helm := range kust.HelmCharts {
+		files = append(files, helm.ValuesFile)
+	}
+	return files
+}

Feedback & Suggestions:

1. Error Handling for ValuesFile: The getValuesFromKustomizationHelm function appends helm.ValuesFile directly to the files slice. It would be prudent to check if ValuesFile is not empty before appending to avoid potential issues with empty strings. Consider adding a check like if helm.ValuesFile != "" { ... }.

2. Function Documentation: The comment for getValuesFromKustomizationHelm is helpful, but it could be more descriptive. Consider mentioning that it returns a slice of strings containing the paths to the Helm values files, and clarify what happens if there are no Helm charts.

3. Security Consideration: Ensure that the paths returned by getValuesFromKustomizationHelm are sanitized and validated before being used to open files. This is crucial to prevent path traversal vulnerabilities.

4. Performance Consideration: If kust.HelmCharts is large, consider pre-allocating the files slice with an appropriate capacity to avoid multiple allocations. This can be done using files := make([]string, 0, len(kust.HelmCharts)).

---

😼 Mergecat review of pkg/kustomize/process_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"sigs.k8s.io/kustomize/api/types"
 	_ "sigs.k8s.io/kustomize/api/types"
 )
 
@@ -203,4 +204,70 @@ resources:
 		assert.Error(t, err)
 		assert.Contains(t, err.Error(), "failed to stat testdir/missing-resource.yaml")
 	})
+	t.Run("helmChart", func(t *testing.T) {
+		kustContent := `
+helmCharts:
+  - name: dummy
+    repo: https://dummy.local/repo
+    version: 1.2.3
+    releaseName: dummy
+    namespace: dummy
+    includeCRDs: true
+    valuesFile: values-dummy.yaml
+`
+		valueContent := `
+dummy:
+  labels:
+    release: dummy
+`
+		sourceFS := fstest.MapFS{
+			"testdir/kustomization.yaml": &fstest.MapFile{
+				Data: []byte(kustContent),
+			},
+			"testdir/values-dummy.yaml": &fstest.MapFile{
+				Data: []byte(valueContent),
+			},
+		}
+
+		files, _, err := processDir(sourceFS, "testdir")
+		assert.NoError(t, err)
+		assert.Contains(t, files, "testdir/values-dummy.yaml")
+	})
+}
+
+func Test_getValuesFromKustomizationHelm(t *testing.T) {
+	type args struct {
+		kust *types.Kustomization
+	}
+	tests := []struct {
+		name      string
+		args      args
+		wantFiles []string
+	}{
+		{
+			name: "normal",
+			args: args{
+				kust: &types.Kustomization{
+					HelmCharts: []types.HelmChart{
+						{Name: "dummy", ValuesFile: "values-dummy.yaml"},
+					},
+				},
+			},
+			wantFiles: []string{"values-dummy.yaml"},
+		},
+		{
+			name: "helmChart is nil.",
+			args: args{
+				kust: &types.Kustomization{
+					HelmCharts: nil,
+				},
+			},
+			wantFiles: nil,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.wantFiles, getValuesFromKustomizationHelm(tt.args.kust), "getValuesFromKustomizationHelm(%v)", tt.args.kust)
+		})
+	}
 }

Feedback & Suggestions:

1. Redundant Import: The import of "sigs.k8s.io/kustomize/api/types" is duplicated. You can remove the line with the underscore import (_ "sigs.k8s.io/kustomize/api/types") as it is not necessary to import the same package twice. 🗑️

2. Test Coverage: The new test Test_getValuesFromKustomizationHelm is a good addition. However, consider adding more test cases to cover edge cases, such as when HelmCharts contains multiple entries or when ValuesFile is an empty string. This will ensure robustness. 🧪

3. Error Handling: In the helmChart test case, ensure that the processDir function is correctly handling errors related to Helm charts. Consider adding assertions to check for specific error messages if the function is expected to handle such cases. 🔍

4. Code Comments: Adding comments to explain the purpose of the new test cases and any complex logic would improve code readability and maintainability. 📝

---

---

Dependency Review

Click to read mergecats review!

No suggestions found