fix: address sttts feedback

Author: exdx

pkg/controller/operators/catalog/operator.go

@@ -1306,65 +1306,6 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
 	return nil
 }
 
-func GetCRDV1VersionsMap(crd *apiextensionsv1.CustomResourceDefinition) map[string]struct{} {
-	versionsMap := map[string]struct{}{}
-
-	for _, version := range crd.Spec.Versions {
-		versionsMap[version.Name] = struct{}{}
-	}
-	return versionsMap
-}
-
-// Ensure all existing served versions are present in new CRD
-func GetCRDV1Beta1VersionsMap(crd *apiextensionsv1beta1.CustomResourceDefinition) map[string]struct{} {
-	versionsMap := map[string]struct{}{}
-
-	for _, version := range crd.Spec.Versions {
-		versionsMap[version.Name] = struct{}{}
-	}
-	if crd.Spec.Version != "" {
-		versionsMap[crd.Spec.Version] = struct{}{}
-	}
-
-	return versionsMap
-}
-
-// Ensure all existing served versions are present in new CRD
-func EnsureV1CRDVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
-	newCRDVersions := GetCRDV1VersionsMap(newCRD)
-
-	for _, oldVersion := range oldCRD.Spec.Versions {
-		if oldVersion.Served {
-			_, ok := newCRDVersions[oldVersion.Name]
-			if !ok {
-				return fmt.Errorf("New CRD (%s) must contain existing served versions (%s)", oldCRD.Name, oldVersion.Name)
-			}
-		}
-	}
-	return nil
-}
-
-// Ensure all existing served versions are present in new CRD
-func EnsureV1Beta1CRDVersions(oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) error {
-	newCRDVersions := GetCRDV1Beta1VersionsMap(newCRD)
-
-	for _, oldVersion := range oldCRD.Spec.Versions {
-		if oldVersion.Served {
-			_, ok := newCRDVersions[oldVersion.Name]
-			if !ok {
-				return fmt.Errorf("New CRD (%s) must contain existing served versions (%s)", oldCRD.Name, oldVersion.Name)
-			}
-		}
-	}
-	if oldCRD.Spec.Version != "" {
-		_, ok := newCRDVersions[oldCRD.Spec.Version]
-		if !ok {
-			return fmt.Errorf("New CRD (%s) must contain existing version (%s)", oldCRD.Name, oldCRD.Spec.Version)
-		}
-	}
-	return nil
-}
-
 // Validate all existing served versions against new CRD's validation (if changed)
 func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
 	logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)
@@ -1407,6 +1348,8 @@ func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *ap
 	if reflect.DeepEqual(oldCRD.Spec.Validation, newCRD.Spec.Validation) {
 		return nil
 	}
+
+	// TODO return early of all versions are equal
 	convertedCRD := &apiextensions.CustomResourceDefinition{}
 	if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
 		return err
@@ -1451,55 +1394,6 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersio
 	return nil
 }
 
-// Attempt to remove stored versions that have been deprecated before allowing
-// those versions to be removed from the new CRD.
-// The function may not always succeed as storedVersions requires at least one
-// version. If there is only stored version, it won't be removed until a new
-// stored version is added.
-func removeDeprecatedV1StoredVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) []string {
-	// StoredVersions requires to have at least one version.
-	if len(oldCRD.Status.StoredVersions) <= 1 {
-		return nil
-	}
-
-	newStoredVersions := []string{}
-	newCRDVersions := GetCRDV1VersionsMap(newCRD)
-	for _, v := range oldCRD.Status.StoredVersions {
-		_, ok := newCRDVersions[v]
-		if ok {
-			newStoredVersions = append(newStoredVersions, v)
-		}
-	}
-
-	if len(newStoredVersions) < 1 {
-		return nil
-	} else {
-		return newStoredVersions
-	}
-}
-
-func removeDeprecatedV1Beta1StoredVersions(oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) []string {
-	// StoredVersions requires to have at least one version.
-	if len(oldCRD.Status.StoredVersions) <= 1 {
-		return nil
-	}
-
-	newStoredVersions := []string{}
-	newCRDVersions := GetCRDV1Beta1VersionsMap(newCRD)
-	for _, v := range oldCRD.Status.StoredVersions {
-		_, ok := newCRDVersions[v]
-		if ok {
-			newStoredVersions = append(newStoredVersions, v)
-		}
-	}
-
-	if len(newStoredVersions) < 1 {
-		return nil
-	} else {
-		return newStoredVersions
-	}
-}
-
 // ExecutePlan applies a planned InstallPlan to a namespace.
 func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
 	if plan.Status.Phase != v1alpha1.InstallPlanPhaseInstalling {

pkg/controller/operators/catalog/operator_test.go

@@ -135,203 +135,6 @@ func TestTransitionInstallPlan(t *testing.T) {
 	}
 }
 
