Export Version priority parser with Ord impls in kube_core #764

clux · 2021-12-21T11:03:27Z

This does the self-comment cleanup steps described to make this worthy of exporting:

reverse Ord impl so we have semantic coherence (stable 2 > stable 1)
reverse sort usage internally to compensate (ascending version order default, matches rust conventions)

Also adds a small doctest for the thing, and an infallible FromStr impl (which may end up being useful to someone - maybe).

clux · 2021-12-21T11:07:14Z

kube-core/src/version.rs

@@ -137,7 +207,7 @@ mod tests {
            Version::Stable(2),
            Version::Beta(2, Some(3)),
        ];
-        vers.sort();
+        vers.sort_by_cached_key(|x| Reverse(x.clone()));


This is the only semi-annoying consequence of the reversal here.
A clone is forced on us from rust-lang/rust#34162 unless we use the manual cmp calll reversed ourselves.

I think this is ok, the usage in kube_client does not need to clone because the Version parsing IS the temporary step, and the test here is probably less realistic of a use-case)

Ord has a blanket impl that &T: Ord where T: Ord, so I think this should line up correctly. Or do the lifetimes not match properly?

End up getting this :/

error: lifetime may not live long enough --> kube-core/src/version.rs:208:37 | 208 | vers.sort_by_cached_key(|x| Reverse(x)); | -- ^^^^^^^^^^ returning this value requires that `'1` must outlive `'2` | || | |return type of closure is Reverse<&'2 version::Version> | has type `&'1 version::Version`

nightkr · 2021-12-21T14:05:11Z

kube-core/src/version.rs

    Alpha(u32, Option<u32>),
-    // CRDs and APIServices can use arbitrary strings as versions.
+    /// An non-conformant api string (sorted lexicographically)


This isn't really new, but this might be worth reconsidering before stabilizing. Keeping this "everything else" case means that anyone relying on it will silently be broken if we add new primary cases.

Instead, we might want to turn Nonconformant into the error case on <Version as FromStr>?

According to Version Priority any non-conformant value is legal. I may want to set my CRD version to testing and expect it to work with k8s. So failing the parser on that will make it unusable from kube

To be clear: I am not saying that a non-conformant version is illegal, or that we should reject working with it. Rather, that we should treat it as an opaque String in general, parse it into a Version when needed (such as for comparisons), and accept that that parsing may fail (with reasonable fallback behaviour, as described in the PR body).

kube-core/src/version.rs

nightkr · 2021-12-21T14:15:41Z

kube-core/src/version.rs

@@ -137,7 +207,7 @@ mod tests {
            Version::Stable(2),
            Version::Beta(2, Some(3)),
        ];
-        vers.sort();
+        vers.sort_by_cached_key(|x| Reverse(x.clone()));


Ord has a blanket impl that &T: Ord where T: Ord, so I think this should line up correctly. Or do the lifetimes not match properly?

kube-core/src/version.rs

doing the cleanup steps described to make this worthy of exporting. needed for kube-rs/kopium#23 Signed-off-by: clux <sszynrae@gmail.com>

Signed-off-by: clux <sszynrae@gmail.com>

* Explicitly request which ordering to use Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Add `Version::latest` that includes considers v2 > v2beta1 > v1 Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Rename Version::latest_stable to priority Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Also rename LatestStable accordingly Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

we had two orders ::latest and ::priority, but v.latest() was a ambiguous. v.latest could mean: - we are sorting by age (but that's not true) - we are sorting by latest version in semver semantics (true so think v.semantic() is slightly more clear. v2.semantic() > v1.semantic() feels a little easier to comprehend than v2.latest() > v1.latest() Signed-off-by: clux <sszynrae@gmail.com>

kube-core/src/version.rs

Signed-off-by: clux <sszynrae@gmail.com>

nightkr

LGTM, just a few minor nits remaining

kube-client/src/discovery/apigroup.rs

kube-core/src/version.rs

See kube-rs#764 (comment)

See kube-rs#764 (comment) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

See #764 (comment) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de>

clux · 2021-12-26T17:14:10Z

Thanks for all the diligent review here 👍

Happy to merge at this point.

As a funny side-note about this naming: Nonconformant("Z").generation() is now a thing we can ask for.

kube-core/src/version.rs

Co-authored-by: kazk <kazk.dev@gmail.com>

clux · 2021-12-27T05:22:32Z

CI instability from github's side this morning. Either it's the cargo deny locking up in a docker run, or k3d failing to start the cluster, so guessing there are some temporary docker issues on some actions runners. In either case everything here has passed checks. Merging.

clux added the changelog-add changelog added category for prs label Dec 21, 2021

clux commented Dec 21, 2021

View reviewed changes

clux requested a review from a team December 21, 2021 11:07

clux mentioned this pull request Dec 21, 2021

This is my take on version comparison kube-rs/kopium#24

Closed

This comment has been minimized.

Sign in to view

nightkr reviewed Dec 21, 2021

View reviewed changes

kazk reviewed Dec 22, 2021

View reviewed changes

kube-core/src/version.rs Outdated Show resolved Hide resolved

clux force-pushed the version-export branch from 1e9553b to 9942b0d Compare December 22, 2021 08:07

clux added 4 commits December 22, 2021 17:34

Export Version parser with Ord impl in kube_core

f476cb4

doing the cleanup steps described to make this worthy of exporting. needed for kube-rs/kopium#23 Signed-off-by: clux <sszynrae@gmail.com>

remove leftover TODO + improve doc links between discovery and core

2626a71

Signed-off-by: clux <sszynrae@gmail.com>

import more tests, fix one, better docs

3082efa

Signed-off-by: clux <sszynrae@gmail.com>

a bit less Reverse and less double negatives

e8061ff

Signed-off-by: clux <sszynrae@gmail.com>

clux force-pushed the version-export branch from 9942b0d to e8061ff Compare December 22, 2021 10:34

nightkr mentioned this pull request Dec 22, 2021

Version export - explicitly request ordering #767

Merged

nightkr and others added 3 commits December 24, 2021 09:04

Merge branch 'master' into version-export

f69d422

kazk reviewed Dec 24, 2021

View reviewed changes

kube-core/src/version.rs Outdated Show resolved Hide resolved

kazk reviewed Dec 24, 2021

View reviewed changes

kube-core/src/version.rs Outdated Show resolved Hide resolved

nightkr reviewed Dec 25, 2021

View reviewed changes

kube-core/src/version.rs Outdated Show resolved Hide resolved

clux added 3 commits December 26, 2021 23:00

fix self-references and rename semantic to distance

9b81b05

Signed-off-by: clux <sszynrae@gmail.com>

Merge branch 'master' into version-export

3874758

rename to generation

036d028

Signed-off-by: clux <sszynrae@gmail.com>

clux changed the title ~~Export Version priority parser with Ord impl in kube_core~~ Export Version priority parser with Ord impls in kube_core Dec 26, 2021

nightkr approved these changes Dec 26, 2021

View reviewed changes

kube-client/src/discovery/apigroup.rs Outdated Show resolved Hide resolved

kube-client/src/discovery/apigroup.rs Outdated Show resolved Hide resolved

kube-core/src/version.rs Outdated Show resolved Hide resolved

nightkr added a commit to nightkr/kube-rs that referenced this pull request Dec 26, 2021

We need to split on byte lengths, not code point counts

fed4850

See kube-rs#764 (comment)

nightkr mentioned this pull request Dec 26, 2021

Export Version / Unicode in parsing #772

Merged

nightkr added a commit to nightkr/kube-rs that referenced this pull request Dec 26, 2021

We need to split on byte lengths, not code point counts

9069649

See kube-rs#764 (comment) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

nightkr and others added 2 commits December 26, 2021 17:09

We need to split on byte lengths, not code point counts (#772)

b5e9a8a

See #764 (comment) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Apply suggestions from code review

1bb9d53

Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de>

kazk approved these changes Dec 26, 2021

View reviewed changes

kube-core/src/version.rs Outdated Show resolved Hide resolved

Update kube-core/src/version.rs

c77e2de

Co-authored-by: kazk <kazk.dev@gmail.com>

clux merged commit 4017a3f into master Dec 27, 2021

clux deleted the version-export branch December 27, 2021 05:22

clux mentioned this pull request Dec 27, 2021

select version by priority rather than picking first kube-rs/kopium#27

Merged

clux added this to the 0.66.0 milestone Jan 15, 2022

soenkeliebau mentioned this pull request Jan 17, 2022

Update kube-rs from 0.65.0 to 0.66.0 stackabletech/operator-rs#295

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export Version priority parser with Ord impls in kube_core #764

Export Version priority parser with Ord impls in kube_core #764

clux commented Dec 21, 2021

clux Dec 21, 2021

nightkr Dec 21, 2021

clux Dec 22, 2021

This comment has been minimized.

nightkr Dec 21, 2021

imp Dec 21, 2021

nightkr Dec 21, 2021

nightkr Dec 21, 2021

nightkr left a comment

clux commented Dec 26, 2021

clux commented Dec 27, 2021

Export Version priority parser with Ord impls in kube_core #764

Export Version priority parser with Ord impls in kube_core #764

Conversation

clux commented Dec 21, 2021

clux Dec 21, 2021

Choose a reason for hiding this comment

nightkr Dec 21, 2021

Choose a reason for hiding this comment

clux Dec 22, 2021

Choose a reason for hiding this comment

This comment has been minimized.

nightkr Dec 21, 2021

Choose a reason for hiding this comment

imp Dec 21, 2021

Choose a reason for hiding this comment

nightkr Dec 21, 2021

Choose a reason for hiding this comment

nightkr Dec 21, 2021

Choose a reason for hiding this comment

nightkr left a comment

Choose a reason for hiding this comment

clux commented Dec 26, 2021

clux commented Dec 27, 2021