Rename CopyConfigMapWithForceUpdate to CopyConfigMap

This patch renames CopyConfigMapWithForceUpdate to CopyConfigMap and removes `forceUpdate` param and always allow to add extra fields in ConfigMap Fixes: knative#1568 Signed-off-by: Shiv Verma <[email protected]>
katanomi · Nov 13, 2023 · a698748 · a698748
1 parent 628ca86
commit a698748
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 34 deletions.
diff --git a/pkg/reconciler/common/testdata/test-empty-config.yaml b/pkg/reconciler/common/testdata/test-empty-config.yaml
@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: empty-config
+  namespace: tekton-pipelines-resolvers
+  labels:
+    app.kubernetes.io/component: resolvers
+    app.kubernetes.io/instance: default
+    app.kubernetes.io/part-of: tekton-pipelines
+data: {}
diff --git a/pkg/reconciler/common/transformers.go b/pkg/reconciler/common/transformers.go
@@ -767,16 +767,15 @@ func AddJobRestrictedPSA() mf.Transformer {
 	}
 }
 
-// CopyConfigMapWithForceUpdate will copy all the values from the passed configmap to the configmap
-// in the manifest, the fields which are in manifest configmap will only be copied
-// any extra fields will be copied only if the "forceUpdate" is true
-func CopyConfigMapWithForceUpdate(configMapName string, expectedValues map[string]string, forceUpdate bool) mf.Transformer {
+// CopyConfigMap will copy all the values from the passed configmap to the configmap
+// in the manifest and any extra fields will be added in the manifest
+func CopyConfigMap(configMapName string, expectedValues map[string]string) mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
 		kind := strings.ToLower(u.GetKind())
 		if kind != "configmap" {
 			return nil
 		}
-		if u.GetName() != configMapName {
+		if u.GetName() != configMapName || len(expectedValues) == 0 {
 			return nil
 		}
 
@@ -785,19 +784,15 @@ func CopyConfigMapWithForceUpdate(configMapName string, expectedValues map[strin
 		if err != nil {
 			return err
 		}
-		if cm.Data == nil && !forceUpdate {
-			// we don't add any field in the manifest configmap, if force update is disabled
-			// we will copy any value if defined by user
-			return nil
+
+		if cm.Data == nil {
+			cm.Data = map[string]string{}
 		}
 
 		for key, value := range expectedValues {
-			// check if the key is defined in the config map
-			_, ok := cm.Data[key]
-			// updates values, if the key found or forceUpdate is enabled
-			if ok || forceUpdate {
-				cm.Data[key] = value
-			}
+			// updates values , if the key is found,
+			// adds key and value, if the key is not found
+			cm.Data[key] = value
 		}
 		unstrObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(cm)
 		if err != nil {
@@ -809,13 +804,6 @@ func CopyConfigMapWithForceUpdate(configMapName string, expectedValues map[strin
 	}
 }
 
-// CopyConfigMap will copy all the values from the passed configmap to the configmap
-// in the manifest, the fields which are in manifest configmap will only be copied
-// any extra field will be ignored
-func CopyConfigMap(configMapName string, expectedValues map[string]string) mf.Transformer {
-	return CopyConfigMapWithForceUpdate(configMapName, expectedValues, false)
-}
-
 func ReplaceDeploymentArg(deploymentName, existingArg, newArg string) mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
 		if u.GetKind() != "Deployment" {

diff --git a/pkg/reconciler/common/transformers_test.go b/pkg/reconciler/common/transformers_test.go
@@ -635,11 +635,64 @@ func TestCopyConfigMapValues(t *testing.T) {
 	assert.Equal(t, cm.Data["default-kind"], "task")
 	assert.Equal(t, cm.Data["default-type"], "artifact")
 
-	// extra fields in expected configmap will be ignore and will not be added
-	assert.Equal(t, cm.Data["ignore-me-field"], "")
+	// extra fields in expected configmap will be added
+	assert.Equal(t, cm.Data["ignore-me-field"], "ignore-me")
 
 }
 
+func TestCopyEmptyConfigMapValues(t *testing.T) {
+	testData := path.Join("testdata", "test-empty-config.yaml")
+	manifest, err := mf.ManifestFrom(mf.Recursive(testData))
+	assertNoEror(t, err)
+
+	expectedValues := map[string]string{
+		"default-tekton-hub-catalog":        "abc-catalog",
+		"default-artifact-hub-task-catalog": "some-random-catalog",
+		"ignore-me-field":                   "ignore-me",
+	}
+
+	manifest, err = manifest.Transform(CopyConfigMap("empty-config", expectedValues))
+	assertNoEror(t, err)
+
+	cm := &corev1.ConfigMap{}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(manifest.Resources()[0].Object, cm)
+	assertNoEror(t, err)
+	// ConfigMap will have all expected values
+	assert.Equal(t, cm.Data["default-tekton-hub-catalog"], "abc-catalog")
+	assert.Equal(t, cm.Data["default-artifact-hub-task-catalog"], "some-random-catalog")
+	assert.Equal(t, cm.Data["ignore-me-field"], "ignore-me")
+}
+
+func TestCopyConfigMapWithEmptyExpectedValues(t *testing.T) {
+	testData := path.Join("testdata", "test-empty-config.yaml")
+	manifest, err := mf.ManifestFrom(mf.Recursive(testData))
+	assertNoEror(t, err)
+
+	expectedValues := map[string]string{}
+
+	manifest, err = manifest.Transform(CopyConfigMap("empty-config", expectedValues))
+	assertNoEror(t, err)
+
+	cm := &corev1.ConfigMap{}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(manifest.Resources()[0].Object, cm)
+	assertNoEror(t, err)
+}
+
+func TestCopyConfigMapWithWrongKind(t *testing.T) {
+	testData := path.Join("testdata", "test-namespace-inject.yaml")
+	manifest, err := mf.ManifestFrom(mf.Recursive(testData))
+	assertNoEror(t, err)
+
+	expectedValues := map[string]string{}
+
+	manifest, err = manifest.Transform(CopyConfigMap("tekton-pipelines", expectedValues))
+	assertNoEror(t, err)
+
+	cm := &corev1.ConfigMap{}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(manifest.Resources()[0].Object, cm)
+	assertNoEror(t, err)
+}
+
 func TestReplaceDeploymentArg(t *testing.T) {
 	testData := path.Join("testdata", "test-dashboard-deployment.yaml")
 	manifest, err := mf.ManifestFrom(mf.Recursive(testData))
@@ -707,23 +760,20 @@ func TestReplaceNamespaceInClusterRoleBinding(t *testing.T) {
 	}
 }
 
-func TestCopyConfigMapWithForceUpdate(t *testing.T) {
+func TestCopyConfigMap(t *testing.T) {
 	tests := []struct {
-		name        string
-		forceUpdate bool
-		data        map[string]string
+		name string
+		data map[string]string
 	}{
 		{
-			name:        "force-update-disabled",
-			forceUpdate: false,
+			name: "force-update-disabled",
 			data: map[string]string{
 				"default-tekton-hub-catalog": "foo",
 				"default-kind":               "pipeline",
 			},
 		},
 		{
-			name:        "force-update-enabled",
-			forceUpdate: true,
+			name: "force-update-enabled",
 			data: map[string]string{
 				"default-tekton-hub-catalog": "foo",
 				"default-kind":               "pipeline",
@@ -741,7 +791,7 @@ func TestCopyConfigMapWithForceUpdate(t *testing.T) {
 			manifest, err := mf.ManifestFrom(mf.Recursive(testData))
 			assert.NilError(t, err)
 
-			manifest, err = manifest.Transform(CopyConfigMapWithForceUpdate(configMapName, test.data, test.forceUpdate))
+			manifest, err = manifest.Transform(CopyConfigMap(configMapName, test.data))
 			assert.NilError(t, err)
 
 			cm := &corev1.ConfigMap{}

diff --git a/pkg/reconciler/openshift/openshiftpipelinesascode/transform.go b/pkg/reconciler/openshift/openshiftpipelinesascode/transform.go
@@ -44,7 +44,7 @@ func filterAndTransform(extension common.Extension) client.FilterAndTransform {
 			common.DeploymentImages(images),
 			common.AddConfiguration(pac.Spec.Config),
 			occommon.ApplyCABundles,
-			common.CopyConfigMapWithForceUpdate(pipelinesAsCodeCM, pac.Spec.Settings, true),
+			common.CopyConfigMap(pipelinesAsCodeCM, pac.Spec.Settings),
 			occommon.UpdateServiceMonitorTargetNamespace(pac.Spec.TargetNamespace),
 		}