Address review comments

Author: LucaBernstein

docs/api-reference/landscapekit-v1alpha1.md

@@ -81,6 +81,23 @@ _Appears in:_
 
 
 
+#### MergeMode
+
+_Underlying type:_ _string_
+
+MergeMode controls how operator overwrites are handled during three-way merge.
+
+
+
+_Appears in:_
+- [LandscapeKitConfiguration](#landscapekitconfiguration)
+
+| Field | Description |
+| --- | --- |
+| `Informative` | MergeModeInformative annotates operator-overwritten values with a comment showing the current GLK default.<br /> |
+| `Silent` | MergeModeSilent retains operator overwrites without annotation.<br /> |
+
+
 #### OCMComponent
 
 

example/20-componentconfig-glk.yaml

@@ -19,3 +19,4 @@ kind: LandscapeKitConfiguration
 #   - component-name
 # versionConfig:
 #   defaultVersionsUpdateStrategy: ReleaseBranch
+# mergeMode: Informative

pkg/apis/config/v1alpha1/types.go

@@ -26,8 +26,9 @@ type LandscapeKitConfiguration struct {
 	// VersionConfig is the configuration for versioning.
 	// +optional
 	VersionConfig *VersionConfiguration `json:"versionConfig,omitempty"`
-	// MergeMode controls how operator overrides conflicting with updated GLK defaults are handled during three-way merge.
-	// Possible values are "Informative" (default) and "Silent".
+	// MergeMode determines how merge conflicts are resolved:
+	// - "Informative" (default): New default values from GLK are added as comments after any customized values.
+	// - "Silent": Operator-customized values are retained, new default values are omitted.
 	// +optional
 	MergeMode *MergeMode `json:"mergeMode,omitempty"`
 }
@@ -124,13 +125,13 @@ type VersionConfiguration struct {
 	DefaultVersionsUpdateStrategy *DefaultVersionsUpdateStrategy `json:"defaultVersionsUpdateStrategy,omitempty"`
 }
 
-// MergeMode controls how operator overrides are handled during three-way merge.
+// MergeMode controls how operator overwrites are handled during three-way merge.
 type MergeMode string
 
 const (
-	// MergeModeInformative annotates operator-overridden values with a comment showing the current GLK default.
+	// MergeModeInformative annotates operator-overwritten values with a comment showing the current GLK default.
 	MergeModeInformative MergeMode = "Informative"
-	// MergeModeSilent retains operator overrides without annotation.
+	// MergeModeSilent retains operator overwrites without annotation.
 	MergeModeSilent MergeMode = "Silent"
 )
 
@@ -141,10 +142,9 @@ var AllowedMergeModes = []string{
 }
 
 // GetMergeMode returns the configured MergeMode, defaulting to MergeModeInformative.
-// It is safe to call on a nil receiver.
 func (c *LandscapeKitConfiguration) GetMergeMode() MergeMode {
-	if c != nil && c.MergeMode != nil {
-		return *c.MergeMode
+	if c == nil || c.MergeMode == nil {
+		return MergeModeInformative
 	}
-	return MergeModeInformative
+	return *c.MergeMode
 }

pkg/apis/config/v1alpha1/validation/validation.go

@@ -37,7 +37,7 @@ func ValidateLandscapeKitConfiguration(conf *configv1alpha1.LandscapeKitConfigur
 	}
 
 	if conf.MergeMode != nil && !slices.Contains(configv1alpha1.AllowedMergeModes, string(*conf.MergeMode)) {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("mergeMode"), *conf.MergeMode, "allowed values are: "+strings.Join(configv1alpha1.AllowedMergeModes, ", ")))
+		allErrs = append(allErrs, field.NotSupported(field.NewPath("mergeMode"), *conf.MergeMode, configv1alpha1.AllowedMergeModes))
 	}
 
 	return allErrs

pkg/apis/config/v1alpha1/validation/validation_test.go

