Sort API Services by Kube-Version order #64004

mbohlool · 2018-05-18T01:00:33Z

Sort API services based on kube-version format. If the version does not have kube-version format.

kube-version format: v[MajorVersion]((alpha|beta)[minorVersion])?
e.g. v1alpha1, v4, v21beta12

Sort base on:
Version type first: GA>Beta>Alpha
Major version then Minor version (if exists).

APIServices with kube-like versions (e.g. v1, v2beta1, etc.) will be sorted appropriately within each group.

mbohlool · 2018-05-18T01:03:34Z

@sttts @liggitt @lavalamp @deads2k

liggitt · 2018-05-18T01:04:32Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/helpers.go

+
+func parseKubeVersion(v string) (majorVersion int, vType versionType, minorVersion int, ok bool) {
+	var err error
+	re := regexp.MustCompile("^v([\\d]+)(?:(alpha|beta)([\\d]+))?$")


move compile out to a package var and reuse

liggitt · 2018-05-18T01:04:54Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/helpers.go

+func parseKubeVersion(v string) (majorVersion int, vType versionType, minorVersion int, ok bool) {
+	var err error
+	re := regexp.MustCompile("^v([\\d]+)(?:(alpha|beta)([\\d]+))?$")
+	submatches := re.FindStringSubmatch(strings.ToLower(v))


don't lowercase the string, should match exactly

what should we do if the version is v1Beta1? options: 1. consider it out of format, 2. consider it in-format and put it before v1beta1?

out of format

liggitt · 2018-05-18T01:15:05Z

need to update the docs on the version priority field in APIService

liggitt · 2018-05-18T01:15:52Z

a few nits, lgtm otherwise

dixudx · 2018-05-18T05:41:30Z

staging/src/k8s.io/apimachinery/pkg/version/helpers.go

+
+func parseKubeVersion(v string) (majorVersion int, vType versionType, minorVersion int, ok bool) {
+	var err error
+	re := regexp.MustCompile("^v([\\d]+)(?:(alpha|beta)([\\d]+))?$")


What about rc releases?

The idea is to support kubernetes like versioning. I don't think we have rc in our API object's versioning.

mbohlool · 2018-05-18T07:22:21Z

/test pull-kubernetes-kubemark-e2e-gce

mbohlool · 2018-05-18T07:23:34Z

/test pull-kubernetes-e2e-gce

sttts · 2018-05-18T07:24:31Z

staging/src/k8s.io/apimachinery/pkg/version/helpers.go

+
+const (
+	// Bigger the version type number, higher priority it is
+	versionTypeAlpha versionType = 0


this should be all iota, not zero: http://programming.guide/go/iota.html. Otherwise, alpha and beta are both zero.

nvmd. iota is the index of the const declaration in the const block.

sttts · 2018-05-18T08:23:23Z

staging/src/k8s.io/apimachinery/pkg/version/helpers.go

+	case ok1 && !ok2:
+		return 1
+	}
+	if v1type != v2type {


nit: confusing logic. Why not simple as:

if v1type != v2type { return int(v1type) - int(v2type) } if v1major != v2major { return v1major - v2major } return v1minor - v2minor

that works too and simpler. fixed.

sttts · 2018-05-18T09:04:04Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1/types.go

@@ -68,6 +68,11 @@ type APIServiceSpec struct {
 	// The primary sort is based on VersionPriority, ordered highest to lowest (20 before 10).
 	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
 	// Since it's inside of a group, the number can be small, probably in the 10s.
+	// In case of equal (or lack of) version priorities, the version string will be used to compute the order inside a group.


what is "lack of" ? It default to zero.

sttts · 2018-05-18T09:05:53Z

Only a doc nit. Otherwise lgtm.

deads2k · 2018-05-18T11:58:39Z

staging/src/k8s.io/apimachinery/pkg/version/helpers_test.go

+		{"v1alpha2", "v1alpha1", true},
+		{"v1beta1", "v2alpha3", true},
+		{"v1alpha10", "v1alpha2", true},
+		{"v1beta10", "v1beta2", true},


Add cases comparing kubversion against unformatted string and two unformatted strings please.

deads2k · 2018-05-18T12:02:10Z

Just a request for a missing test case. lgtm

mbohlool · 2018-05-18T18:46:05Z

Thanks @deads2k @sttts @liggitt. I addressed your comments. PTAL.

lavalamp

A few nits, sorry!

lavalamp · 2018-05-18T20:12:11Z

staging/src/k8s.io/apimachinery/pkg/version/helpers.go

+	// Bigger the version type number, higher priority it is
+	versionTypeAlpha versionType = iota
+	versionTypeBeta  versionType = iota
+	versionTypeGA    versionType = iota


Omit the second two iotas?

lavalamp · 2018-05-18T20:12:51Z

staging/src/k8s.io/apimachinery/pkg/version/helpers_test.go

+		{"v1beta10", "v1beta2", true},
+		{"foo", "v1beta2", false},
+		{"bar", "foo", true},
+		{"version1", "version2", true}, // Non kube-like versions are sorted alphabetically


Add foo10 and foo1 or something else to demonstrate that it's lexicographic.

lavalamp · 2018-05-18T20:14:40Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/helpers_test.go

+		{
+			name:     "case4",
+			versions: []string{"v1", "v2", "test", "final"},
+			expected: []string{"v2", "v1", "final", "test"},


and maybe one here that also demonstrates lexicographic ordering.

lavalamp · 2018-05-18T20:15:29Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/v1/types.go

@@ -68,6 +68,11 @@ type APIServiceSpec struct {
 	// The primary sort is based on VersionPriority, ordered highest to lowest (20 before 10).
 	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)
 	// Since it's inside of a group, the number can be small, probably in the 10s.
+	// In case of equal version priorities, the version string will be used to compute the order inside a group.
+	// If the version string is kube-like, it will be used for ordering otherwise lexical order will be used.


If the version string is "kube-like", it will sort above non "kube-like" version strings, which are ordered lexicographically. "Kube-like" versions start with a "v", then are followed by a number (the major version), then optionally the string "alpha" or "beta" and another number (the minor version). These are sorted first by GA > beta > alpha, and then within each group by comparing major version, then minor version. An example sorted list of versions: v10, v2, v1, v11beta2, v10beta3, v3beta1, v12alpha1, v11alpha2, foo1, foo10.

lavalamp · 2018-05-18T20:48:29Z

/approve

If we could get one last set of eyes to review the comment wording I proposed before we merge this that would be great (@liggitt?).

mbohlool · 2018-05-18T21:19:58Z

/retest

mbohlool · 2018-05-19T04:41:30Z

/retest

liggitt · 2018-05-19T23:53:04Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

@@ -68,6 +68,12 @@ type APIServiceSpec struct {
 	// The primary sort is based on VersionPriority, ordered highest to lowest (20 before 10).
 	// The secondary sort is based on the alphabetical comparison of the name of the object.  (v1.bar before v1.foo)


Drop this line, the adjusted secondary sort behavior is described below

liggitt · 2018-05-19T23:55:25Z

staging/src/k8s.io/kube-aggregator/pkg/apis/apiregistration/types.go

+	// If the version string is "kube-like", it will sort above non "kube-like" version strings, which are ordered
+	// lexicographically. "Kube-like" versions start with a "v", then are followed by a number (the major version),
+	// then optionally the string "alpha" or "beta" and another number (the minor version). These are sorted first
+	// by GA > beta > alpha, and then within each group by comparing major version, then minor version. An example


“group” in this context means ga/beta/alpha, but is easily confused with API group. Maybe just drop “within each group”?

liggitt · 2018-05-19T23:56:31Z

If we could get one last set of eyes to review the comment wording I proposed before we merge this that would be great (@liggitt?).

one bit of the old field doc needs removing, one bit of the new I found confusing but don’t feel strongly about

mbohlool · 2018-05-20T10:15:28Z

/test pull-kubernetes-e2e-kops-aws

mbohlool · 2018-05-20T10:15:43Z

@liggitt Please take another look.

liggitt · 2018-05-20T12:17:27Z

/lgtm

k8s-ci-robot · 2018-05-20T12:17:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, liggitt, mbohlool

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [lavalamp,liggitt]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp,liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [lavalamp]

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

fejta-bot · 2018-05-20T15:15:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-05-20T19:27:10Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-05-20T22:57:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

mbohlool · 2018-05-21T00:30:45Z

/test pull-kubernetes-e2e-gce

mbohlool · 2018-05-21T01:47:15Z

/retest

k8s-github-robot · 2018-05-21T02:45:17Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-05-21T03:43:24Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2018

k8s-ci-robot requested review from deads2k and liggitt May 18, 2018 01:00

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2018

mbohlool force-pushed the apiservice_order branch from c1e09fa to 2585c77 Compare May 18, 2018 01:02

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 18, 2018

mbohlool force-pushed the apiservice_order branch from 2585c77 to ba4b3c8 Compare May 18, 2018 01:14

liggitt reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch from ba4b3c8 to 1808a7c Compare May 18, 2018 01:45

dixudx reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch 2 times, most recently from 11ef8c6 to 1187391 Compare May 18, 2018 06:15

sttts reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch from 1187391 to 0969d8f Compare May 18, 2018 07:33

sttts reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch from 0969d8f to ba4c7d9 Compare May 18, 2018 08:42

sttts reviewed May 18, 2018

View reviewed changes

deads2k reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch from ba4c7d9 to 12fd47b Compare May 18, 2018 18:45

lavalamp reviewed May 18, 2018

View reviewed changes

mbohlool force-pushed the apiservice_order branch from 12fd47b to 19e1ad7 Compare May 18, 2018 20:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2018

mbohlool force-pushed the apiservice_order branch from 19e1ad7 to bfd0cbe Compare May 18, 2018 21:26

liggitt reviewed May 19, 2018

View reviewed changes

mbohlool added 2 commits May 20, 2018 01:40

Sort API Services by Kube-Version order

97ada15

Generated files

cc87ba3

mbohlool force-pushed the apiservice_order branch from bfd0cbe to cc87ba3 Compare May 20, 2018 08:40

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 21, 2018

k8s-github-robot merged commit b87105a into kubernetes:master May 21, 2018

mbohlool mentioned this pull request May 21, 2018

Crd versioning with nop Conversion #63830

Merged

		@@ -68,6 +68,12 @@ type APIServiceSpec struct {
		// The primary sort is based on VersionPriority, ordered highest to lowest (20 before 10).
		// The secondary sort is based on the alphabetical comparison of the name of the object. (v1.bar before v1.foo)

Sort API Services by Kube-Version order #64004

Sort API Services by Kube-Version order #64004

Conversation

mbohlool commented May 18, 2018 • edited Loading

mbohlool commented May 18, 2018

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

liggitt commented May 18, 2018

liggitt commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented May 18, 2018

mbohlool commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented May 18, 2018

Choose a reason for hiding this comment

deads2k commented May 18, 2018

mbohlool commented May 18, 2018

lavalamp 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

lavalamp commented May 18, 2018

mbohlool commented May 18, 2018

mbohlool commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented May 19, 2018

mbohlool commented May 20, 2018

mbohlool commented May 20, 2018

liggitt commented May 20, 2018

k8s-ci-robot commented May 20, 2018

fejta-bot commented May 20, 2018

fejta-bot commented May 20, 2018

fejta-bot commented May 20, 2018

mbohlool commented May 21, 2018

mbohlool commented May 21, 2018

k8s-github-robot commented May 21, 2018

k8s-github-robot commented May 21, 2018

mbohlool commented May 18, 2018 •

edited

Loading

sttts May 18, 2018 •

edited

Loading