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 IsPatch/Minor/Major/K0sUpgrade functions #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add IsPatch/Minor/Major/K0sUpgrade functions #21

wants to merge 1 commit into from

Conversation

kke
Copy link
Collaborator

@kke kke commented Jan 24, 2025

Adds functions for checking if upgrading to another version would be a patch, minor, major or k0s upgrade.

@kke kke added the enhancement New feature or request label Jan 24, 2025
@kke kke marked this pull request as ready for review January 24, 2025 10:08
@kke kke requested a review from twz123 January 24, 2025 10:08
@@ -125,6 +125,14 @@ func (v *Version) Segments() []int {
return v.segments[:v.numSegments]
}

// segmentEqual checks if the segments at the specified index are equal between two versions.
func (v *Version) segmentEqual(b *Version, index int) bool {
if v == nil || b == nil || index < 0 || index >= maxSegments {
Copy link
Member

@twz123 twz123 Jan 24, 2025

Choose a reason for hiding this comment

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

Smart-ass question: If both v and b are nil, this function returns false, albeit v == b. This somehow breaks the transitive relationship that the method's name implies (i.e. I think it's reasonable to assume that a.segmentEqual(b) is true if a == b). Should we support the receiver being nil here in the first place? The existing methods don't do it, if I'm not mistaken.

Same for the other new methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤔 Hmmmmmm.

Maybe these should have an error return. Neither true or false is "safe".

Maybe there should be something like:

type Upgrade struct {
  a, b *Version
}

func NewUpgrade(a, b *Version) (Upgrade, error) {
   ....
}

u, err := version.NewUpgrade(this, that)
if err ..... { }

if u.IsMinor() { .... }

Copy link
Member

Choose a reason for hiding this comment

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

That would be a good approach if there's the need to explicitly deal with nil pointers on each invocation. However, I would be okay with panicking if the receiver or the argument is nil. Why would a pointer to a Version be nil in real code? I can only come up with these reasons:

  1. A variable was forgotten to be initialized and its zero value is used
  2. A returned non-nil error was ignored when parsing a version string or calling another function that returns (*Version, error).
  3. The version is a piece of information that might or might not be available at runtime.

The first two are bugs. The third also somehow indicates that it should be up to the caller to deal with the situation. The methods here have no way of knowing what makes sense to assume in the absence of version information. The caller should decide.

In any case, I'd argue that calling the methods with nil pointers is a programmer's error that warrants a panic not to hide bugs, and not something that needs to be dealt with by this method. When in doubt, callers can still choose to do the nil checks themselves. They have more knowledge about whether one of the pointers can be nil at call time. I suppose it will turn out that in practice these nil checks are almost always unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants