Add more tests and explain some strange quotes.

api/internal/target/kusttarget_test.go

@@ -244,7 +244,7 @@ metadata:
 			t.Fatalf("unexpected error %v", err)
 		}
 	}
-	assert.NoError(t, expected.RemoveIdAnnotations())
+	expected.RemoveIdAnnotations()
 	expYaml, err := expected.AsYaml()
 	assert.NoError(t, err)
 
@@ -252,7 +252,7 @@ metadata:
 	assert.NoError(t, kt.Load())
 	actual, err := kt.MakeCustomizedResMap()
 	assert.NoError(t, err)
-	assert.NoError(t, actual.RemoveIdAnnotations())
+	actual.RemoveIdAnnotations()
 	actYaml, err := actual.AsYaml()
 	assert.NoError(t, err)
 	assert.Equal(t, expYaml, actYaml)

api/krusty/basic_io_test.go

@@ -4,13 +4,33 @@
 package krusty_test
 
 import (
+	"fmt"
 	"testing"
 
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
-func TestBasicIO1(t *testing.T) {
+func TestBasicIO_1(t *testing.T) {
 	th := kusttest_test.MakeHarness(t)
+	opts := th.MakeDefaultOptions()
+	if !opts.UseKyaml {
+		// This test won't pass under apimachinery, because in the bowels of
+		// that code (see GetAnnotations in v0.17.0 of
+		// k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go)
+		// an error returned from NestedStringMap is discarded, and an
+		// empty annotation map is silently returned, making this test fail
+		// The swallowed error arises from code like:
+		//   var v interface{}
+		//   v = true
+		//   if str, ok := v.(string); ok {
+		//     save the value in a map (doesn't happen)
+		//   } else {
+		//     return an error (that is then ignored by GetAnnotations)
+		//   }
+		// The error happens when any annotation value can be interpreted as
+		// a boolean or number.
+		t.SkipNow()
+	}
 	th.WriteK(".", `
 resources:
 - service.yaml
@@ -27,7 +47,8 @@ metadata:
 spec:
   clusterIP: None
 `)
-	m := th.Run(".", th.MakeDefaultOptions())
+	m := th.Run(".", opts)
+	// The annotations are sorted by key, hence the order change.
 	th.AssertActualEqualsExpected(
 		m, `
 apiVersion: v1
@@ -42,3 +63,45 @@ spec:
   clusterIP: None
 `)
 }
+
+func TestBasicIO_2(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	opts := th.MakeDefaultOptions()
+	th.WriteK(".", `
+resources:
+- service.yaml
+`)
+	// All the annotation values are quoted.
+	// The apimachinery code path retains the quotes, the kyaml
+	// path drops them at the moment.
+	th.WriteF("service.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  annotations:
+    port: "8080"
+    happy: "true"
+    color: green
+  name: demo
+spec:
+  clusterIP: None
+`)
+	m := th.Run(".", opts)
+	// The annotations are sorted by key, hence the order change.
+	expFmt := `
+apiVersion: v1
+kind: Service
+metadata:
+  annotations:
+    color: green
+    happy: %s
+    port: %s
+  name: demo
+spec:
+  clusterIP: None
+`
+	th.AssertActualEqualsExpected(
+		m, opts.IfApiMachineryElseKyaml(
+			fmt.Sprintf(expFmt, `"true"`, `"8080"`),
+			fmt.Sprintf(expFmt, `true`, `8080`)))
+}

api/krusty/configmaps_test.go

@@ -27,10 +27,6 @@ configMapGenerator:
 apiVersion: v1
 kind: Service
 metadata:
-  annotations:
-    port: 8080
-    happy: true
-    color: green
   name: demo
 spec:
   clusterIP: None
@@ -41,10 +37,6 @@ spec:
 apiVersion: v1
 kind: Service
 metadata:
-  annotations:
-    color: green
-    happy: true
-    port: 8080
   name: demo
 spec:
   clusterIP: None

api/krusty/kustomizer.go

@@ -93,9 +93,6 @@ func (b *Kustomizer) Run(path string) (resmap.ResMap, error) {
 		}
 		t.Transform(m)
 	}
-	err = m.RemoveIdAnnotations()
-	if err != nil {
-		return nil, err
-	}
+	m.RemoveIdAnnotations()
 	return m, nil
 }

api/krusty/variableref_test.go

@@ -4,6 +4,7 @@
 package krusty_test
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -360,7 +361,7 @@ resources:
 	}
 }
 
-// TODO: varref has some quote issues
+// TODO(#3449): varref has some quote issues
 // https://github.com/kubernetes-sigs/kustomize/issues/3449
 func TestVarRefBig(t *testing.T) {
 	th := kusttest_test.MakeHarness(t)
@@ -682,7 +683,7 @@ resources:
 - ../../base
 `)
 	m := th.Run("/app/overlay/staging", opts)
-	th.AssertActualEqualsExpected(m, `
+	expFmt := `
 apiVersion: v1
 kind: ServiceAccount
 metadata:
@@ -756,9 +757,9 @@ kind: Service
 metadata:
   annotations:
     prometheus.io/path: _status/vars
-    prometheus.io/port: 8080
-    prometheus.io/scrape: true
-    service.alpha.kubernetes.io/tolerate-unready-endpoints: true
+    prometheus.io/port: %s
+    prometheus.io/scrape: %s
+    service.alpha.kubernetes.io/tolerate-unready-endpoints: %s
   labels:
     app: cockroachdb
   name: dev-base-cockroachdb
@@ -769,8 +770,8 @@ spec:
     port: 26257
     targetPort: 26257
   - name: http
-    port: "8080"
-    targetPort: "8080"
+    port: %s
+    targetPort: %s
   selector:
     app: cockroachdb
 ---
@@ -786,8 +787,8 @@ spec:
     port: 26257
     targetPort: 26257
   - name: http
-    port: "8080"
-    targetPort: "8080"
+    port: %s
+    targetPort: %s
   selector:
     app: cockroachdb
 ---
@@ -824,8 +825,8 @@ spec:
         - --host $(hostname -f)
         - --http-host 0.0.0.0
         - --join dev-base-cockroachdb-0.dev-base-cockroachdb,dev-base-cockroachdb-1.dev-base-cockroachdb,dev-base-cockroachdb-2.dev-base-cockroachdb
-        - --cache 25%
-        - --max-sql-memory 25%
+        - --cache 25%%
+        - --max-sql-memory 25%%
         image: cockroachdb/cockroach:v1.1.5
         imagePullPolicy: IfNotPresent
         name: cockroachdb
@@ -925,7 +926,16 @@ data:
 kind: ConfigMap
 metadata:
   name: dev-base-test-config-map-6b85g79g7g
-`)
+`
+	th.AssertActualEqualsExpected(m,
+		opts.IfApiMachineryElseKyaml(
+			fmt.Sprintf(
+				expFmt,
+				`"8080"`, `"true"`, `"true"`, `8080`, `8080`, `8080`, `8080`),
+			fmt.Sprintf(
+				expFmt,
+				`8080`, `true`, `true`, `"8080"`, `"8080"`, `"8080"`, `"8080"`),
+		))
 }
 
 func TestVariableRefIngressBasic(t *testing.T) {

api/resmap/resmap.go

@@ -246,5 +246,6 @@ type ResMap interface {
 	ApplySmPatch(
 		selectedSet *resource.IdSet, patch *resource.Resource) error
 
-	RemoveIdAnnotations() error
+	// Remove annotations used exclusively by the kustomize build process.
+	RemoveIdAnnotations()
 }

api/resmap/reswrangler.go

@@ -621,12 +621,8 @@ func (m *resWrangler) ApplySmPatch(
 	return nil
 }
 
-func (m *resWrangler) RemoveIdAnnotations() error {
+func (m *resWrangler) RemoveIdAnnotations() {
 	for _, r := range m.Resources() {
-		err := r.RemoveIdAnnotations()
-		if err != nil {
-			return err
-		}
+		r.RemoveIdAnnotations()
 	}
-	return nil
 }

api/resmap/reswrangler_test.go

@@ -998,7 +998,7 @@ spec:
 				return
 			}
 			assert.False(t, tc.errorExpected)
-			assert.NoError(t, m.RemoveIdAnnotations())
+			m.RemoveIdAnnotations()
 			yml, err := m.AsYaml()
 			assert.NoError(t, err)
 			assert.Equal(t, strings.Join(tc.expected, "---\n"), string(yml))
@@ -1111,7 +1111,7 @@ $patch: delete
 			assert.NoError(t, err, name)
 			assert.NoError(t, m.ApplySmPatch(idSet, p), name)
 			assert.Equal(t, tc.finalMapSize, m.Size(), name)
-			assert.NoError(t, m.RemoveIdAnnotations())
+			m.RemoveIdAnnotations()
 			yml, err := m.AsYaml()
 			assert.NoError(t, err, name)
 			assert.Equal(t, tc.expected, string(yml), name)

api/resource/resource.go

@@ -244,28 +244,26 @@ func copyStringSlice(s []string) []string {
 
 // Implements ResCtx AddNamePrefix
 func (r *Resource) AddNamePrefix(p string) {
-	annotations := r.GetAnnotations()
-	if annotations == nil {
-		annotations = make(map[string]string)
-	}
-	if _, ok := annotations[prefixAnnotation]; !ok {
-		annotations[prefixAnnotation] = p
-	} else {
-		annotations[prefixAnnotation] = annotations[prefixAnnotation] + "," + p
-	}
-	r.SetAnnotations(annotations)
+	r.addAdditiveAnnotation(prefixAnnotation, p)
 }
 
 // Implements ResCtx AddNameSuffix
 func (r *Resource) AddNameSuffix(s string) {
+	r.addAdditiveAnnotation(suffixAnnotation, s)
+}
+
+func (r *Resource) addAdditiveAnnotation(name, value string) {
+	if value == "" {
+		return
+	}
 	annotations := r.GetAnnotations()
 	if annotations == nil {
 		annotations = make(map[string]string)
 	}
-	if _, ok := annotations[suffixAnnotation]; !ok {
-		annotations[suffixAnnotation] = s
+	if existing, ok := annotations[name]; ok {
+		annotations[name] = existing + "," + value
 	} else {
-		annotations[suffixAnnotation] = annotations[suffixAnnotation] + "," + s
+		annotations[name] = value
 	}
 	r.SetAnnotations(annotations)
 }
@@ -347,50 +345,16 @@ func (r *Resource) PrefixesSuffixesEquals(o ResCtx) bool {
 	return sameEndingSubarray(r.GetNamePrefixes(), o.GetNamePrefixes()) && sameEndingSubarray(r.GetNameSuffixes(), o.GetNameSuffixes())
 }
 
-func (r *Resource) RemoveIdAnnotations() error {
+func (r *Resource) RemoveIdAnnotations() {
 	annotations := r.GetAnnotations()
-	if annotations == nil {
-		annotations = make(map[string]string)
+	if len(annotations) == 0 {
+		return
 	}
 	delete(annotations, nameAnnotation)
 	delete(annotations, prefixAnnotation)
 	delete(annotations, suffixAnnotation)
 	delete(annotations, namespaceAnnotation)
-	if len(annotations) == 0 {
-		json, err := r.MarshalJSON()
-		if err != nil {
-			return err
-		}
-
-		yml, err := yaml.JSONToYAML(json)
-		if err != nil {
-			return err
-		}
-
-		rnode, err := kyaml.Parse(string(yml))
-		if err != nil {
-			return err
-		}
-
-		err = rnode.SetAnnotations(nil)
-		if err != nil {
-			return err
-		}
-
-		b, err := rnode.MarshalJSON()
-		if err != nil {
-			return err
-		}
-
-		err = r.UnmarshalJSON(b)
-		if err != nil {
-			return err
-		}
-		return nil
-	}
-
 	r.SetAnnotations(annotations)
-	return nil
 }
 
 func (r *Resource) GetOriginalName() string {

api/testutils/kusttest/harness.go

@@ -127,10 +127,7 @@ func (th Harness) AssertActualEqualsExpected(
 }
 
 func (th Harness) AssertActualEqualsExpectedNoIdAnnotations(m resmap.ResMap, expected string) {
-	err := m.RemoveIdAnnotations()
-	if err != nil {
-		th.t.Fatalf("Err: %v", err)
-	}
+	m.RemoveIdAnnotations()
 	th.AssertActualEqualsExpectedWithTweak(m, nil, expected)
 }
 

plugin/builtin/prefixsuffixtransformer/PrefixSuffixTransformer_test.go

@@ -137,7 +137,6 @@ metadata:
   annotations:
     config.kubernetes.io/originalName: deployment
     config.kubernetes.io/prefixes: test-
-    config.kubernetes.io/suffixes: null
   name: test-deployment
 spec:
   template: