Merge pull request #180 from opendevstack/fix/empty-drift

Do not calculate drift when adding empty string to missing fields

Author: Michael Sauter

internal/test/fixtures/empty-values/bc-platform.yml renamed to internal/test/fixtures/empty-values/bc-platform-defaulted.yml

@@ -0,0 +1,26 @@
+kind: BuildConfig
+apiVersion: v1
+metadata:
+  name: foo
+  labels:
+    app: foo
+spec:
+  nodeSelector: null
+  postCommit: {}
+  resources: {}
+  runPolicy: Serial
+  triggers: []
+  source:
+    binary: {}
+    type: Binary
+  strategy:
+    type: Docker
+    dockerStrategy:
+      env:
+        - name: FOO_BAR
+        - name: BAZ
+          value: qux
+  output:
+    to:
+      kind: ImageStreamTag
+      name: foo:latest

internal/test/fixtures/empty-values/bc-platform-missing-env.yml

@@ -0,0 +1,27 @@
+kind: BuildConfig
+apiVersion: v1
+metadata:
+  name: foo
+  labels:
+    app: foo
+spec:
+  nodeSelector: null
+  postCommit: {}
+  resources: {}
+  runPolicy: Serial
+  triggers: []
+  source:
+    binary: {}
+    type: Binary
+  strategy:
+    type: Docker
+    dockerStrategy:
+      env:
+        - name: FOO_BAR
+          value: ""
+        - name: BAZ
+          value: qux
+  output:
+    to:
+      kind: ImageStreamTag
+      name: foo:latest

internal/test/fixtures/empty-values/bc-template.yml renamed to internal/test/fixtures/empty-values/bc-template-defaulted.yml

@@ -160,7 +160,19 @@ func calculateChanges(templateItem *ResourceItem, platformItem *ResourceItem, pr
 
 			}
 			comparedPaths[path] = true
-			addedPaths = append(addedPaths, path)
+
+			// OpenShift sometimes removes the whole field when the value is an
+			// empty string. Therefore, we do not want to add the path in that
+			// case, otherwise we would cause endless drift. See
+			// https://github.com/opendevstack/tailor/issues/157.
+			if v, ok := templateItemVal.(string); ok && len(v) == 0 {
+				_, err := pathPointer.Delete(templateItem.Config)
+				if err != nil {
+					return nil, err
+				}
+			} else {
+				addedPaths = append(addedPaths, path)
+			}
 		} else {
 			// Pointer exists in both items
 			switch templateItemVal.(type) {

internal/test/fixtures/empty-values/bc-template-empty-env.yml

@@ -256,19 +256,40 @@ func TestCalculateChangesOmittedFields(t *testing.T) {
 }
 
 func TestEmptyValuesDoNotCauseDrift(t *testing.T) {
-	platformItem := getPlatformItem(t, "empty-values/bc-platform.yml")
-	templateItem := getTemplateItem(t, "empty-values/bc-template.yml")
-	changes, err := calculateChanges(templateItem, platformItem, []string{}, true)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(changes) != 1 {
-		t.Fatalf("Expected 1 change, got: %d", len(changes))
+
+	tests := map[string]struct {
+		platformFixture string
+		templateFixture string
+		expectedAction  string
+	}{
+		"Field not defined in template": {
+			platformFixture: "bc-platform-defaulted.yml",
+			templateFixture: "bc-template-defaulted.yml",
+			expectedAction:  "Noop",
+		},
+		"Field not set in platform, and empty in template": {
+			platformFixture: "bc-platform-missing-env.yml",
+			templateFixture: "bc-template-empty-env.yml",
+			expectedAction:  "Noop",
+		},
 	}
-	actualChange := changes[0]
-	expectedAction := "Noop"
-	if actualChange.Action != expectedAction {
-		t.Fatalf("Expected change action to be: %s, got: %s. Diff was: %s", expectedAction, actualChange.Action, actualChange.Diff(false))
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			platformItem := getPlatformItem(t, "empty-values/"+tc.platformFixture)
+			templateItem := getTemplateItem(t, "empty-values/"+tc.templateFixture)
+			changes, err := calculateChanges(templateItem, platformItem, []string{}, true)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if len(changes) != 1 {
+				t.Fatalf("Expected 1 change, got: %d", len(changes))
+			}
+			actualChange := changes[0]
+			if actualChange.Action != tc.expectedAction {
+				t.Fatalf("Expected change action to be: %s, got: %s. Diff was: %s", tc.expectedAction, actualChange.Action, actualChange.Diff(false))
+			}
+		})
 	}
 }