handle kustomize folders vs folder of manifests

[djeebus]

Turns out that kustomize can point to a folder full of manifests, in which case we need to track the full directory. Alternately, if a folder has a kustomization.yaml file, we don't need to track the folder, only the files in it.

[github-actions]

Temporary image deleted.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/kustomize/process.go

@@ -8,25 +8,49 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
+	"github.com/rs/zerolog/log"
 	"sigs.k8s.io/kustomize/api/types"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
 // ProcessKustomizationFile processes a kustomization file and returns all the files and directories it references.
-func ProcessKustomizationFile(sourceFS fs.FS, relKustomizationPath string) (files []string, dirs []string, err error) {
+func ProcessKustomizationFile(sourceFS fs.FS, relKustomizationPath string) (files, dirs []string, err error) {
 	dirName := filepath.Dir(relKustomizationPath)
-	return processDir(sourceFS, dirName)
+
+	proc := processor{
+		visitedDirs: make(map[string]struct{}),
+	}
+
+	files, dirs, err = proc.processDir(sourceFS, dirName)
+	if err != nil {
+		return nil, nil, errors.Wrapf(err, "failed to process kustomize file %q", relKustomizationPath)
+	}
+
+	return files, dirs, nil
 }
 
-func processDir(sourceFS fs.FS, relBase string) (files []string, dirs []string, err error) {
+type processor struct {
+	visitedDirs map[string]struct{}
+}
+
+func (p processor) processDir(sourceFS fs.FS, relBase string) (files, dirs []string, err error) {
+	if _, ok := p.visitedDirs[relBase]; ok {
+		log.Warn().Msgf("directory %q already processed", relBase)
+		return nil, nil, nil
+	}
+
+	log.Info().Msgf("processing directory %q", relBase)
+	p.visitedDirs[relBase] = struct{}{}
+
 	absKustPath := filepath.Join(relBase, "kustomization.yaml")
 
 	// Parse using official Kustomization type
 	file, err := sourceFS.Open(absKustPath)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return nil, nil, nil // No kustomization.yaml in this directory
+			return nil, []string{relBase}, nil // No kustomization.yaml in this directory, the dir is the important thing
 		}
+
 		return nil, nil, errors.Wrapf(err, "failed to open file %q", absKustPath)
 	}
 
@@ -44,7 +68,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...)
 
