Merge pull request #3470 from monopole/annotateIssue3304

Annotate code with decisions on issue 3304 in kyaml conversion

api/internal/accumulator/refvartransformer_test.go

@@ -124,6 +124,7 @@ func TestRefVarTransformer(t *testing.T) {
 							"slice": []interface{}{5}, // noticeably *not* a []string
 						}}).ResMap(),
 			},
+			// TODO(#3304): DECISION - kyaml better; not a bug.
 			errMessage: konfig.IfApiMachineryElseKyaml(
 				`obj '{"apiVersion": "v1", "data": {"slice": [5]}, "kind": "ConfigMap", "metadata": {"name": "cm1"}}
 ' at path 'data/slice': invalid value type expect a string`,

api/krusty/configmaps_test.go

@@ -214,12 +214,16 @@ metadata:
 type: Opaque
 `
 	th.AssertActualEqualsExpected(
-		m, opts.IfApiMachineryElseKyaml(
-			fmt.Sprintf(expFmt,
+		m,
+		// TODO(#3304): DECISION - kyaml better; not a bug.
+		opts.IfApiMachineryElseKyaml(
+			fmt.Sprintf(
+				expFmt,
 				`CmdyYXZpdGF0aW9uYWwKZWxlY3Ryb21hZ25ldGljCnN0cm9uZyBudWNsZWFyCndlYWsgbnVjbGVhcgo=`,
 				`CkxpZmUgaXMgc2hvcnQuCkJ1dCB0aGUgeWVhcnMgYXJlIGxvbmcuCk5vdCB3aGlsZSB0aGUgZXZpbCBkYXlzIGNvbWUgbm90Lgo=`,
 				`ftht6hfgmb`),
-			fmt.Sprintf(expFmt, `|
+			fmt.Sprintf(
+				expFmt, `|
     CmdyYXZpdGF0aW9uYWwKZWxlY3Ryb21hZ25ldGljCnN0cm9uZyBudWNsZWFyCndlYWsgbn
     VjbGVhcgo=`, `|
     CkxpZmUgaXMgc2hvcnQuCkJ1dCB0aGUgeWVhcnMgYXJlIGxvbmcuCk5vdCB3aGlsZSB0aG

api/krusty/multiplepatch_test.go

@@ -318,8 +318,9 @@ spec:
       volumes:%s
         name: nginx-persistent-storage
 `
-	// TODO(#3394)
 	th.AssertActualEqualsExpected(
+		// TODO(#3394)
+		// TODO(#3304): DECISION - still a bug, emptyDir should be deleted.
 		m, opts.IfApiMachineryElseKyaml(
 			fmt.Sprintf(expFmt, `
       - gcePersistentDisk:

api/krusty/poddisruptionbudget_test.go

@@ -121,7 +121,14 @@ spec:
 	// minAvailable are mutually exclusive, and both can hold
 	// either an integer, i.e. 10, or string that has to be
 	// an int followed by a percent sign, e.g. 10%.
-	th.AssertActualEqualsExpected(m, opts.IfApiMachineryElseKyaml(
-		fmt.Sprintf(expFmt, `"1"`, `"1"`),
-		fmt.Sprintf(expFmt, `1`, `1`)))
+	// In the former case - bare integer - they should NOT be quoted
+	// as the api server will reject it.  In the latter case with
+	// the percent sign, quotes can be added and the API server will
+	// accept it, but they don't have to be added.
+	th.AssertActualEqualsExpected(
+		m,
+		// TODO(#3304): DECISION - kyaml better; not a bug.
+		opts.IfApiMachineryElseKyaml(
+			fmt.Sprintf(expFmt, `"1"`, `"1"`),
+			fmt.Sprintf(expFmt, `1`, `1`)))
 }

api/krusty/variableref_test.go

@@ -361,6 +361,86 @@ resources:
 	}
 }
 
+func TestSimpleServicePortVarReplace(t *testing.T) {
+	th := kusttest_test.MakeHarness(t)
+	th.WriteK(".", `
+resources:
+- service.yaml
+- statefulset.yaml
+vars:
+ - name: THE_PORT
+   objref:
+     kind: StatefulSet
+     name: cockroachdb
+     apiVersion: apps/v1beta1
+   fieldref:
+     fieldpath: spec.template.spec.containers[0].ports[1].containerPort
+`)
+	th.WriteF("service.yaml", `
+apiVersion: v1
+kind: Service
+metadata:
+  name: myService
+spec:
+  ports:
+  - port: $(THE_PORT)
+    targetPort: $(THE_PORT)
+    name: grpc
+`)
+	th.WriteF("statefulset.yaml", `
+apiVersion: apps/v1beta1
+kind: StatefulSet
+metadata:
+  name: cockroachdb
+spec:
+  template:
+    spec:
+      containers:
+      - name: cockroachdb
+        image: cockroachdb/cockroach:v1.1.5
+        ports:
+        - containerPort: 26257
+          name: grpc
+        - containerPort: 8888
+          name: http
+`)
+	opts := th.MakeDefaultOptions()
+	m := th.Run(".", opts)
+	expFmt := `
+apiVersion: v1
+kind: Service
+metadata:
+  name: myService
+spec:
+  ports:
+  - name: grpc
+    port: %s
+    targetPort: %s
+---
+apiVersion: apps/v1beta1
+kind: StatefulSet
+metadata:
+  name: cockroachdb
+spec:
+  template:
+    spec:
+      containers:
+      - image: cockroachdb/cockroach:v1.1.5
+        name: cockroachdb
+        ports:
+        - containerPort: 26257
+          name: grpc
+        - containerPort: 8888
+          name: http
+`
+	th.AssertActualEqualsExpected(m,
+		// TODO(#3304): DECISION - quotes bad here, this is a bug.
+		opts.IfApiMachineryElseKyaml(
+			fmt.Sprintf(expFmt, `8888`, `8888`),
+			fmt.Sprintf(expFmt, `"8888"`, `"8888"`),
+		))
+}
+
 // TODO(#3449): varref has some quote issues
 // https://github.com/kubernetes-sigs/kustomize/issues/3449
 func TestVarRefBig(t *testing.T) {
@@ -928,6 +1008,7 @@ metadata:
   name: dev-base-test-config-map-6b85g79g7g
 `
 	th.AssertActualEqualsExpected(m,
+		// TODO(#3304): DECISION - quotes bad here, this still a bug.
 		opts.IfApiMachineryElseKyaml(
 			fmt.Sprintf(expFmt, `8080`, `8080`, `8080`, `8080`),
 			fmt.Sprintf(expFmt, `"8080"`, `"8080"`, `"8080"`, `"8080"`),

api/resmap/factory_test.go

@@ -392,6 +392,7 @@ spec:
 	yml, err = rm.AsYaml()
 	assert.NoError(t, err)
 
+	// TODO(#3304): DECISION - kyaml better; not a bug.
 	assert.Equal(t, konfig.IfApiMachineryElseKyaml(`apiVersion: example.com/v1
 kind: Foo
 metadata:

api/resource/factory_test.go

@@ -351,6 +351,7 @@ kind: List
 			// yaml, json, Resource, RNode, Unstructured etc.
 			// These conversions can be removed after closing
 			// https://github.com/kubernetes-sigs/kustomize/issues/2506
+			// TODO(#3304): DECISION - still a bug.
 			expectedErr: konfig.FlagEnableKyamlDefaultValue,
 		},
 		{

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go

@@ -13,7 +13,7 @@ import (
 	kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
 )
 
-// TODO(#3304)
+// TODO(#3304): DECISION - OK to move to kyaml and not do conflict detection.
 const skipConflictDetectionTests = konfig.FlagEnableKyamlDefaultValue
 
 func errorContains(err error, possibilities ...string) bool {
@@ -609,6 +609,7 @@ spec:
 		// when patch1 and patch2 are merged, so the patch
 		// applied is effectively only patch2.yaml.
 		// So it cannot delete the "B: Y"
+		// TODO(#3304): DECISION - undecided.
 		konfig.IfApiMachineryElseKyaml(`
 apiVersion: example.com/v1
 kind: Foo
@@ -656,7 +657,7 @@ spec:
 		resMap,
 		// This time only patch2 was applied.  Same answer on the kyaml
 		// path, but different answer on apimachinery path (B becomes "true"?)
-		// The kyaml path is doing better here.
+		// TODO(#3304): DECISION - kyaml doing better here, not a bug.
 		konfig.IfApiMachineryElseKyaml(`
 apiVersion: example.com/v1
 kind: Foo
@@ -779,7 +780,6 @@ spec:
 // Note that for SMP/WithSchema merge, the name:nginx entry
 // is mandatory
 func addLabelAndEnvPatch(kind string) string {
-
 	res := `
 apiVersion: apps/v1
 kind: %s

plugin/builtin/secretgenerator/SecretGenerator_test.go

@@ -44,15 +44,7 @@ literals:
 - VEGETABLE=carrot
 `)
 
-	obscure := `obscure: CkxvcmVtIGlwc3VtIGRvbG9yIHNpdCBhbWV0LApjb25zZWN0ZXR1ciBhZGlwaXNjaW5nIGVsaXQuCg==`
-	if konfig.FlagEnableKyamlDefaultValue {
-		// The kyaml code does a better job of line wrapping.
-		obscure = `obscure: |
-    CkxvcmVtIGlwc3VtIGRvbG9yIHNpdCBhbWV0LApjb25zZWN0ZXR1ciBhZGlwaXNjaW5nIG
-    VsaXQuCg==`
-	}
-
-	th.AssertActualEqualsExpected(rm, fmt.Sprintf(`
+	expFmt :=  `
 apiVersion: v1
 data:
   DB_PASSWORD: aWxvdmV5b3U=
@@ -65,5 +57,18 @@ metadata:
   name: mySecret
   namespace: whatever
 type: Opaque
-`, obscure))
+`
+	th.AssertActualEqualsExpected(
+		rm,
+		// TODO(#3304): DECISION - kyaml doing better here, not a bug.
+		konfig.IfApiMachineryElseKyaml(
+			fmt.Sprintf(
+				expFmt,
+				`obscure: CkxvcmVtIGlwc3VtIGRvbG9yIHNpdCBhbWV0LApjb25zZWN0ZXR1ciBhZGlwaXNjaW5nIGVsaXQuCg==`),
+			fmt.Sprintf(
+				expFmt,
+				`obscure: |
+    CkxvcmVtIGlwc3VtIGRvbG9yIHNpdCBhbWV0LApjb25zZWN0ZXR1ciBhZGlwaXNjaW5nIG
+    VsaXQuCg==`),
+			))
 }