fix(openshift): handle unquoted short max versions

pkg/controller/operators/openshift/clusteroperator_controller_test.go

@@ -1,6 +1,8 @@
 package openshift
 
 import (
+	"fmt"
+
 	semver "github.com/blang/semver/v4"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -161,7 +163,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		withMax := func(version string) map[string]string {
 			maxProperty := &api.Property{
 				Type:  MaxOpenShiftVersionProperty,
-				Value: `"` + version + `"`, // Wrap in quotes so we don't break property marshaling
+				Value: version,
 			}
 			value, err := projection.PropertiesAnnotationFromPropertyList([]*api.Property{maxProperty})
 			Expect(err).ToNot(HaveOccurred())
@@ -170,7 +172,7 @@ var _ = Describe("ClusterOperator controller", func() {
 				projection.PropertiesAnnotationKey: value,
 			}
 		}
-		incompatible.SetAnnotations(withMax(clusterVersion))
+		incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, clusterVersion))) // Wrap in quotes so we don't break property marshaling
 
 		Eventually(func() error {
 			return k8sClient.Create(ctx, incompatible)
@@ -202,7 +204,7 @@ var _ = Describe("ClusterOperator controller", func() {
 		// Set compatibility to the next minor version
 		next := semver.MustParse(clusterVersion)
 		Expect(next.IncrementMinor()).To(Succeed())
-		incompatible.SetAnnotations(withMax(next.String()))
+		incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, next.String())))
 
 		Eventually(func() error {
 			return k8sClient.Update(ctx, incompatible)
@@ -216,5 +218,32 @@ var _ = Describe("ClusterOperator controller", func() {
 			Status:             configv1.ConditionTrue,
 			LastTransitionTime: fixedNow(),
 		}))
+
+		By("understanding unquoted short max versions; e.g. X.Y")
+		// Mimic common pipeline shorthand
+		v := semver.MustParse(clusterVersion)
+		short := fmt.Sprintf("%d.%d", v.Major, v.Minor)
+		incompatible.SetAnnotations(withMax(short))
+
+		Eventually(func() error {
+			return k8sClient.Update(ctx, incompatible)
+		}).Should(Succeed())
+
+		Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
+			err := k8sClient.Get(ctx, clusterOperatorName, co)
+			return co.Status.Conditions, err
+		}, timeout).Should(ContainElement(configv1.ClusterOperatorStatusCondition{
+			Type:   configv1.OperatorUpgradeable,
+			Status: configv1.ConditionFalse,
+			Reason: IncompatibleOperatorsInstalled,
+			Message: skews{
+				{
+					namespace:           ns.GetName(),
+					name:                incompatible.GetName(),
+					maxOpenShiftVersion: short + ".0",
+				},
+			}.String(),
+			LastTransitionTime: fixedNow(),
+		}))
 	})
 })

pkg/controller/operators/openshift/helpers.go

@@ -2,7 +2,6 @@ package openshift
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -190,19 +189,15 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.
 			continue
 		}
 
-		var value string
-		if err := json.Unmarshal([]byte(property.Value), &value); err != nil {
-			errs = append(errs, err)
-			continue
-		}
-
+		value := strings.Trim(property.Value, "\"")
 		if value == "" {
 			continue
 		}
 
 		version, err := semver.ParseTolerant(value)
 		if err != nil {
 			errs = append(errs, err)
+			continue
 		}
 
 		if max == nil {

pkg/controller/operators/openshift/helpers_test.go

@@ -2,6 +2,7 @@ package openshift
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	semver "github.com/blang/semver/v4"
@@ -341,6 +342,11 @@ func TestIncompatibleOperators(t *testing.T) {
 					namespace:           "default",
 					maxOpenShiftVersion: "1.0.0",
 				},
+				{
+					name:                "chestnut",
+					namespace:           "default",
+					maxOpenShiftVersion: "1.0",
+				},
 			},
 			expect: expect{
 				err: false,
@@ -350,6 +356,11 @@ func TestIncompatibleOperators(t *testing.T) {
 						namespace:           "default",
 						maxOpenShiftVersion: "1.0.0",
 					},
+					{
+						name:                "chestnut",
+						namespace:           "default",
+						maxOpenShiftVersion: "1.0.0",
+					},
 				},
 			},
 		},
