Address code review feedback for build metadata precedence

- Add documentation for AsLegacyRegistryV1Version method explaining
  the build metadata conversion logic
- Fix AsLegacyRegistryV1Version to preserve original build metadata
  when Release field is not set
- Add comprehensive test coverage for NewLegacyRegistryV1VersionRelease
  including edge cases for build metadata and release parsing
- Add test coverage for AsLegacyRegistryV1Version conversion logic
- Improve compare_test.go test structure with descriptive test names
  and assertion functions for better clarity
- Add test case for non-release build metadata comparison

Author: joelanford

internal/operator-controller/bundle/versionrelease.go

@@ -65,14 +65,22 @@ func (vr *VersionRelease) Compare(other VersionRelease) int {
 	return vr.Release.Compare(other.Release)
 }
 
+// AsLegacyRegistryV1Version converts a VersionRelease into a standard semver version.
+// If the VersionRelease's Release field is set, the returned semver version's build
+// metadata is set to the VersionRelease's Release. Otherwise, the build metadata is
+// set to the VersionRelease's Version field's build metadata.
 func (vr *VersionRelease) AsLegacyRegistryV1Version() bsemver.Version {
-	return bsemver.Version{
+	v := bsemver.Version{
 		Major: vr.Version.Major,
 		Minor: vr.Version.Minor,
 		Patch: vr.Version.Patch,
 		Pre:   vr.Version.Pre,
-		Build: slicesutil.Map(vr.Release, func(i bsemver.PRVersion) string { return i.String() }),
+		Build: vr.Version.Build,
 	}
+	if len(vr.Release) > 0 {
+		v.Build = slicesutil.Map(vr.Release, func(i bsemver.PRVersion) string { return i.String() })
+	}
+	return v
 }
 
 type Release []bsemver.PRVersion

internal/operator-controller/bundle/versionrelease_test.go

@@ -3,12 +3,118 @@ package bundle_test
 import (
 	"testing"
 
+	bsemver "github.com/blang/semver/v4"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-controller/internal/operator-controller/bundle"
 )
 
+func TestNewLegacyRegistryV1VersionRelease(t *testing.T) {
+	type testCase struct {
+		name   string
+		input  string
+		expect func(*testing.T, *bundle.VersionRelease, error)
+	}
+	for _, tc := range []testCase{
+		{
+			name:  "empty input",
+			input: "",
+			expect: func(t *testing.T, _ *bundle.VersionRelease, err error) {
+				assert.Error(t, err)
+			},
+		},
+		{
+			name:  "v-prefix is invalid",
+			input: "v1.0.0",
+			expect: func(t *testing.T, _ *bundle.VersionRelease, err error) {
+				assert.Error(t, err)
+			},
+		},
+		{
+			name:  "valid semver, valid build metadata, but not a valid release",
+			input: "1.2.3-alpha+4.005",
+			expect: func(t *testing.T, vr *bundle.VersionRelease, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, uint64(1), vr.Version.Major)
+				assert.Equal(t, uint64(2), vr.Version.Minor)
+				assert.Equal(t, uint64(3), vr.Version.Patch)
+				assert.Equal(t, []bsemver.PRVersion{{VersionStr: "alpha"}}, vr.Version.Pre)
+				assert.Equal(t, []string{"4", "005"}, vr.Version.Build)
+				assert.Empty(t, vr.Release)
+			},
+		},
+		{
+			name:  "valid semver, valid build metadata, valid release",
+			input: "1.2.3-alpha+4.5",
+			expect: func(t *testing.T, vr *bundle.VersionRelease, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, uint64(1), vr.Version.Major)
+				assert.Equal(t, uint64(2), vr.Version.Minor)
+				assert.Equal(t, uint64(3), vr.Version.Patch)
+				assert.Equal(t, []bsemver.PRVersion{{VersionStr: "alpha"}}, vr.Version.Pre)
+				assert.Empty(t, vr.Version.Build)
+				assert.Equal(t, bundle.Release([]bsemver.PRVersion{
+					{VersionNum: 4, IsNum: true},
+					{VersionNum: 5, IsNum: true},
+				}), vr.Release)
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			actual, err := bundle.NewLegacyRegistryV1VersionRelease(tc.input)
+			tc.expect(t, actual, err)
+		})
+	}
+}
+
+func TestVersionRelease_AsLegacyRegistryV1Version(t *testing.T) {
+	type testCase struct {
+		name   string
+		input  bundle.VersionRelease
+		expect bsemver.Version
+	}
+	for _, tc := range []testCase{
+		{
+			name: "Release is set, so release is set as build metadata",
+			input: bundle.VersionRelease{
+				Version: bsemver.Version{
+					Major: 1,
+					Minor: 2,
+					Patch: 3,
+				},
+				Release: bundle.Release([]bsemver.PRVersion{{VersionStr: "release"}}),
+			},
+			expect: bsemver.Version{
+				Major: 1,
+				Minor: 2,
+				Patch: 3,
+				Build: []string{"release"},
+			},
+		},
+		{
+			name: "Release is unset, so version build metadata is set as build metadata",
+			input: bundle.VersionRelease{
+				Version: bsemver.Version{
+					Major: 1,
+					Minor: 2,
+					Patch: 3,
+					Build: []string{"buildmetadata"},
+				},
+			},
+			expect: bsemver.Version{
+				Major: 1,
+				Minor: 2,
+				Patch: 3,
+				Build: []string{"buildmetadata"},
+			}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.expect, tc.input.AsLegacyRegistryV1Version())
+		})
+	}
+}
+
 func TestLegacyRegistryV1VersionRelease_Compare(t *testing.T) {
 	type testCase struct {
 		name   string
@@ -107,6 +213,12 @@ func TestLegacyRegistryV1VersionRelease_Compare(t *testing.T) {
 			v2:     "0.0.0-0+1",
 			expect: 1,
 		},
+		{
+			name:   "non-release build metadata is less than valid release build metadata",
+			v1:     "0.0.0-0+1.001",
+			v2:     "0.0.0-0+0",
+			expect: -1,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			vr1, err1 := bundle.NewLegacyRegistryV1VersionRelease(tc.v1)

internal/operator-controller/bundleutil/bundle_test.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"testing"
 
-	"github.com/blang/semver/v4"
 	bsemver "github.com/blang/semver/v4"
 	"github.com/stretchr/testify/require"
 
@@ -29,7 +28,7 @@ func TestGetVersionAndRelease(t *testing.T) {
 				Value: json.RawMessage(`{"version": "1.0.0-pre+1.alpha.2"}`),
 			},
 			wantVersionRelease: &bundle.VersionRelease{
-				Version: semver.MustParse("1.0.0-pre"),
+				Version: bsemver.MustParse("1.0.0-pre"),
 				Release: bundle.Release([]bsemver.PRVersion{
 					{VersionNum: 1, IsNum: true},
 					{VersionStr: "alpha"},
@@ -53,7 +52,7 @@ func TestGetVersionAndRelease(t *testing.T) {
 				Value: json.RawMessage(`{"version": "1.0.0+001"}`),
 			},
 			wantVersionRelease: &bundle.VersionRelease{
-				Version: semver.MustParse("1.0.0+001"),
+				Version: bsemver.MustParse("1.0.0+001"),
 			},
 			wantErr: false,
 		},

internal/operator-controller/catalogmetadata/compare/compare_test.go

@@ -19,40 +19,68 @@ func TestNewVersionRange(t *testing.T) {
 	type testCase struct {
 		name         string
 		versionRange string
-		inputVersion bsemver.Version
-		expect       bool
+		assertFunc   func(t *testing.T, vr bsemver.Range, err error)
 	}
 	for _, tc := range []testCase{
 		{
+			name:         "version without build metadata matches range with build metadata",
 			versionRange: "1.0.0+1",
-			inputVersion: bsemver.MustParse("1.0.0"),
-			expect:       true,
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.True(t, vr(bsemver.MustParse("1.0.0")))
+			},
 		},
 		{
+			name:         "version with different build metadata matches range with build metadata",
 			versionRange: "1.0.0+1",
-			inputVersion: bsemver.MustParse("1.0.0+2"),
-			expect:       true,
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.True(t, vr(bsemver.MustParse("1.0.0+2")))
+			},
 		},
 		{
+			name:         "version with same build metadata matches range with build metadata",
 			versionRange: "1.0.0+1",
-			inputVersion: bsemver.MustParse("1.0.0+1"),
-			expect:       true,
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.True(t, vr(bsemver.MustParse("1.0.0+1")))
+			},
 		},
 		{
+			name:         "exact version match without build metadata",
 			versionRange: "1.0.0",
-			inputVersion: bsemver.MustParse("1.0.0"),
-			expect:       true,
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.True(t, vr(bsemver.MustParse("1.0.0")))
+			},
 		},
 		{
+			name:         "version with build metadata matches range without build metadata",
 			versionRange: "1.0.0",
-			inputVersion: bsemver.MustParse("1.0.0+1"),
-			expect:       true,
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.True(t, vr(bsemver.MustParse("1.0.0+1")))
+			},
+		},
+		{
+			name:         "invalid range returns error",
+			versionRange: "not-a-valid-version",
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.Error(t, err)
+			},
+		},
+		{
+			name:         "version does not match range",
+			versionRange: ">=2.0.0",
+			assertFunc: func(t *testing.T, vr bsemver.Range, err error) {
+				require.NoError(t, err)
+				assert.False(t, vr(bsemver.MustParse("1.0.0")))
+			},
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			versionRange, err := compare.NewVersionRange(tc.versionRange)
-			require.NoError(t, err)
-			assert.Equal(t, tc.expect, versionRange(tc.inputVersion))
+			tc.assertFunc(t, versionRange, err)
 		})
 	}
 }