Repository: knative/pkg (go)

Issue:
Storage Version Migrator doing empty patches for no reason
When a CRD has a single storage version in it's status. The storage version migrator should skip performing migrations (empty PATCHes)

Currently it seems like we don't check for this and always perform migrations when we don't need to.


Pull request:
Don't migrate storage version if CRD has one storage version
<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 
-->
- :bug: Fixed an issue where the [storage version migrator ](https://github.com/knative/pkg/blob/efe9f4a7b3f33a9db1a355b5375136b3a8c0fb1d/apiextensions/storageversion/migrator.go#L57)was running on CRDs that only had one storage version (related [comment](https://github.com/knative/serving/issues/14029#issuecomment-1741550974) for context).

Fixes #2845 

<!-- Please include the 'why' behind your changes if no issue exists -->


Patch:
diff --git a/apiextensions/storageversion/migrator.go b/apiextensions/storageversion/migrator.go
index 7ac1cc932..469031251 100644
--- a/apiextensions/storageversion/migrator.go
+++ b/apiextensions/storageversion/migrator.go
@@ -68,6 +68,11 @@ func (m *Migrator) Migrate(ctx context.Context, gr schema.GroupResource) error {
 		return fmt.Errorf("unable to determine storage version for %s", gr)
 	}
 
+	// don't migrate storage version if CRD has a single valid storage in its status
+	if len(crd.Status.StoredVersions) == 1 && crd.Status.StoredVersions[0] == version {
+		return nil
+	}
+
 	if err := m.migrateResources(ctx, gr.WithVersion(version)); err != nil {
 		return err
 	}


Test patch:
diff --git a/apiextensions/storageversion/migrator_test.go b/apiextensions/storageversion/migrator_test.go
index 8129d456c..f5b50c334 100644
--- a/apiextensions/storageversion/migrator_test.go
+++ b/apiextensions/storageversion/migrator_test.go
@@ -19,6 +19,7 @@ package storageversion
 import (
 	"context"
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -88,6 +89,42 @@ func TestMigrate(t *testing.T) {
 	)
 }
 
+func TestMigrate_SingleStoredVersion(t *testing.T) {
+	singleVersionFakeCRD := &apix.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: fakeGR.String(),
+		},
+		Spec: apix.CustomResourceDefinitionSpec{
+			Group: fakeGK.Group,
+			Versions: []apix.CustomResourceDefinitionVersion{
+				{Name: "v1", Served: true, Storage: true},
+			},
+		},
+		Status: apix.CustomResourceDefinitionStatus{
+			StoredVersions: []string{"v1"},
+		},
+	}
+
+	// setup
+	dclient := dynamicFake.NewSimpleDynamicClient(runtime.NewScheme())
+	cclient := apixFake.NewSimpleClientset(singleVersionFakeCRD)
+
+	cclient.Fake.PrependReactor("patch", "customresourcedefinitions", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+		if pa, ok := action.(k8stesting.PatchAction); ok {
+			if pa.GetName() == singleVersionFakeCRD.Name {
+				return false, nil, fmt.Errorf("resource shouldn't have been patched")
+			}
+		}
+
+		return false, nil, nil
+	})
+
+	m := NewMigrator(dclient, cclient)
+	if err := m.Migrate(context.Background(), fakeGR); err != nil {
+		t.Fatal("Migrate() =", err)
+	}
+}
+
 // func TestMigrate_Paging(t *testing.T) {
 // 	t.Skip("client-go lacks testing pagination " +
 // 		"since list options aren't captured in actions")