Merge pull request #4344 from natasha41575/ResIdStr

improve gvk and resid strings for error messages

api/filters/replacement/replacement.go

@@ -172,7 +172,7 @@ func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, err
 		return nil, err
 	}
 	if rn.IsNilOrEmpty() {
-		return nil, fmt.Errorf("fieldPath `%s` is missing for replacement source %s", r.Source.FieldPath, r.Source)
+		return nil, fmt.Errorf("fieldPath `%s` is missing for replacement source %s", r.Source.FieldPath, r.Source.ResId)
 	}
 
 	return getRefinedValue(r.Source.Options, rn)

api/filters/replacement/replacement_test.go

@@ -269,7 +269,7 @@ spec:
   - select:
       kind: Deployment
 `,
-			expectedErr: "multiple matches for selector ~G_~V_Deployment|~X|~N",
+			expectedErr: "multiple matches for selector Deployment.[noVer].[noGrp]/[noName].[noNs]",
 		},
 		"replacement has no source": {
 			input: `apiVersion: v1
@@ -1586,7 +1586,7 @@ data:
     options:
       create: true
 `,
-			expectedErr: "fieldPath `data.httpPort` is missing for replacement source ~G_~V_ConfigMap|~X|ports-from:data.httpPort",
+			expectedErr: "fieldPath `data.httpPort` is missing for replacement source ConfigMap.[noVer].[noGrp]/ports-from.[noNs]",
 		},
 		"annotationSelector": {
 			input: `apiVersion: v1

api/internal/accumulator/namereferencetransformer_test.go

@@ -521,7 +521,7 @@ func TestNameReferenceUnhappyRun(t *testing.T) {
 					},
 				}).ResMap(),
 			expectedErr: `updating name reference in 'rules/resourceNames' field of ` +
-				`'rbac.authorization.k8s.io_v1_ClusterRole|~X|cr'` +
+				`'ClusterRole.v1.rbac.authorization.k8s.io/cr.[noNs]'` +
 				`: considering field 'rules/resourceNames' of object
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole

api/krusty/complexcomposition_test.go

@@ -368,7 +368,7 @@ resources:
 		t.Fatalf("Expected resource accumulation error")
 	}
 	if !strings.Contains(
-		err.Error(), "already registered id: apps_v1_StatefulSet|~X|my-sts") {
+		err.Error(), "already registered id: StatefulSet.v1.apps/my-sts.[noNs]") {
 		t.Fatalf("Unexpected err: %v", err)
 	}
 }
@@ -459,7 +459,7 @@ resources:
 		t.Fatalf("Expected resource accumulation error")
 	}
 	if !strings.Contains(
-		err.Error(), "already registered id: apps_v1_StatefulSet|~X|my-sts") {
+		err.Error(), "already registered id: StatefulSet.v1.apps/my-sts.[noNs]") {
 		t.Fatalf("Unexpected err: %v", err)
 	}
 }

api/krusty/component_test.go

@@ -589,7 +589,7 @@ components:
 - ../comp-b`),
 			},
 			runPath:       "prod",
-			expectedError: "may not add resource with an already registered id: ~G_v1_Deployment|~X|proxy",
+			expectedError: "may not add resource with an already registered id: Deployment.v1.[noGrp]/proxy.[noNs]",
 		},
 		"components-cannot-add-the-same-base": {
 			input: []FileGen{writeTestBase,
@@ -608,7 +608,7 @@ components:
 - ../comp-b`),
 			},
 			runPath:       "prod",
-			expectedError: "may not add resource with an already registered id: ~G_v1_Deployment|~X|storefront",
+			expectedError: "may not add resource with an already registered id: Deployment.v1.[noGrp]/storefront.[noNs]",
 		},
 		"components-cannot-add-bases-containing-the-same-resource": {
 			input: []FileGen{writeTestBase,
@@ -639,7 +639,7 @@ components:
 - ../comp-b`),
 			},
 			runPath:       "prod",
-			expectedError: "may not add resource with an already registered id: ~G_v1_Deployment|~X|proxy",
+			expectedError: "may not add resource with an already registered id: Deployment.v1.[noGrp]/proxy.[noNs]",
 		},
 	}
 

api/krusty/diamondcomposition_test.go

@@ -141,7 +141,7 @@ resources:
 		t.Fatalf("Expected resource accumulation error")
 	}
 	if !strings.Contains(
-		err.Error(), "already registered id: apps_v1_Deployment|~X|my-deployment") {
+		err.Error(), "already registered id: Deployment.v1.apps/my-deployment.[noNs]") {
 		t.Fatalf("Unexpected err: %v", err)
 	}
 }

api/krusty/repeatbase_test.go

@@ -128,7 +128,7 @@ spec:
 
 	err := th.RunWithErr("mango", th.MakeDefaultOptions())
 	if !strings.Contains(
-		err.Error(), "multiple matches for Id apps_v1_Deployment|~X|banana; failed to find unique target for patch") {
+		err.Error(), "multiple matches for Id Deployment.v1.apps/banana.[noNs]; failed to find unique target for patch Deployment.v1.apps/banana.[noNs]") {
 		t.Fatalf("Unexpected err: %v", err)
 	}
 

api/resmap/reswrangler.go

@@ -215,7 +215,7 @@ func (m *resWrangler) GetById(
 	if err != nil {
 		return nil, fmt.Errorf(
 			"%s; failed to find unique target for patch %s",
-			err.Error(), id.GvknString())
+			err.Error(), id.String())
 	}
 	return r, nil
 }

api/resource/resource_test.go

@@ -1377,7 +1377,7 @@ spec:
   numReplicas: 1
 `))
 	assert.NoError(t, err)
-	r.SetGvk(resid.GvkFromString("grp_ver_knd"))
+	r.SetGvk(resid.GvkFromString("knd.ver.grp"))
 	gvk := r.GetGvk()
 	if expected, actual := "grp", gvk.Group; expected != actual {
 		t.Fatalf("expected '%s', got '%s'", expected, actual)
@@ -1400,30 +1400,30 @@ spec:
   numReplicas: 1
 `))
 	assert.NoError(t, err)
-	r.AppendRefBy(resid.FromString("gr1_ver1_knd1|ns1|name1"))
+	r.AppendRefBy(resid.FromString("knd1.ver1.gr1/name1.ns1"))
 	assert.Equal(t, r.RNode.MustString(), `apiVersion: v1
 kind: Deployment
 metadata:
   name: clown
   annotations:
-    internal.config.kubernetes.io/refBy: gr1_ver1_knd1|ns1|name1
+    internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1
 spec:
   numReplicas: 1
 `)
-	assert.Equal(t, r.GetRefBy(), []resid.ResId{resid.FromString("gr1_ver1_knd1|ns1|name1")})
+	assert.Equal(t, r.GetRefBy(), []resid.ResId{resid.FromString("knd1.ver1.gr1/name1.ns1")})
 
-	r.AppendRefBy(resid.FromString("gr2_ver2_knd2|ns2|name2"))
+	r.AppendRefBy(resid.FromString("knd2.ver2.gr2/name2.ns2"))
 	assert.Equal(t, r.RNode.MustString(), `apiVersion: v1
 kind: Deployment
 metadata:
   name: clown
   annotations:
-    internal.config.kubernetes.io/refBy: gr1_ver1_knd1|ns1|name1,gr2_ver2_knd2|ns2|name2
+    internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1,knd2.ver2.gr2/name2.ns2
 spec:
   numReplicas: 1
 `)
 	assert.Equal(t, r.GetRefBy(), []resid.ResId{
-		resid.FromString("gr1_ver1_knd1|ns1|name1"),
+		resid.FromString("knd1.ver1.gr1/name1.ns1"),
-		resid.FromString("gr2_ver2_knd2|ns2|name2"),
+		resid.FromString("knd2.ver2.gr2/name2.ns2"),
 	})
 }

kyaml/resid/gvk.go

@@ -49,7 +49,7 @@ func ParseGroupVersion(apiVersion string) (group, version string) {
 // GvkFromString makes a Gvk from the output of Gvk.String().
 func GvkFromString(s string) Gvk {
 	values := strings.Split(s, fieldSep)
-	if len(values) != 3 {
+	if len(values) < 3 {
 		// ...then the string didn't come from Gvk.String().
 		return Gvk{
 			Group:   noGroup,
@@ -57,27 +57,27 @@ func GvkFromString(s string) Gvk {
 			Kind:    noKind,
 		}
 	}
-	g := values[0]
+	k := values[0]
-	if g == noGroup {
+	if k == noKind {
-		g = ""
+		k = ""
 	}
 	v := values[1]
 	if v == noVersion {
 		v = ""
 	}
-	k := values[2]
+	g := strings.Join(values[2:], fieldSep)
-	if k == noKind {
+	if g == noGroup {
-		k = ""
+		g = ""
 	}
 	return NewGvk(g, v, k)
 }
 
 // Values that are brief but meaningful in logs.
 const (
-	noGroup   = "~G"
+	noGroup   = "[noGrp]"
-	noVersion = "~V"
+	noVersion = "[noVer]"
-	noKind    = "~K"
+	noKind    = "[noKind]"
-	fieldSep  = "_"
+	fieldSep  = "."
 )
 
 // String returns a string representation of the GVK.
@@ -94,7 +94,7 @@ func (x Gvk) String() string {
 	if k == "" {
 		k = noKind
 	}
-	return strings.Join([]string{g, v, k}, fieldSep)
+	return strings.Join([]string{k, v, g}, fieldSep)
 }
 
 // ApiVersion returns the combination of Group and Version
@@ -109,7 +109,8 @@ func (x Gvk) ApiVersion() string {
 }
 
 // StringWoEmptyField returns a string representation of the GVK. Non-exist
-// fields will be omitted.
+// fields will be omitted. This is called when generating a filename for the
+// resource.
 func (x Gvk) StringWoEmptyField() string {
 	var s []string
 	if x.Group != "" {
@@ -121,7 +122,7 @@ func (x Gvk) StringWoEmptyField() string {
 	if x.Kind != "" {
 		s = append(s, x.Kind)
 	}
-	return strings.Join(s, fieldSep)
+	return strings.Join(s, "_")
 }
 
 // Equals returns true if the Gvk's have equal fields.

kyaml/resid/gvk_test.go

@@ -47,8 +47,8 @@ var lessThanTests = []struct {
 		Gvk{Group: "a", Version: "c", Kind: "ClusterRole"}},
 	{Gvk{Group: "a", Version: "c", Kind: "Namespace"},
 		Gvk{Group: "a", Version: "b", Kind: "ClusterRole"}},
-	{Gvk{Group: "a", Version: "d", Kind: "Namespace"},
+	{Gvk{Group: "b", Version: "c", Kind: "Namespace"},
-		Gvk{Group: "b", Version: "c", Kind: "Namespace"}},
+		Gvk{Group: "a", Version: "d", Kind: "Namespace"}},
 	{Gvk{Group: "a", Version: "b", Kind: orderFirst[len(orderFirst)-1]},
 		Gvk{Group: "a", Version: "b", Kind: orderLast[0]}},
 	{Gvk{Group: "a", Version: "b", Kind: orderFirst[len(orderFirst)-1]},
@@ -87,14 +87,16 @@ var stringTests = []struct {
 	s string
 	r string
 }{
-	{Gvk{}, "~G_~V_~K", ""},
+	{Gvk{}, "[noKind].[noVer].[noGrp]", ""},
-	{Gvk{Kind: "k"}, "~G_~V_k", "k"},
+	{Gvk{Kind: "k"}, "k.[noVer].[noGrp]", "k"},
-	{Gvk{Version: "v"}, "~G_v_~K", "v"},
+	{Gvk{Version: "v"}, "[noKind].v.[noGrp]", "v"},
-	{Gvk{Version: "v", Kind: "k"}, "~G_v_k", "v_k"},
+	{Gvk{Version: "v", Kind: "k"}, "k.v.[noGrp]", "v_k"},
-	{Gvk{Group: "g"}, "g_~V_~K", "g"},
+	{Gvk{Group: "g"}, "[noKind].[noVer].g", "g"},
-	{Gvk{Group: "g", Kind: "k"}, "g_~V_k", "g_k"},
+	{Gvk{Group: "g", Kind: "k"}, "k.[noVer].g", "g_k"},
-	{Gvk{Group: "g", Version: "v"}, "g_v_~K", "g_v"},
+	{Gvk{Group: "g", Version: "v"}, "[noKind].v.g", "g_v"},
-	{Gvk{Group: "g", Version: "v", Kind: "k"}, "g_v_k", "g_v_k"},
+	{Gvk{Group: "g", Version: "v", Kind: "k"}, "k.v.g", "g_v_k"},
+	{Gvk{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole", isClusterScoped: true},
+		"ClusterRole.v1.rbac.authorization.k8s.io", "rbac.authorization.k8s.io_v1_ClusterRole"},
 }
 
 func TestString(t *testing.T) {

kyaml/resid/resid.go

@@ -37,9 +37,9 @@ func NewResIdKindOnly(k string, n string) ResId {
 }
 
 const (
-	noNamespace          = "~X"
+	noNamespace          = "[noNs]"
-	noName               = "~N"
+	noName               = "[noName]"
-	separator            = "|"
+	separator            = "/"
 	TotallyNotANamespace = "_non_namespaceable_"
 	DefaultNamespace     = "default"
 )
@@ -55,33 +55,31 @@ func (id ResId) String() string {
 		nm = noName
 	}
 	return strings.Join(
-		[]string{id.Gvk.String(), ns, nm}, separator)
+		[]string{id.Gvk.String(), strings.Join([]string{nm, ns}, fieldSep)}, separator)
 }
 
 func FromString(s string) ResId {
 	values := strings.Split(s, separator)
-	g := GvkFromString(values[0])
+	gvk := GvkFromString(values[0])
 
-	ns := values[1]
+	values = strings.Split(values[1], fieldSep)
+	last := len(values)-1
+
+	ns := values[last]
 	if ns == noNamespace {
 		ns = ""
 	}
-	nm := values[2]
+	nm := strings.Join(values[:last], fieldSep)
 	if nm == noName {
 		nm = ""
 	}
 	return ResId{
-		Gvk:       g,
+		Gvk:       gvk,
 		Namespace: ns,
 		Name:      nm,
 	}
 }
 
-// GvknString of ResId based on GVK and name
-func (id ResId) GvknString() string {
-	return id.Gvk.String() + separator + id.Name
-}
-
 // GvknEquals returns true if the other id matches
 // Group/Version/Kind/name.
 func (id ResId) GvknEquals(o ResId) bool {

kyaml/resid/resid_test.go

@@ -17,7 +17,7 @@ var resIdStringTests = []struct {
 			Gvk:       Gvk{Group: "g", Version: "v", Kind: "k"},
 			Name:      "nm",
 		},
-		"g_v_k|ns|nm",
+		"k.v.g/nm.ns",
 	},
 	{
 		ResId{
@@ -25,7 +25,7 @@ var resIdStringTests = []struct {
 			Gvk:       Gvk{Version: "v", Kind: "k"},
 			Name:      "nm",
 		},
-		"~G_v_k|ns|nm",
+		"k.v.[noGrp]/nm.ns",
 	},
 	{
 		ResId{
@@ -33,7 +33,7 @@ var resIdStringTests = []struct {
 			Gvk:       Gvk{Kind: "k"},
 			Name:      "nm",
 		},
-		"~G_~V_k|ns|nm",
+		"k.[noVer].[noGrp]/nm.ns",
 	},
 	{
 		ResId{
@@ -41,38 +41,26 @@ var resIdStringTests = []struct {
 			Gvk:       Gvk{},
 			Name:      "nm",
 		},
-		"~G_~V_~K|ns|nm",
+		"[noKind].[noVer].[noGrp]/nm.ns",
 	},
 	{
 		ResId{
 			Gvk:  Gvk{},
 			Name: "nm",
 		},
-		"~G_~V_~K|~X|nm",
+		"[noKind].[noVer].[noGrp]/nm.[noNs]",
-	},
-	{
-		ResId{
-			Gvk:  Gvk{},
-			Name: "nm",
-		},
-		"~G_~V_~K|~X|nm",
 	},
 	{
 		ResId{
 			Gvk: Gvk{},
 		},
-		"~G_~V_~K|~X|~N",
+		"[noKind].[noVer].[noGrp]/[noName].[noNs]",
-	},
-	{
-		ResId{
-			Gvk: Gvk{},
-		},
-		"~G_~V_~K|~X|~N",
 	},
 	{
 		ResId{},
-		"~G_~V_~K|~X|~N",
+		"[noKind].[noVer].[noGrp]/[noName].[noNs]",
 	},
+
 }
 
 func TestResIdString(t *testing.T) {
@@ -83,84 +71,7 @@ func TestResIdString(t *testing.T) {
 	}
 }
 
-var gvknStringTests = []struct {
-	x ResId
-	s string
-}{
-	{
-		ResId{
-			Namespace: "ns",
-			Gvk:       Gvk{Group: "g", Version: "v", Kind: "k"},
-			Name:      "nm",
-		},
-		"g_v_k|nm",
-	},
-	{
-		ResId{
-			Namespace: "ns",
-			Gvk:       Gvk{Version: "v", Kind: "k"},
-			Name:      "nm",
-		},
-		"~G_v_k|nm",
-	},
-	{
-		ResId{
-			Namespace: "ns",
-			Gvk:       Gvk{Kind: "k"},
-			Name:      "nm",
-		},
-		"~G_~V_k|nm",
-	},
-	{
-		ResId{
-			Namespace: "ns",
-			Gvk:       Gvk{},
-			Name:      "nm",
-		},
-		"~G_~V_~K|nm",
-	},
-	{
-		ResId{
-			Gvk:  Gvk{},
-			Name: "nm",
-		},
-		"~G_~V_~K|nm",
-	},
-	{
-		ResId{
-			Gvk:  Gvk{},
-			Name: "nm",
-		},
-		"~G_~V_~K|nm",
-	},
-	{
-		ResId{
-			Gvk: Gvk{},
-		},
-		"~G_~V_~K|",
-	},
-	{
-		ResId{
-			Gvk: Gvk{},
-		},
-		"~G_~V_~K|",
-	},
-	{
-		ResId{},
-		"~G_~V_~K|",
-	},
-}
-
-func TestGvknString(t *testing.T) {
-	for _, hey := range gvknStringTests {
-		if hey.x.GvknString() != hey.s {
-			t.Fatalf("Actual: %s,  Expected: '%s'", hey.x.GvknString(), hey.s)
-		}
-	}
-}
-
 func TestResIdEquals(t *testing.T) {
-
 	var GvknEqualsTest = []struct {
 		id1        ResId
 		id2        ResId
@@ -358,6 +269,24 @@ var ids = []ResId{
 	{
 		Gvk: Gvk{},
 	},
+	{
+		Gvk: Gvk{
+			Group: "rbac.authorization.k8s.io",
+			Version: "v1",
+			Kind: "ClusterRole",
+			isClusterScoped: true,
+		},
+		Name: "nm",
+	},
+	{
+		Gvk: Gvk{
+			Group: "rbac.authorization.k8s.io",
+			Version: "v1",
+			Kind: "ClusterRole",
+			isClusterScoped: true,
+		},
+		Name: "my.name",
+	},
 }
 
 func TestResIdIsSelected(t *testing.T) {

plugin/builtin/replicacounttransformer/ReplicaCountTransformer_test.go

@@ -218,7 +218,7 @@ spec:
 		t.Fatalf("No match should return an error")
 	}
 	if err.Error() !=
-		"resource with name service does not match a config with the following GVK [~G_~V_Deployment]" {
+		"resource with name service does not match a config with the following GVK [Deployment.[noVer].[noGrp]]" {
 		t.Fatalf("Unexpected error: %v", err)
 	}
 }