@@ -53,6 +77,10 @@ func processDir(sourceFS fs.FS, relBase string) (files []string, dirs []string,
 	files = append(files, kust.Crds...)
 	files = append(files, kust.Transformers...)
 
+	for _, helm := range kust.HelmCharts {
+		files = append(files, helm.ValuesFile)
+	}
+
 	for _, patch := range kust.Patches {
 		if patch.Path != "" {
 			files = append(files, patch.Path)
@@ -93,13 +121,12 @@ func processDir(sourceFS fs.FS, relBase string) (files []string, dirs []string,
 		}
 	}
 
-	// We now know this directory has a kustomization.yaml, so add it to "dirs".
-	allDirs := append([]string(nil), relBase)
 	allFiles := append([]string(nil), files...)
+	var allDirs []string
 
 	// process directories and add them
 	for _, relResource := range directories {
-		subFiles, subDirs, err := processDir(sourceFS, relResource)
+		subFiles, subDirs, err := p.processDir(sourceFS, relResource)
 		if err != nil {
 			return nil, nil, errors.Wrapf(err, "failed to process %q", relResource)
 		}
@@ -154,11 +181,3 @@ 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. Logging Overhead: The addition of logging using zerolog is a good practice for debugging and monitoring. However, ensure that the logging level is appropriately set in production to avoid performance overhead. Consider using log.Debug() for less critical information to reduce verbosity. 🐢

2. Processor Struct: The introduction of the processor struct to track visited directories is a great improvement to prevent redundant processing. However, consider using a pointer receiver for the processDir method to avoid copying the processor struct on each method call. This can improve performance, especially if the struct grows in size. 📈

3. Error Handling: The error handling has been improved by wrapping errors with more context. Ensure that the error messages are consistent and provide enough information for debugging. 🛠️

4. Removal of getValuesFromKustomizationHelm: The function getValuesFromKustomizationHelm was removed, and its logic was inlined. This is fine for now, but if the logic becomes more complex or is reused, consider refactoring it back into a separate function for better maintainability. 🔄

5. Directory Handling: The change to return the directory when kustomization.yaml is not found is a logical improvement. Ensure that this behavior aligns with the intended functionality and does not introduce any unexpected side effects. 📂

Overall, the changes improve the code's robustness and maintainability. Keep an eye on performance and maintainability as the codebase evolves. 🚀

---

😼 Mergecat review of pkg/kustomize/process_test.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sigs.k8s.io/kustomize/api/types"
 	_ "sigs.k8s.io/kustomize/api/types"
 )
 
@@ -56,10 +55,10 @@ func (d dummyFileInfo) Sys() interface{}   { return nil }
 func TestProcessDir(t *testing.T) {
 	t.Run("NoKustomization", func(t *testing.T) {
 		sourceFS := fstest.MapFS{}
-		files, dirs, err := processDir(sourceFS, "testdir")
+		files, dirs, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		assert.NoError(t, err)
 		assert.Empty(t, files)
-		assert.Empty(t, dirs)
+		assert.Equal(t, []string{"testdir"}, dirs)
 	})
 
 	t.Run("OpenError", func(t *testing.T) {
@@ -71,7 +70,7 @@ func TestProcessDir(t *testing.T) {
 				return nil, fs.ErrNotExist
 			},
 		}
-		_, _, err := processDir(mfs, "testdir")
+		_, _, err := ProcessKustomizationFile(mfs, filepath.Join("testdir", "kustomation.yaml"))
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "failed to open file")
 	})
@@ -85,24 +84,26 @@ func TestProcessDir(t *testing.T) {
 				return nil, fs.ErrNotExist
 			},
 		}
-		_, _, err := processDir(mfs, "testdir")
+		_, _, err := ProcessKustomizationFile(mfs, filepath.Join("testdir", "kustomization.yaml"))
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "failed to read file")
 	})
 
 	t.Run("ValidKustomization", func(t *testing.T) {
 		kustContent := `
 resources:
- - resource.yaml
+- resource.yaml
+- ../rootdir
 bases:
- - base
+- base
 components:
- - components
+- components
 `
 		sourceFS := fstest.MapFS{
 			"testdir/kustomization.yaml": &fstest.MapFile{
 				Data: []byte(kustContent),
 			},
+			"rootdir/file.yaml":     &fstest.MapFile{},
 			"testdir/resource.yaml": &fstest.MapFile{},
 			"testdir/base/kustomization.yaml": &fstest.MapFile{
 				Data: []byte(`resources: ["base_resource.yaml"]`),
@@ -113,7 +114,7 @@ components:
 			},
 		}
 
-		files, dir, err := processDir(sourceFS, "testdir")
+		files, dirs, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		require.NoError(t, err)
 
 		expectedFiles := []string{
@@ -123,18 +124,44 @@ components:
 			"testdir/base/base_resource.yaml",
 			"testdir/components/kustomization.yaml",
 		}
+		expectedDirs := []string{
+			"rootdir",
+		}
 		sort.Strings(files)
 		sort.Strings(expectedFiles)
 		assert.Equal(t, expectedFiles, files)
+		assert.Equal(t, expectedDirs, dirs)
+	})
 
