Skip to content
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

Add MajorMinor to match whole releases #23

Closed
wants to merge 1 commit into from

Conversation

twz123
Copy link
Member

@twz123 twz123 commented Jan 27, 2025

Also introduce the VersionMatcher and VersionComparer interfaces, so that new (or custom) types can be used to match and compare versions in a canonical style.

This allows, for example, this:

// RBAC is enabled by default since Kubernetes 1.6
var rbacEnabledByDefault = version.AtLeast(version.NewMajorMinor(1, 6))

k0sVersion := version.MustParse(...)
if k0sVersion.Is(rbacEnabledByDefault) {
    ... do something related to RBAC
}

Also introduce the VersionMatcher and VersionComparer interfaces, so
that new (or custom) types can be used to match and compare versions
in a canonical style.

Signed-off-by: Tom Wieczorek <[email protected]>
@twz123 twz123 marked this pull request as ready for review January 28, 2025 07:17
@kke
Copy link
Collaborator

kke commented Jan 29, 2025

I suppose this is more of an alternative to #21 than to #25 but at the same time it's a step towards rewriting the constraint thing as the same kind of comparators.

I like the interfaces, but they should be somehow chainable to achieve the same as the constraint syntax does, ie. something like:

comparer := version.AllowPrereleases().And(
  version.IsAtleast(version.MajorMinor(1,30)).And( // does this allow 1.30.0-rc.1? 
    version.IsOlderThanOrEqualTo(version.MustParse("1.31.4"))
  )
)


if k0sVersion.Is(comparer) {
   ...
}

vs

if version.MustConstraint(">= 1.30, <= 1.31.4").Check(k0sVersion) {
  // (does not allow any prereleases)
  ...
}

(or with #25 with prereleases allowed):

if version.MustConstraint(">>= 1.30, <<= 1.31.4").Check(k0sVersion) {
  // ( allows min 1.30.0 but not 1.30.0-alpha.1 
  //    and max 1.31.4 (including 1.31.4-alpha.1))
  ...
}

(there's now no good syntax for ~1.30 or ^1.30 or whatever that would match 1.30.0-unknownalpha.1. currently >> 1.29 gets converted to 1.29.0 and would thus match 1.29.25-alpha.1)

I believe both approaches will require runtime string parsing for some operations unless you introduce something like version.NewMinorMajorPatchPreK0s(1,30,4,"",1) to use instead of version.NewVersion("1.30.4+k0s.1") or >= 1.30.4+k0s.1

For checking if an upgrade is patch only, with this one you could do something like:

target.MajorMinor().Is(version.IsExactly(k0sVersion.MajorMinor()))
// or
target.MajorMinor().Equal(k0sVersion.MajorMinor())

instead of this in #21:

k0sVersion.IsPatchUpgrade(targetVersion)

While this MajorMinor-checking may be useful somewhere, it doesn't solve the k0sctl use case, as k0s core is effectively not versioned. Some completely incompatible k0s core change may be (and has been) introduced together with a kubernetes patch version bump or even in the same patch version but in a more recent +k0s.x buildtag.

🤷

Comment on lines +377 to +385
func (v *Version) IsNewerThan(c VersionComparer) bool {
return v.Is(NewerThan(c))
}

func (v *Version) IsAtLeast(c VersionComparer) bool {
return v.Is(AtLeast(c))
}

func (v *Version) IsOlderThan(c VersionComparer) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer/Older may not be the best naming as something like v1.30.1 could be older than v.1.29.20 when something is backported to the 1.29 branch

@twz123 twz123 closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants