Fix tests and PR feedback

Author: matthchr

v2/cmd/asoctl/internal/crd/cleaner.go

@@ -97,9 +97,8 @@ func (c *Cleaner) Run(ctx context.Context) error {
 		asoCRDsSeen++
 		newStoredVersions, deprecatedVersions := crdmanagement.GetDeprecatedStorageVersions(crd)
 
-		// If there is no new version found other than the matched version, we short circuit here, as there is no updated version found in the CRDs
-		if len(newStoredVersions) <= 0 {
-			// TODO: test?
+		// If there is no new version found other than alpha/beta, it means there wasn't a newer version installed/reconciled yet and we short circuit here, as there is no updated version found in the CRDs
+		if len(newStoredVersions) == 1 && (strings.HasPrefix(newStoredVersions[0], "v1alpha") || strings.HasPrefix(newStoredVersions[0], "v1beta")) {
 			return eris.Errorf("it doesn't look like your version of ASO is one that supports deprecating CRD %q, versions %q. Have you upgraded ASO yet?", crd.Name, deprecatedVersions)
 		}
 

v2/internal/crdmanagement/deprecation.go

@@ -12,7 +12,7 @@ import (
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 )
 
-var versionRegexp = regexp.MustCompile(`^v(1api)?(\d{8})(preview)?(storage)?$`)
+var versionRegexp = regexp.MustCompile(`^v(1api|1alpha1api|1beta)?(\d{8}|\d)(preview)?(storage)?$`)
 
 // GetDeprecatedStorageVersions returns a new list of storedVersions by removing the deprecated versions
 // The first return value is the list of versions that are NOT deprecated.
@@ -79,6 +79,26 @@ func CompareVersions(a string, b string) int {
 		return 1
 	}
 
+	// Prefer everything over v1alpha1apiYYYYMMDD
+	hasV1Alpha1A := matchA[1] == "1alpha1api"
+	hasV1Alpha1B := matchB[1] == "1alpha1api"
+
+	if hasV1Alpha1A && !hasV1Alpha1B {
+		return -1
+	} else if !hasV1Alpha1A && hasV1Alpha1B {
+		return 1
+	}
+
+	// Prefer everything over v1betaYYYYMMDD
+	hasV1BetaA := matchA[1] == "1beta"
+	hasV1BetaB := matchB[1] == "1beta"
+
+	if hasV1BetaA && !hasV1BetaB {
+		return -1
+	} else if !hasV1BetaA && hasV1BetaB {
+		return 1
+	}
+
 	// Prefer newer date over older date
 	dateA := matchA[2]
 	dateB := matchB[2]
@@ -90,12 +110,12 @@ func CompareVersions(a string, b string) int {
 	}
 
 	// Prefer vYYYYMMDD over v1apiYYYYMMDD
-	hasV1apiA := matchA[1] == "1api"
-	hasV1apiB := matchB[1] == "1api"
+	hasV1ApiA := matchA[1] == "1api"
+	hasV1ApiB := matchB[1] == "1api"
 
-	if hasV1apiA && !hasV1apiB {
+	if hasV1ApiA && !hasV1ApiB {
 		return -1
-	} else if !hasV1apiA && hasV1apiB {
+	} else if !hasV1ApiA && hasV1ApiB {
 		return 1
 	}
 

v2/internal/crdmanagement/deprecation_test.go

@@ -287,19 +287,19 @@ func Test_CompareVersions(t *testing.T) {
 		{
 			name:     "One invalid pattern (a < b)",
 			a:        "v1api20240101",
-			b:        "v1beta1",
+			b:        "v1betazzz1",
 			expected: -1,
 		},
 		{
 			name:     "One invalid pattern (a > b)",
-			a:        "v1beta1",
+			a:        "v1betazzz1",
 			b:        "v1api20240101",
 			expected: 1,
 		},
 		{
 			name:     "Equal invalid patterns",
-			a:        "v1beta1",
-			b:        "v1beta1",
+			a:        "v1betazzz1",
+			b:        "v1betazzz1",
 			expected: 0,
 		},
 		{
@@ -332,6 +332,110 @@ func Test_CompareVersions(t *testing.T) {
 			b:        "v1api20240101storage",
 			expected: -1,
 		},
+		// v1alpha1api tests
+		{
+			name:     "v1alpha1api vs v1api same date (a < b)",
+			a:        "v1alpha1api20240101",
+			b:        "v1api20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1alpha1api vs v1api same date (a > b)",
+			a:        "v1api20240101",
+			b:        "v1alpha1api20240101",
+			expected: 1,
+		},
+		{
+			name:     "v1alpha1api vs v same date (a < b)",
+			a:        "v1alpha1api20240101",
+			b:        "v20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1alpha1api vs v same date (a > b)",
+			a:        "v20240101",
+			b:        "v1alpha1api20240101",
+			expected: 1,
+		},
+		{
+			name:     "v1alpha1api newer date vs v1api older date (a < b)",
+			a:        "v1alpha1api20250101",
+			b:        "v1api20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1alpha1api newer date vs v older date (a < b)",
+			a:        "v1alpha1api20250101",
+			b:        "v20240101",
+			expected: -1,
+		},
+		// v1beta tests
+		{
+			name:     "v1beta vs v1api same date (a < b)",
+			a:        "v1beta20240101",
+			b:        "v1api20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta vs v1api same date (a > b)",
+			a:        "v1api20240101",
+			b:        "v1beta20240101",
+			expected: 1,
+		},
+		{
+			name:     "v1beta vs v same date (a < b)",
+			a:        "v1beta20240101",
+			b:        "v20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta vs v same date (a > b)",
+			a:        "v20240101",
+			b:        "v1beta20240101",
+			expected: 1,
+		},
+		{
+			name:     "v1beta newer date vs v1api older date (a < b)",
+			a:        "v1beta20250101",
+			b:        "v1api20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta newer date vs v older date (a < b)",
+			a:        "v1beta20250101",
+			b:        "v20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta vs v1alpha1api same date (a > b)",
+			a:        "v1beta20240101",
+			b:        "v1alpha1api20240101",
+			expected: 1,
+		},
+		{
+			name:     "v1beta vs v1alpha1api same date (a < b)",
+			a:        "v1alpha1api20240101",
+			b:        "v1beta20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta with preview vs non-preview v1api (a < b)",
+			a:        "v1beta20240101preview",
+			b:        "v1api20240101",
+			expected: -1,
+		},
+		{
+			name:     "v1beta with storage vs v1api with storage same date (a < b)",
+			a:        "v1beta20240101storage",
+			b:        "v1api20240101storage",
+			expected: -1,
+		},
+		{
+			name:     "v1beta vs v1 (a < b)",
+			a:        "v1beta1",
+			b:        "v1",
+			expected: -1,
+		},
 	}
 
 	for _, c := range cases {