-func TestEnsureV1CRDVersions(t *testing.T) {
-	mainCRDPlural := "ins-main-abcde"
-
-	currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	addedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	missingVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	tests := []struct {
-		name            string
-		oldCRD          apiextensionsv1.CustomResourceDefinition
-		newCRD          apiextensionsv1.CustomResourceDefinition
-		expectedFailure bool
-	}{
-		{
-			name: "existing versions are present",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = addedVersions
-				return newCRD
-			}(),
-			expectedFailure: false,
-		},
-		{
-			name: "missing versions in new CRD 1",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = missingVersions
-				return newCRD
-			}(),
-			expectedFailure: true,
-		},
-		{
-			name: "missing version in new CRD 2",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions[0].Name = "v1alpha1"
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions[0].Name = "v1alpha2"
-				return newCRD
-			}(),
-			expectedFailure: true,
-		},
-		{
-			name: "existing version is present in new CRD's versions",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions[0].Name = "v1alpha1"
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = addedVersions
-				return newCRD
-			}(),
-			expectedFailure: false,
-		},
-	}
-
-	for _, tt := range tests {
-		err := EnsureV1CRDVersions(&tt.oldCRD, &tt.newCRD)
-		if tt.expectedFailure {
-			require.Error(t, err)
-		}
-	}
-}
-
-func TestRemoveDeprecatedV1StoredVersions(t *testing.T) {
-	mainCRDPlural := "ins-main-test"
-
-	currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha1",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	newVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1alpha2",
-			Served:  true,
-			Storage: false,
-		},
-		{
-			Name:    "apiextensionsv1beta1",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	crdStatusStoredVersions := apiextensionsv1.CustomResourceDefinitionStatus{
-		StoredVersions: []string{},
-	}
-
-	tests := []struct {
-		name           string
-		oldCRD         apiextensionsv1.CustomResourceDefinition
-		newCRD         apiextensionsv1.CustomResourceDefinition
-		expectedResult []string
-	}{
-		{
-			name: "only one stored version exists",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status = crdStatusStoredVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: nil,
-		},
-		{
-			name: "multiple stored versions with one deprecated version",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha2"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: []string{"v1alpha2"},
-		},
-		{
-			name: "multiple stored versions with all deprecated version",
-			oldCRD: func() apiextensionsv1.CustomResourceDefinition {
-				oldCRD := v1crd(mainCRDPlural)
-				oldCRD.Spec.Versions = currentVersions
-				oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha3"}
-				return oldCRD
-			}(),
-			newCRD: func() apiextensionsv1.CustomResourceDefinition {
-				newCRD := v1crd(mainCRDPlural)
-				newCRD.Spec.Versions = newVersions
-				return newCRD
-			}(),
-			expectedResult: nil,
-		},
-	}
-
-	for _, tt := range tests {
-		resultCRD := removeDeprecatedV1StoredVersions(&tt.oldCRD, &tt.newCRD)
-		require.Equal(t, tt.expectedResult, resultCRD)
-	}
-}
-
 func TestExecutePlan(t *testing.T) {
 	namespace := "ns"