@@ -324,7 +324,7 @@ var _ = Describe("Validation", func() {
 				errList := validation.ValidateLandscapeKitConfiguration(conf)
 				Expect(errList).To(ConsistOf(
 					PointTo(MatchFields(IgnoreExtras, Fields{
-						"Type":     Equal(field.ErrorTypeInvalid),
+						"Type":     Equal(field.ErrorTypeNotSupported),
 						"Field":    Equal("mergeMode"),
 						"BadValue": Equal(invalid),
 					})),

pkg/components/types.go

@@ -36,7 +36,7 @@ type Options interface {
 	GetFilesystem() afero.Afero
 	// GetLogger returns the logger instance.
 	GetLogger() logr.Logger
-	// GetMergeMode returns the configured merge mode for three-way merges.
+	// GetMergeMode returns the configured mode to solve merge conflicts.
 	GetMergeMode() configv1alpha1.MergeMode
 }
 

pkg/utils/files/writer.go

@@ -10,7 +10,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"strings"
 
 	"github.com/spf13/afero"
 
@@ -46,30 +45,6 @@ func isSecret(contents []byte) bool {
 	return bytes.HasPrefix(contents, []byte(kindSecret)) || bytes.Contains(contents, []byte("\n"+kindSecret))
 }
 
-// stripGLKManagedAnnotations removes GLK-managed annotation comments from YAML bytes.
-// A line is annotated when it contains both GLKDefaultPrefix and GLKManagedMarker.
-func stripGLKManagedAnnotations(data []byte) []byte {
-	lines := strings.Split(string(data), "\n")
-	out := make([]string, 0, len(lines))
-	for _, line := range lines {
-		if !strings.Contains(line, meta.GLKManagedMarker) {
-			out = append(out, line)
-			continue
-		}
-		// Strip from the start of the GLK annotation prefix.
-		if idx := strings.Index(line, meta.GLKDefaultPrefix); idx >= 0 {
-			stripped := strings.TrimRight(line[:idx], " \t")
-			// If the entire line was a head comment annotation, drop the line entirely.
-			if strings.TrimSpace(stripped) == "" {
-				continue
-			}
-			out = append(out, stripped)
-		}
-		// If for some reason the marker is present but the prefix is not, drop the line.
-	}
-	return []byte(strings.Join(out, "\n"))
-}
-
 // WriteObjectsToFilesystem writes the given objects to the filesystem at the specified rootDir and relativeFilePath.
 // If the manifest file already exists, it patches changes from the new default.
 // Additionally, it maintains a default version of the manifest in a separate directory for future diff checks.
@@ -107,9 +82,6 @@ func WriteObjectsToFilesystem(objects map[string][]byte, rootDir, relativeFilePa
 			object = append([]byte(secretEncryptionDisclaimer), object...)
 		}
 
-		// Strip GLK-managed annotations from the current file before merging, so they are always re-evaluated (idempotency: the annotation reflects the *current* GLK default).
-		currentYaml = stripGLKManagedAnnotations(currentYaml)
-
 		output, err := meta.ThreeWayMergeManifest(oldDefaultYaml, object, currentYaml, mode)
 		if err != nil {
 			return err

pkg/utils/files/writer_test.go

@@ -140,102 +140,168 @@ spec:
 				Not(ContainSubstring(`# SECURITY ADVISORY`)),
 			))
 		})
-	})
 
-	Describe("#WriteObjectsToFilesystem - MergeModeInformative", func() {
-		It("should annotate operator-overridden scalar values with the GLK default and preserve user comments idempotently", func() {
-			initial := []byte(`apiVersion: v1
+		DescribeTable("should annotate operator-overwritten values only in Informative mode",
+			func(mode configv1alpha1.MergeMode, expectAnnotation bool) {
+				initial := []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.0.0
 `)
-			// First generate: establish defaults
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": initial}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": initial}, "/landscape", "manifest", fs, mode)).To(Succeed())
 
-			// Operator pins to a custom version with a comment explaining why
-			Expect(fs.WriteFile("/landscape/manifest/test.yaml", []byte(`apiVersion: v1
+				// Operator pins to a custom version with a comment explaining why
+				Expect(fs.WriteFile("/landscape/manifest/test.yaml", []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.0.5 # pinned for production
 `), 0600)).To(Succeed())
 
-			// GLK ships a new default with a newer version
-			updated := []byte(`apiVersion: v1
+				// GLK ships a new default with a newer version
+				updated := []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.1.0
 `)
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "manifest", fs, mode)).To(Succeed())
+
+				content, err := fs.ReadFile("/landscape/manifest/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).To(ContainSubstring("version: v1.0.5"))
+				Expect(string(content)).To(ContainSubstring("pinned for production"))
+
+				if expectAnnotation {
+					Expect(string(content)).To(ContainSubstring(meta.GLKDefaultPrefix + "v1.1.0"))
+
+					// Re-run with the same default — annotation persists because the user did not remove it.
+					Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "manifest", fs, mode)).To(Succeed())
+					content2, err := fs.ReadFile("/landscape/manifest/test.yaml")
+					Expect(err).NotTo(HaveOccurred())
+					Expect(string(content2)).To(Equal(string(content)))
+				} else {
+					Expect(string(content)).NotTo(ContainSubstring(meta.GLKDefaultPrefix))
+				}
+			},
+			Entry("Silent", configv1alpha1.MergeModeSilent, false),
+			Entry("Informative", configv1alpha1.MergeModeInformative, true),
+		)
 
