SQUASH: separate function with tests

Author: cbandy

go.mod

@@ -3,7 +3,7 @@ module sigs.k8s.io/controller-runtime
 go 1.24.0
 
 require (
-	github.com/blang/semver/v4 v4.0.0
+	github.com/Masterminds/semver/v3 v3.4.0
 	github.com/evanphx/json-patch/v5 v5.9.11
 	github.com/fsnotify/fsnotify v1.9.0
 	github.com/go-logr/logr v1.4.2
@@ -37,6 +37,7 @@ require (
 	cel.dev/expr v0.24.0 // indirect
 	github.com/antlr4-go/antlr/v4 v4.13.0 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/blang/semver/v4 v4.0.0 // indirect
 	github.com/cenkalti/backoff/v4 v4.3.0 // indirect
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect

go.sum

@@ -1,5 +1,7 @@
 cel.dev/expr v0.24.0 h1:56OvJKSH3hDGL0ml5uSxZmz3/3Pq4tJ+fb1unVLAFcY=
 cel.dev/expr v0.24.0/go.mod h1:hLPLo1W4QUmuYdA72RBX06QTs6MXw941piREPl3Yfiw=
+github.com/Masterminds/semver/v3 v3.4.0 h1:Zog+i5UMtVoCU8oKka5P7i9q9HgrJeGzI9SA1Xbatp0=
+github.com/Masterminds/semver/v3 v3.4.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM=
 github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI=
 github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g=
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=

pkg/envtest/binaries.go

@@ -32,10 +32,9 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
-	"sort"
 	"strings"
 
-	"github.com/blang/semver/v4"
+	"github.com/Masterminds/semver/v3"
 	"sigs.k8s.io/yaml"
 )
 
@@ -111,6 +110,33 @@ type archive struct {
 	SelfLink string `json:"selfLink"`
 }
 
+// interpretVersion parses a "SemVer-ish" request into a single [semver.Version]
+// or a looser set of [semver.Constraints].
+//
+// This does *not* understand wildcards, comparators, ranges, or other special syntax.
+func interpretVersion(request string) (*semver.Version, *semver.Constraints) {
+	// When no version is requested, look for one in the release index.
+	if request == "" {
+		c, _ := semver.NewConstraint("*")
+		return nil, c
+	}
+
+	// When an exact version is requested, don't look for it in the release index.
+	if v, err := semver.StrictNewVersion(strings.TrimPrefix(request, "v")); err == nil {
+		return v, nil
+	}
+
+	// When it is an inexact version, turn it into a search of the release index.
+	if _, err := semver.NewVersion(request); err == nil {
+		if c, err := semver.NewConstraint(request); err == nil {
+			return nil, c
+		}
+	}
+
+	// The request isn't a version at all.
+	return nil, nil
+}
+
 func downloadBinaryAssets(ctx context.Context, binaryAssetsDirectory, binaryAssetsVersion, binaryAssetsIndexURL string) (string, string, string, error) {
 	if binaryAssetsIndexURL == "" {
 		binaryAssetsIndexURL = DefaultBinaryAssetsIndexURL
@@ -124,54 +150,24 @@ func downloadBinaryAssets(ctx context.Context, binaryAssetsDirectory, binaryAsse
 		}
 	}
 
-	var requestedRange semver.Range
-	if binaryAssetsVersion != "" {
-		binaryAssetsVersion = strings.TrimPrefix(binaryAssetsVersion, "v")
-		parsedVersion, errV := semver.ParseTolerant(binaryAssetsVersion)
-		parsedRange, errR := semver.ParseRange(binaryAssetsVersion)
-
-		switch {
-		// When an exact version is requested, don't look for it in the release index.
-		case errV == nil && parsedVersion.String() == binaryAssetsVersion:
-			requestedRange = nil
-
-		// When the version looks like a range, apply it to the index.
-		case errR == nil:
-			requestedRange = parsedRange
-
-		// When the version isn't exact, turn it into a range.
-		//
-		// The `setup-envtest` tool interprets a partial version to mean "latest stable with that prefix."
-		// For example, "1" and "1.2" are akin to "1.x.x" and "1.2.x" in [semver.ParseRange].
-		// [semver.ParseTolerant] fills in missing minor or patch with "0", so this replaces those with "x".
-		//
-		// That *should* produce a valid range. If it doesn't, use the original value and skip the index.
-		case errV == nil:
-			suffix := strings.TrimPrefix(parsedVersion.FinalizeVersion(), binaryAssetsVersion)
-			suffix = strings.ReplaceAll(suffix, "0", "x")
-			parsedRange, errR = semver.ParseRange(binaryAssetsVersion + suffix)
-
-			if errR == nil {
-				requestedRange = parsedRange
-			} else {
-				requestedRange = nil
-			}
-		}
-	} else {
-		// When no version is requested, look for one in the release index.
-		requestedRange = semver.MustParseRange(">0.0.0")
+	version, search := interpretVersion(binaryAssetsVersion)
+
+	// When an exact version is requested, use its canonical form to download without the release index.
+	if version != nil {
+		binaryAssetsVersion = version.String()
 	}
 
-	// When a range a versions is requested, select one from the release index.
+	// When a range of versions is requested, select a stable one from the release index.
 	var binaryAssetsIndex *index
-	if requestedRange != nil {
+	if search != nil {
 		var err error
 		binaryAssetsIndex, err = getIndex(ctx, binaryAssetsIndexURL)
 		if err != nil {
 			return "", "", "", err
 		}
 
-		binaryAssetsVersion, err = latestStableVersionFromIndex(binaryAssetsIndex, requestedRange)
+		search.IncludePrerelease = false
+		binaryAssetsVersion, err = latestVersionFromIndex(binaryAssetsIndex, search)
 		if err != nil {
 			return "", "", "", err
 		}
@@ -293,34 +289,33 @@ func downloadBinaryAssetsArchive(ctx context.Context, index *index, version stri
 	return readBody(resp, out, archiveName, archive.Hash)
 }
 
-func latestStableVersionFromIndex(index *index, satisfying semver.Range) (string, error) {
+func latestVersionFromIndex(index *index, satisfying *semver.Constraints) (string, error) {
 	if len(index.Releases) == 0 {
-		return "", fmt.Errorf("failed to find latest stable version from index: index is empty")
+		return "", fmt.Errorf("failed to find latest version from index: index is empty")
 	}
 
-	parsedVersions := []semver.Version{}
+	var maxVersion *semver.Version
 	for releaseVersion := range index.Releases {
-		v, err := semver.ParseTolerant(releaseVersion)
+		v, err := semver.NewVersion(releaseVersion)
 		if err != nil {
 			return "", fmt.Errorf("failed to parse version %q: %w", releaseVersion, err)
 		}
 
-		// Filter out pre-releases and undesirable versions.
-		if len(v.Pre) > 0 || !satisfying(v) {
+		// Filter out undesirable versions.
+		if !satisfying.Check(v) {
 			continue
 		}
 
-		parsedVersions = append(parsedVersions, v)
+		if maxVersion == nil || v.GreaterThan(maxVersion) {
+			maxVersion = v
+		}
 	}
 
-	if len(parsedVersions) == 0 {
-		return "", fmt.Errorf("failed to find latest stable version from index: index does not have stable versions")
+	if maxVersion == nil {
+		return "", fmt.Errorf("failed to find latest version from index: nothing matches %q", satisfying)
 	}
 
-	sort.Slice(parsedVersions, func(i, j int) bool {
-		return parsedVersions[i].GT(parsedVersions[j])
-	})
-	return "v" + parsedVersions[0].String(), nil
+	return "v" + maxVersion.String(), nil
 }
 
 func getIndex(ctx context.Context, indexURL string) (*index, error) {

pkg/envtest/binaries_test.go

@@ -28,13 +28,85 @@ import (
 	"os"
 	"path"
 	"runtime"
+	"testing"
 
+	"github.com/Masterminds/semver/v3"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/ghttp"
 	"sigs.k8s.io/yaml"
 )
 
+func TestInterpretVersion(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name string
+		in   []string
+
+		canonical string
+		included  []string
+		excluded  []string
+	}{
+		{
+			name:      "an exact release is not a search",
+			in:        []string{"v1.2.3", "1.2.3"},
+			canonical: "1.2.3",
+		},
+		{
+			name:      "an exact prerelease is not a search",
+			in:        []string{"v1.2.0-beta.1", "1.2.0-beta.1"},
+			canonical: "1.2.0-beta.1",
+		},
+		{
+			name:     "a version prefix is a search",
+			in:       []string{"v1.20", "1.20", "001.20"},
+			included: []string{"1.20.0", "v1.20.1", "1.20.999"},
+			excluded: []string{"1.19.999", "1.20.0-beta.1", "v1.20.0-rc.1", "v1.21.0"},
+		},
+		{
+			name:     "a version prefix can be short",
+			in:       []string{"v2", "2", "02"},
+			included: []string{"2.0.0", "v2.5.6"},
+			excluded: []string{"1.3.0", "v2.33.0-beta.0", "3.1.0"},
+		},
+		{
+			name: "weird stuff is ignored",
+			in: []string{
+				"asdf", "version", "vegeta4",
+				"the.1", "2ne1",
+				"=7.8.9", "10.x", "*",
+			},
+			// no canonical version and no search
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, in := range tc.in {
+				g := NewWithT(t)
+				version, search := interpretVersion(in)
+
+				if version != nil {
+					g.Expect(version.String()).To(Equal(tc.canonical))
+				} else {
+					g.Expect(tc.canonical).To(BeEmpty())
+				}
+
+				if search != nil {
+					compare := func(s string) bool { return search.Check(semver.MustParse(s)) }
+
+					g.Expect(tc.included).To(HaveEach(WithTransform(compare, BeTrue())))
+					g.Expect(tc.excluded).To(HaveEach(WithTransform(compare, BeFalse())))
+				} else {
+					g.Expect(tc.included).To(BeEmpty())
+					g.Expect(tc.excluded).To(BeEmpty())
+				}
+			}
+		})
+	}
+}
+
 var _ = Describe("Test download binaries", func() {
 	var downloadDirectory string
 	var server *ghttp.Server