add the version package to utils #98

yastij · 2019-07-12T15:54:21Z

Signed-off-by: Yassine TIJANI ytijani@vmware.com

This moves the version package to utils. it only depends on stdlib packages and is tested.

This would help unblock kubernetes/kubernetes#79526 specifically kubernetes/kubernetes#79526 (comment)

k8s-ci-robot · 2019-07-12T15:54:22Z

Welcome @yastij!

It looks like this is your first PR to kubernetes/utils 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/utils has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

yastij · 2019-07-12T15:55:40Z

/assign @dims

andrewsykim · 2019-07-12T15:57:13Z

/assign @lavalamp

Signed-off-by: Yassine TIJANI <ytijani@vmware.com>

k8s-ci-robot · 2019-07-12T15:58:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yastij
To complete the pull request process, please assign dims
You can assign the PR to them by writing /assign @dims in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yastij · 2019-07-12T16:07:05Z

build fails because of some lint errors, but I wanted to kick discussions with this PR

dims · 2019-07-12T16:07:26Z

/assign @thockin

dims · 2019-07-12T16:08:02Z

yea, ignore the lint errors. someone is working on it.

thockin

Don't we have like 4 semver libs in vendor/... already? Why do we have our own?

thockin · 2019-07-12T17:30:30Z

version/version.go

+// or more dot-separated numeric fields (the first of which can't have leading zeroes),
+// followed by arbitrary uninterpreted data (which need not be separated from the final
+// numeric field by punctuation). For convenience, leading and trailing whitespace is
+// ignored, and the version can be preceded by the letter "v". See also ParseSemantic.


Actual examples of allowed values and disallowed values, highlighting the rules would be very helpful

thockin · 2019-07-12T17:30:43Z

version/version.go

+// the "Semantic Versioning" specification (http://semver.org/) (although it ignores
+// leading and trailing whitespace, and allows the version to be preceded by "v"). For
+// version strings that are not guaranteed to obey the Semantic Versioning syntax, use
+// ParseGeneric.


actual examples would be helpful

thockin · 2019-07-12T17:32:02Z

version/version.go

+	return v.components[2]
+}
+
+// BuildMetadata returns the build metadata, if v is a Semantic Version, or ""


example of this

thockin · 2019-07-12T17:32:56Z

version/version.go

+	return v.components[1]
+}
+
+// Patch returns the patch release number if v is a Semantic Version, or 0


What if a value is semver-like (triple) but not strictly semver - do we handle X.Y.Z%foo

thockin · 2019-07-12T17:38:37Z

version/version.go

+}
+
+// Major returns the major release number
+func (v *Version) Major() uint {


I would argue none of these methods should be on pointers. The struct is value-like, we should treat it as such. Pointer methods require everyone use pointer (swill comment more below).

https://play.golang.org/p/aDDvEZZ3aqX

thockin · 2019-07-12T17:40:18Z

version/version.go

+}
+
+// WithMajor returns copy of the version object with requested major number
+func (v *Version) WithMajor(major uint) *Version {


I don't think we should be returning pointers. It defeats some possible optimizations and forces everyone to use pointers.

https://play.golang.org/p/aDDvEZZ3aqX show how a helper function must return pointers to call these methods. If this is value-like, treat it like a value.

thockin · 2019-07-12T17:42:41Z

version/version.go

+		if i > 0 {
+			buffer.WriteString(".")
+		}
+		buffer.WriteString(fmt.Sprintf("%d", comp))


we sopecifically call components uint, but then you print it as an int here. Why don't we just store it as int?

The you could use ItoA

Yeah just because it must be positive is NOT a good enough reason to use a uint.

thockin · 2019-07-12T17:44:36Z

version/version.go

+
+var (
+	// versionMatchRE splits a version string into numeric and "extra" parts
+	versionMatchRE = regexp.MustCompile(`^\s*v?([0-9]+(?:\.[0-9]+)*)(.*)*$`)


This seems to allow parsing of numbers that could be bigger than a uint? Should we bound this to 10 characters (2^32) ?

thockin · 2019-07-12T17:45:41Z

version/version.go

+// AtLeast tests if a version is at least equal to a given minimum version. If both
+// Versions are Semantic Versions, this will use the Semantic Version comparison
+// algorithm. Otherwise, it will compare only the numeric components, with non-present
+// components being considered "0" (ie, "1.4" is equal to "1.4.0").


Examples of normal and edge cases?

danwinship · 2019-07-15T13:26:51Z

Don't we have like 4 semver libs in vendor/... already? Why do we have our own?

Well, this one is not just semver, which is why we have it (kubernetes/kubernetes#35436). But it's already in k/apimachinery. Why would it need to be moved to k/utils?

fejta-bot · 2019-10-13T14:15:01Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

yastij · 2019-10-14T13:47:49Z

/close

k8s-ci-robot · 2019-10-14T13:47:51Z

@yastij: Closed this PR.

In response to this:

/close

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/test-infra repository.

lavalamp · 2019-10-14T16:51:24Z

I think it'd be OK to move this (this is in apimachinery for lack of a better place; it's more build machinery, though, IMO), but it needs a name other than "version". Like, what's special about this version type? (Special attention to / opinions about all the stuff after the patch version number, it looks like.)

Either way, it'd be nice to address @thockin's comments on the source package. :) :)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 12, 2019

k8s-ci-robot requested review from apelisse and thockin July 12, 2019 15:54

k8s-ci-robot assigned dims Jul 12, 2019

k8s-ci-robot assigned lavalamp Jul 12, 2019

add the version package to utils

9792021

Signed-off-by: Yassine TIJANI <ytijani@vmware.com>

yastij force-pushed the move-version-package branch from 33e2355 to 9792021 Compare July 12, 2019 15:58

k8s-ci-robot assigned thockin Jul 12, 2019

thockin reviewed Jul 12, 2019

View reviewed changes

dims removed their assignment Jul 15, 2019

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 13, 2019

k8s-ci-robot closed this Oct 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the version package to utils #98

add the version package to utils #98

yastij commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

yastij commented Jul 12, 2019

andrewsykim commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

yastij commented Jul 12, 2019

dims commented Jul 12, 2019

dims commented Jul 12, 2019

thockin left a comment

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

lavalamp Oct 14, 2019

thockin Jul 12, 2019

thockin Jul 12, 2019

danwinship commented Jul 15, 2019 •

edited

fejta-bot commented Oct 13, 2019

yastij commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

lavalamp commented Oct 14, 2019

add the version package to utils #98

add the version package to utils #98

Conversation

yastij commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

yastij commented Jul 12, 2019

andrewsykim commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

yastij commented Jul 12, 2019

dims commented Jul 12, 2019

dims commented Jul 12, 2019

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Jul 15, 2019 • edited

fejta-bot commented Oct 13, 2019

yastij commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

lavalamp commented Oct 14, 2019

danwinship commented Jul 15, 2019 •

edited