Merge pull request #34010 from soltysh/fix_edit_sj

Automatic merge from submit-queue

Match GroupVersionKind against specific version

Currently when multiple GVK match a specific kind in `KindForGroupVersionKinds` only the first will be matched, which not necessarily will be the correct one. I'm proposing to extend this to pick the best match, instead.

Here's my problematic use-case, of course it involves ScheduledJobs 😉:
I have a `GroupVersions` with `batch/v1` and `batch/v2alpha1` in that order. I'm calling `KindForGroupVersionKinds` with kind `batch/v2alpha1 ScheduledJob` and that currently results this matching first `GroupVersion`, instead of picking more concrete one. There's a [clear description](ee77d4e6ca/pkg/api/unversioned/group_version.go (L183)) why it is on single `GroupVersion`, but `GroupVersions` should pick this more carefully.

@deads2k this is your baby, wdyt?

pkg/api/unversioned/group_version.go

@@ -268,17 +268,37 @@ type GroupVersions []GroupVersion
 
 // KindForGroupVersionKinds identifies the preferred GroupVersionKind out of a list. It returns ok false
 // if none of the options match the group.
-func (gvs GroupVersions) KindForGroupVersionKinds(kinds []GroupVersionKind) (target GroupVersionKind, ok bool) {
+func (gvs GroupVersions) KindForGroupVersionKinds(kinds []GroupVersionKind) (GroupVersionKind, bool) {
+	var targets []GroupVersionKind
 	for _, gv := range gvs {
 		target, ok := gv.KindForGroupVersionKinds(kinds)
 		if !ok {
 			continue
 		}
-		return target, true
+		targets = append(targets, target)
+	}
+	if len(targets) == 1 {
+		return targets[0], true
+	}
+	if len(targets) > 1 {
+		return bestMatch(kinds, targets), true
 	}
 	return GroupVersionKind{}, false
 }
 
+// bestMatch tries to pick best matching GroupVersionKind and falls back to the first
+// found if no exact match exists.
+func bestMatch(kinds []GroupVersionKind, targets []GroupVersionKind) GroupVersionKind {
+	for _, gvk := range targets {
+		for _, k := range kinds {
+			if k == gvk {
+				return k
+			}
+		}
+	}
+	return targets[0]
+}
+
 // ToAPIVersionAndKind is a convenience method for satisfying runtime.Object on types that
 // do not use TypeMeta.
 func (gvk *GroupVersionKind) ToAPIVersionAndKind() (string, string) {

pkg/api/unversioned/group_version_test.go

@@ -147,3 +147,47 @@ func TestGroupVersionMarshalJSON(t *testing.T) {
 		}
 	}
 }
+
+func TestKindForGroupVersionKinds(t *testing.T) {
+	gvks := GroupVersions{
+		GroupVersion{Group: "batch", Version: "v1"},
+		GroupVersion{Group: "batch", Version: "v2alpha1"},
+		GroupVersion{Group: "policy", Version: "v1alpha1"},
+	}
+	cases := []struct {
+		input  []GroupVersionKind
+		target GroupVersionKind
+		ok     bool
+	}{
+		{
+			input:  []GroupVersionKind{{Group: "batch", Version: "v2alpha1", Kind: "ScheduledJob"}},
+			target: GroupVersionKind{Group: "batch", Version: "v2alpha1", Kind: "ScheduledJob"},
+			ok:     true,
+		},
+		{
+			input:  []GroupVersionKind{{Group: "batch", Version: "v3alpha1", Kind: "CronJob"}},
+			target: GroupVersionKind{Group: "batch", Version: "v1", Kind: "CronJob"},
+			ok:     true,
+		},
+		{
+			input:  []GroupVersionKind{{Group: "policy", Version: "v1alpha1", Kind: "PodDisruptionBudget"}},
+			target: GroupVersionKind{Group: "policy", Version: "v1alpha1", Kind: "PodDisruptionBudget"},
+			ok:     true,
+		},
+		{
+			input:  []GroupVersionKind{{Group: "apps", Version: "v1alpha1", Kind: "PetSet"}},
+			target: GroupVersionKind{},
+			ok:     false,
+		},
+	}
+
+	for i, c := range cases {
+		target, ok := gvks.KindForGroupVersionKinds(c.input)
+		if c.target != target {
+			t.Errorf("%d: unexpected target: %v, expected %v", i, target, c.target)
+		}
+		if c.ok != ok {
+			t.Errorf("%d: unexpected ok: %v, expected %v", i, ok, c.ok)
+		}
+	}
+}

pkg/runtime/scheme_test.go

@@ -602,12 +602,15 @@ func TestConvertToVersion(t *testing.T) {
 			gv:     unversioned.GroupVersion{Version: "__internal"},
 			out:    &TestType1{A: "test"},
 		},
-		// prefers the first group version in the list
+		// prefers the best match
 		{
 			scheme: GetTestScheme(),
 			in:     &ExternalTestType1{A: "test"},
 			gv:     unversioned.GroupVersions{{Version: "__internal"}, {Version: "v1"}},
-			out:    &TestType1{A: "test"},
+			out: &ExternalTestType1{
+				MyWeirdCustomEmbeddedVersionKindField: MyWeirdCustomEmbeddedVersionKindField{APIVersion: "v1", ObjectKind: "TestType1"},
+				A: "test",
+			},
 		},
 		// unversioned type returned as-is
 		{