@@ -463,7 +474,10 @@ func TestIncompatibleOperators(t *testing.T) {
 
 func TestMaxOpenShiftVersion(t *testing.T) {
 	mustParse := func(s string) *semver.Version {
-		version := semver.MustParse(s)
+		version, err := semver.ParseTolerant(s)
+		if err != nil {
+			panic(fmt.Sprintf("bad version given for test case: %s", err))
+		}
 		return &version
 	}
 
@@ -485,7 +499,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Nothing",
-			in:          []string{""},
+			in:          []string{`""`},
 			expect: expect{
 				err: false,
 				max: nil,
@@ -494,8 +508,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Nothing/Mixed",
 			in: []string{
-				"",
-				"1.0.0",
+				`""`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -504,7 +518,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Garbage",
-			in:          []string{"bad_version"},
+			in:          []string{`"bad_version"`},
 			expect: expect{
 				err: true,
 				max: nil,
@@ -513,8 +527,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Garbage/Mixed",
 			in: []string{
-				"bad_version",
-				"1.0.0",
+				`"bad_version"`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: true,
@@ -523,7 +537,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		},
 		{
 			description: "Single",
-			in:          []string{"1.0.0"},
+			in:          []string{`"1.0.0"`},
 			expect: expect{
 				err: false,
 				max: mustParse("1.0.0"),
@@ -532,8 +546,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Multiple",
 			in: []string{
-				"1.0.0",
-				"2.0.0",
+				`"1.0.0"`,
+				`"2.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -543,8 +557,8 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Duplicates",
 			in: []string{
-				"1.0.0",
-				"1.0.0",
+				`"1.0.0"`,
+				`"1.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -554,9 +568,9 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Duplicates/NonMax",
 			in: []string{
-				"1.0.0",
-				"1.0.0",
-				"2.0.0",
+				`"1.0.0"`,
+				`"1.0.0"`,
+				`"2.0.0"`,
 			},
 			expect: expect{
 				err: false,
@@ -566,21 +580,30 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 		{
 			description: "Ambiguous",
 			in: []string{
-				"1.0.0",
-				"1.0.0+1",
+				`"1.0.0"`,
+				`"1.0.0+1"`,
 			},
 			expect: expect{
 				err: true,
 				max: nil,
 			},
 		},
+		{
+			// Ensure unquoted short strings are accepted; e.g. X.Y
+			description: "Unquoted/Short",
+			in:          []string{"4.8"},
+			expect: expect{
+				err: false,
+				max: mustParse("4.8"),
+			},
+		},
 	} {
 		t.Run(tt.description, func(t *testing.T) {
 			var properties []*api.Property
 			for _, max := range tt.in {
 				properties = append(properties, &api.Property{
 					Type:  MaxOpenShiftVersionProperty,
-					Value: `"` + max + `"`, // Wrap in quotes so we don't break property marshaling
+					Value: max,
 				})
 			}
 

pkg/controller/registry/resolver/projection/properties_test.go

@@ -54,6 +54,16 @@ func TestPropertiesAnnotationFromPropertyList(t *testing.T) {
 			},
 			expected: `{"properties":[{"type":"string","value":"hello"},{"type":"number","value":5},{"type":"array","value":[1,"two",3,"four"]},{"type":"object","value":{"hello":{"worl":"d"}}}]}`,
 		},
+		{
+			name: "unquoted string",
+			properties: []*api.Property{
+				{
+					Type:  "version",
+					Value: "4.8",
+				},
+			},
+			expected: `{"properties":[{"type":"version","value":4.8}]}`,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			actual, err := projection.PropertiesAnnotationFromPropertyList(tc.properties)
@@ -120,12 +130,23 @@ func TestPropertyListFromPropertiesAnnotation(t *testing.T) {
 				{
 					Type:  "array",
 					Value: `[1,"two",3,"four"]`,
-				}, {
+				},
+				{
 					Type:  "object",
 					Value: `{"hello":{"worl":"d"}}`,
 				},
 			},
 		},
+		{
+			name:       "unquoted string values",
+			annotation: `{"properties":[{"type": "version","value": 4.8}]}`,
+			expected: []*api.Property{
+				{
+					Type:  "version",
+					Value: "4.8",
+				},
+			},
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			actual, err := projection.PropertyListFromPropertiesAnnotation(tc.annotation)