From cb9a1d34d7bfce77888620bd43e0945fa49af094 Mon Sep 17 00:00:00 2001
From: Anik Bhattacharjee <anbhatta@redhat.com>
Date: Tue, 22 Oct 2024 19:04:09 +0530
Subject: [PATCH] fix: buggy nextY logic for maxOCPVersion

The handling logic of versions that are pre-releases by the
nextY() func (that determines the next Y release) was erroneous.

Eg: nextY("4.16.0") returns "4.17" correctly, but nextY("4.16.0-rc1")
returns "4.16" (the correct value is still "4.17").

This PR fixes the nextY function.

Also has improvement for the "not-upgradeable to next OCP" version message.
---
 pkg/controller/operators/openshift/helpers.go | 33 ++++++-----------
 .../operators/openshift/helpers_test.go       | 36 +++++++++++++++++--
 2 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/pkg/controller/operators/openshift/helpers.go b/pkg/controller/operators/openshift/helpers.go
index d13f1cc150..dae3807a7e 100644
--- a/pkg/controller/operators/openshift/helpers.go
+++ b/pkg/controller/operators/openshift/helpers.go
@@ -91,7 +91,12 @@ func (s skews) String() string {
 		j--
 	}
 
-	return "ClusterServiceVersions blocking cluster upgrade: " + strings.Join(msg, ",")
+	// it is safe to ignore the error here, with the assumption
+	// that we build each skew object only after verifying that the
+	// version string is parseable safely.
+	maxOCPVersion, _ := semver.ParseTolerant(s[0].maxOpenShiftVersion)
+	nextY := nextY(maxOCPVersion).String()
+	return fmt.Sprintf("ClusterServiceVersions blocking minor version upgrades to %s or higher:\n%s", nextY, strings.Join(msg, "\n"))
 }
 
 type skew struct {
@@ -103,10 +108,9 @@ type skew struct {
 
 func (s skew) String() string {
 	if s.err != nil {
-		return fmt.Sprintf("%s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err)
+		return fmt.Sprintf("- %s/%s has invalid %s properties: %s", s.namespace, s.name, MaxOpenShiftVersionProperty, s.err)
 	}
-
-	return fmt.Sprintf("%s/%s is incompatible with OpenShift minor versions greater than %s", s.namespace, s.name, s.maxOpenShiftVersion)
+	return fmt.Sprintf("- maximum supported OCP version for %s/%s is %s", s.namespace, s.name, s.maxOpenShiftVersion)
 }
 
 type transientError struct {
@@ -131,11 +135,6 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 		return nil, fmt.Errorf("failed to determine current OpenShift Y-stream release")
 	}
 
-	next, err := nextY(*current)
-	if err != nil {
-		return nil, err
-	}
-
 	csvList := &operatorsv1alpha1.ClusterServiceVersionList{}
 	if err := cli.List(ctx, csvList); err != nil {
 		return nil, &transientError{fmt.Errorf("failed to list ClusterServiceVersions: %w", err)}
@@ -158,7 +157,7 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 			continue
 		}
 
-		if max == nil || max.GTE(next) {
+		if max == nil || max.GTE(nextY(*current)) {
 			continue
 		}
 
@@ -224,18 +223,8 @@ func getCurrentRelease() (*semver.Version, error) {
 	return currentRelease.version, nil
 }
 
-func nextY(v semver.Version) (semver.Version, error) {
-	v.Build = nil // Builds are irrelevant
-
-	if len(v.Pre) > 0 {
-		// Dropping pre-releases is equivalent to incrementing Y
-		v.Pre = nil
-		v.Patch = 0
-
-		return v, nil
-	}
-
-	return v, v.IncrementMinor() // Sets Y=Y+1 and Z=0
+func nextY(v semver.Version) semver.Version {
+	return semver.Version{Major: v.Major, Minor: v.Minor + 1} // Sets Y=Y+1
 }
 
 const (
diff --git a/pkg/controller/operators/openshift/helpers_test.go b/pkg/controller/operators/openshift/helpers_test.go
index 8e08991e05..3fd56b789e 100644
--- a/pkg/controller/operators/openshift/helpers_test.go
+++ b/pkg/controller/operators/openshift/helpers_test.go
@@ -423,7 +423,7 @@ func TestIncompatibleOperators(t *testing.T) {
 		},
 		{
 			description: "ClusterPre",
-			version:     "1.1.0-pre", // Next Y-stream is 1.1.0, NOT 1.2.0
+			version:     "1.1.0-pre", // Next Y-stream is 1.2.0
 			in: skews{
 				{
 					name:                "almond",
@@ -432,8 +432,14 @@ func TestIncompatibleOperators(t *testing.T) {
 				},
 			},
 			expect: expect{
-				err:          false,
-				incompatible: nil,
+				err: false,
+				incompatible: skews{
+					{
+						name:                "almond",
+						namespace:           "default",
+						maxOpenShiftVersion: "1.1",
+					},
+				},
 			},
 		},
 	} {
@@ -597,3 +603,27 @@ func TestNotCopiedSelector(t *testing.T) {
 		})
 	}
 }
+
+func TestOCPVersionNextY(t *testing.T) {
+	for _, tc := range []struct {
+		description     string
+		inVersion       semver.Version
+		expectedVersion semver.Version
+	}{
+		{
+			description:     "Version: 4.16.0. Expected output: 4.17",
+			inVersion:       semver.MustParse("4.16.0"),
+			expectedVersion: semver.MustParse("4.17.0"),
+		},
+		{
+			description:     "Version: 4.16.0-rc1. Expected output: 4.17",
+			inVersion:       semver.MustParse("4.16.0-rc1"),
+			expectedVersion: semver.MustParse("4.17.0"),
+		},
+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			outVersion := nextY(tc.inVersion)
+			require.Equal(t, outVersion, tc.expectedVersion)
+		})
+	}
+}