-		expectedDirs := []string{
-			"testdir",
-			"testdir/base",
-			"testdir/components",
+	t.Run("relative components", func(t *testing.T) {
+		sourceFS := fstest.MapFS{
+			"apps/app1/overlays/env1/kustomization.yaml": &fstest.MapFile{
+				Data: []byte(`
+components:
+  - ../../components/component1
+  - ../../components/component2
+`),
+			},
+			"apps/app1/components/component1/kustomization.yaml": &fstest.MapFile{
+				Data: []byte(`
+resources:
+- resource1.yaml`),
+			},
+			"apps/app1/components/component1/resource1.yaml": &fstest.MapFile{},
+			"apps/app1/components/component2/resource1.yaml": &fstest.MapFile{},
+			"apps/app1/components/component2/resource2.yaml": &fstest.MapFile{},
 		}
-		sort.Strings(dir)
-		sort.Strings(expectedDirs)
-		assert.Equal(t, expectedDirs, dir)
+
+		files, dirs, err := ProcessKustomizationFile(sourceFS, filepath.Join("apps", "app1", "overlays", "env1", "kustomization.yaml"))
+		require.NoError(t, err)
+		assert.Equal(t, []string{
+			"apps/app1/overlays/env1/kustomization.yaml",
+			"apps/app1/components/component1/kustomization.yaml",
+			"apps/app1/components/component1/resource1.yaml",
+		}, files)
+		assert.Equal(t, []string{
+			"apps/app1/components/component2",
+		}, dirs)
 	})
 
 	t.Run("StrategicMergePatch", func(t *testing.T) {
@@ -150,7 +177,7 @@ patchesStrategicMerge:
 			},
 		}
 
-		files, _, err := processDir(sourceFS, "testdir")
+		files, _, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		assert.NoError(t, err)
 		assert.Equal(t, []string{"testdir/kustomization.yaml"}, files)
 	})
@@ -167,7 +194,7 @@ patches:
 			"testdir/patch.yaml": &fstest.MapFile{},
 		}
 
-		files, _, err := processDir(sourceFS, "testdir")
+		files, _, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		require.NoError(t, err)
 		assert.Contains(t, files, "testdir/patch.yaml")
 	})
@@ -184,7 +211,7 @@ patchesJson6902:
 			"testdir/patch.yaml": &fstest.MapFile{},
 		}
 
-		files, _, err := processDir(sourceFS, "testdir")
+		files, _, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		require.NoError(t, err)
 		assert.Contains(t, files, "testdir/patch.yaml")
 	})
@@ -200,10 +227,11 @@ resources:
 			},
 		}
 
-		_, _, err := processDir(sourceFS, "testdir")
+		_, _, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		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:
@@ -229,45 +257,8 @@ dummy:
 			},
 		}
 
-		files, _, err := processDir(sourceFS, "testdir")
+		files, _, err := ProcessKustomizationFile(sourceFS, filepath.Join("testdir", "kustomization.yaml"))
 		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. Function Name Typo: There is a typo in the function name ProcessKustomizationFile call in the "OpenError" test case. It should be kustomization.yaml instead of kustomation.yaml. This will lead to a file not found error. 🐛

2. Removed Test Function: The Test_getValuesFromKustomizationHelm function has been removed. If this was intentional, ensure that the functionality it was testing is no longer needed. If it was removed by mistake, consider re-adding it to maintain test coverage. 🧪

3. Import Cleanup: The removal of the types import is fine if it's not used directly in the code. However, ensure that the _ "sigs.k8s.io/kustomize/api/types" import is necessary for side effects; otherwise, it can be removed to clean up the imports. 📦

4. Test Case Addition: The addition of the "relative components" test case is a good enhancement to cover more scenarios. Ensure that all edge cases are considered for relative paths. 🧩

5. Directory Assertions: The change from assert.Empty(t, dirs) to assert.Equal(t, []string{"testdir"}, dirs) in the "NoKustomization" test case is a good improvement for clarity. Ensure that this change aligns with the expected behavior of the ProcessKustomizationFile function. 📂

---

---

Dependency Review

Click to read mergecats review!

No suggestions found