-			content, err := fs.ReadFile("/landscape/manifest/test.yaml")
-			Expect(err).NotTo(HaveOccurred())
-			// Operator's override is preserved
-			Expect(string(content)).To(ContainSubstring("version: v1.0.5"))
-			// User comment is preserved
-			Expect(string(content)).To(ContainSubstring("pinned for production"))
-			// GLK default annotation is added
-			Expect(string(content)).To(ContainSubstring("# glk default: v1.1.0"))
-			Expect(string(content)).To(ContainSubstring(meta.GLKManagedMarker))
-
-			// Re-run with the same inputs — annotation and user comment must not be doubled
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
-
-			content2, err := fs.ReadFile("/landscape/manifest/test.yaml")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(string(content2)).To(Equal(string(content)))
-		})
+		Context("MergeMode Informative", func() {
+			It("should not re-add the annotation after the user removed it, until the default changes again", func() {
+				initial := []byte(`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  version: v1.0.0
+`)
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": initial}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+
+				// Operator pins to v1.0.5
+				Expect(fs.WriteFile("/landscape/manifest/test.yaml", []byte(`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  version: v1.0.5
+`), 0600)).To(Succeed())
+
+				// GLK ships v1.1.0 — annotation appears
+				v110 := []byte(`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  version: v1.1.0
+`)
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": v110}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				content, err := fs.ReadFile("/landscape/manifest/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).To(ContainSubstring(meta.GLKDefaultPrefix))
+
+				// User acknowledges the annotation and removes it manually
+				Expect(fs.WriteFile("/landscape/manifest/test.yaml", []byte(`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  version: v1.0.5
+`), 0600)).To(Succeed())
+
+				// Re-run with the same default — annotation stays removed
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": v110}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				content, err = fs.ReadFile("/landscape/manifest/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).To(ContainSubstring("version: v1.0.5"))
+				Expect(string(content)).NotTo(ContainSubstring(meta.GLKDefaultPrefix))
 
-		It("should remove the annotation entirely when the GLK default reverts to the operator's value", func() {
-			initial := []byte(`apiVersion: v1
+				// GLK ships v1.2.0 — annotation re-appears because the default changed
+				v120 := []byte(`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  version: v1.2.0
+`)
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": v120}, "/landscape", "manifest", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				content, err = fs.ReadFile("/landscape/manifest/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).To(ContainSubstring("version: v1.0.5"))
+				Expect(string(content)).To(ContainSubstring(meta.GLKDefaultPrefix + "v1.2.0"))
+			})
+
+			It("should remove the annotation entirely when the GLK default reverts to the operator's value", func() {
+				initial := []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.0.0
 `)
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": initial}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": initial}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
 
-			// Operator pins to v1.0.5
-			Expect(fs.WriteFile("/landscape/revert/test.yaml", []byte(`apiVersion: v1
+				// Operator pins to v1.0.5
+				Expect(fs.WriteFile("/landscape/revert/test.yaml", []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.0.5
 `), 0600)).To(Succeed())
 
-			// GLK ships v1.1.0 — annotation appears
-			updated := []byte(`apiVersion: v1
+				// GLK ships v1.1.0 — annotation appears
+				updated := []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.1.0
 `)
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
-			content, err := fs.ReadFile("/landscape/revert/test.yaml")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(string(content)).To(ContainSubstring(meta.GLKManagedMarker))
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": updated}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				content, err := fs.ReadFile("/landscape/revert/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).To(ContainSubstring(meta.GLKDefaultPrefix))
 
-			// GLK reverts to v1.0.5 — operator's value now matches the default, no annotation
-			reverted := []byte(`apiVersion: v1
+				// GLK reverts to v1.0.5 — operator's value now matches the default, no annotation
+				reverted := []byte(`apiVersion: v1
 kind: ConfigMap
 metadata:
   name: test
 data:
   version: v1.0.5
 `)
-			Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": reverted}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
-			content, err = fs.ReadFile("/landscape/revert/test.yaml")
-			Expect(err).NotTo(HaveOccurred())
-			Expect(string(content)).NotTo(ContainSubstring(meta.GLKManagedMarker))
-			Expect(string(content)).NotTo(ContainSubstring("# glk default:"))
+				Expect(files.WriteObjectsToFilesystem(map[string][]byte{"test.yaml": reverted}, "/landscape", "revert", fs, configv1alpha1.MergeModeInformative)).To(Succeed())
+				content, err = fs.ReadFile("/landscape/revert/test.yaml")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(string(content)).NotTo(ContainSubstring(meta.GLKDefaultPrefix))
+				Expect(string(content)).NotTo(ContainSubstring("# Attention - new default:"))
+			})
 		})
 	})
 

pkg/utils/meta/diff_test.go

@@ -277,14 +277,13 @@ data:
 			result, err := meta.ThreeWayMergeManifest(oldDefault, newDefault, current, configv1alpha1.MergeModeInformative)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(string(result)).To(ContainSubstring("version: v1.0.5"))
-			Expect(string(result)).To(ContainSubstring("# glk default: v1.1.0"))
-			Expect(string(result)).To(ContainSubstring(meta.GLKManagedMarker))
+			Expect(string(result)).To(ContainSubstring("# Attention - new default: v1.1.0"))
 
 			// No conflict: operator did not change the value → new default taken silently, no annotation
 			result, err = meta.ThreeWayMergeManifest(oldDefault, newDefault, oldDefault, configv1alpha1.MergeModeInformative)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(string(result)).To(ContainSubstring("version: v1.1.0"))
-			Expect(string(result)).NotTo(ContainSubstring(meta.GLKManagedMarker))
+			Expect(string(result)).NotTo(ContainSubstring(meta.GLKDefaultPrefix))
 		})
 	})
 })