-
Notifications
You must be signed in to change notification settings - Fork 1.2k
✨ envtest: search the assets index for latest of a release series #3260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Chris Bandy <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cbandy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @cbandy. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
pkg/envtest/binaries.go
Outdated
parsedVersions := []semver.Version{} | ||
for releaseVersion := range index.Releases { | ||
// Filter on prefix. | ||
if !strings.HasPrefix(strings.TrimPrefix(releaseVersion, "v"), prefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too simple, I suppose. 1.2
resolves to v1.29.4, and in about 20 years (~70 releases) two digit numbers will match on three digit ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a semver lib for this to find the minor version rather than this prefix matching and dot splitting business?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should really use "major.minor" to indicate "latest stable major.minor" (I assume we don't want to pick up pre-releases?). I would usually assume major.minor
stands for major.minor.0
. But maybe this is defined somewhere, not sure :)
In Cluster API we used this format: stable-1.29
(so stable-major.minor
)
Also allows us to eventually add latest-major.minor
if there is ever demand for something that picks up the latest release including pre-releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see correctly patch is not optional for semver (https://semver.org/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I see correctly patch is not optional for semver (https://semver.org/)
The semver spec may require it, but I think its possible for an actual lib to (optionally) not require it.
I'm wondering if we should really use "major.minor" to indicate "latest stable major.minor" (I assume we don't want to pick up pre-releases?). I would usually assume major.minor stands for major.minor.0. But maybe this is defined somewhere, not sure :
My assumption would've also been that major.minor
means latest patch of major.minor
but good to know that different ppl have different interpretations of this.
Taking a step back, does it really make sense to ever resolve a version here dynamically for either the patch or the minor? The use-case for envtest is tests, if the k8s version it uses changes over time that means tests may break without any code change in the test or code under test, which seems bad and should probably not be natively supported here...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption would've also been that major.minor means latest patch of major.minor but good to know that different ppl have different interpretations of this.
Hm yeah, you're probably right. E.g. if you use the golang:1.24 Docker image you also always get the latest patch version.
The semver spec may require it, but I think its possible for an actual lib to (optionally) not require it.
Yup. I think the standard library is github.com/blang/semver/v4
, CR already has it as a dependency.
// ParseTolerant allows for certain version specifications that do not strictly adhere to semver
// specs to be parsed by this library. It does so by normalizing versions before passing them to
// Parse(). It currently trims spaces, removes a "v" prefix, adds a 0 patch number to versions
// with only major and minor components specified, and removes leading 0s.
func ParseTolerant(s string) (Version, error) {
We have to add a bit of additional code so we can differentiate between e.g. v1.29.0 and v1.29 as 0 is the zero value of Version.Patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a semver lib for this to find the minor version rather than this prefix matching and dot splitting business?
Yep. That's where I was going to go next.
I would usually assume major.minor stands for major.minor.0. But maybe this is defined somewhere, not sure :)
The two-place version is how we've been using setup-envtest
. It has an entire notion of selectors, but the relevant bit is:
controller-runtime/tools/setup-envtest/versions/parse.go
Lines 26 to 29 in 71f7db5
// The whole string is a version selector as follows: | |
// - X.Y.Z matches version X.Y.Z where x, y, and z are | |
// are ints >= 0, and Z may be '*' or 'x' | |
// - X.Y is equivalent to X.Y.* |
In Cluster API we have various tests that intentionally test Cluster API with latest patch versions of various Kubernetes release series. So that we immediately notice if a new Kubernetes patch version introduced an issue.
That's how we've been using setup-envtest
in our project. We intend to be compatible with a few minor versions of Kubernetes behind latest stable, so we have a matrix in CI like [v1.30, v1.31, v1.32]
. We haven't needed setup-envtest
's selectors beyond that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine if we want to go ahead with major.minor standing for latest stable patch of major.minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 The version package we're importing has its notion of selectors, too. This is the flavor I'm interested in:
// * when dealing with wildcards without // version operator: // 1.2.x will become >= 1.2.0 < 1.3.0 // 1.x will become >= 1.0.0 < 2.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a semver lib for this to find the minor version rather than this prefix matching and dot splitting business?
Done.
/ok-to-test |
This lets one configure the
DownloadBinaryAssetsVersion
field ofenvtest.Environment
with only the Major.Minor portions of a Kubernetes API version. WhenDownloadBinaryAssets
is set, the framework will search the asset index for the latest stable version in that release series.This matches the behavior of
setup-envtest use {major}.{minor}
without the full complexity of version selectors. The leadingv
is now optional, too.For example:
"v1.29"
resolves to v1.29.4"1.27"
resolves to v1.27.1"1.24"
resolves to v1.24.2