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.

Sign up for GitHub

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

Jump to bottom

Add Discovery::groups_alphabetical following kubectl sort order #887

Merged
merged 12 commits into from
May 23, 2022
Merged

Add Discovery::groups_alphabetical following kubectl sort order #887

merged 12 commits into from
May 23, 2022

Conversation

clux
Copy link
Member

@clux clux commented Apr 18, 2022 •
edited
Loading

Simple thing. Fixes #886

@clux clux added this to the 0.72.0 milestone Apr 18, 2022
@clux clux added the changelog-fix changelog fix category for prs label Apr 18, 2022
@clux clux requested a review from a team April 18, 2022 16:08
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022 •
edited
Loading

Codecov Report

Merging #887 (7796464) into master (e5300c0) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
+ Coverage   70.57%   70.68%   +0.10%     
==========================================
  Files          64       64              
  Lines        4337     4343       +6     
==========================================
+ Hits         3061     3070       +9     
+ Misses       1276     1273       -3
Impacted Files Coverage Δ
kube-client/src/discovery/mod.rs 90.00% <100.00%> (+0.81%) ⬆️
kube/src/lib.rs 87.31% <100.00%> (+0.29%) ⬆️
kube-runtime/src/wait.rs 73.07% <0.00%> (+1.92%) ⬆️
kube-client/src/discovery/apigroup.rs 84.50% <0.00%> (+2.81%) ⬆️

@clux clux mentioned this pull request Apr 18, 2022
Merged
@clux clux changed the title Make Discovery::groups have deterministic order Make Discovery::groups follow kubectl sort order Apr 19, 2022
@clux
Copy link
Member Author

clux commented Apr 19, 2022

Updated to match updated idea from #886 (comment)

clux added 7 commits April 24, 2022 08:48
@clux
Make Discovery::groups have deterministic order
3c2edaf 
Fixes #886

Signed-off-by: clux <sszynrae@gmail.com>
@clux
sort Discovery::groups in kubectl order
dbe1604 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
revert back to hashmap (no need for btree anymore)
fa8af57 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
use peek rather than creating group values twice
5f3398a 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
document
4461d57 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
update with suggestions
9b4a5d3 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
update test
41a5bf8 
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Apr 24, 2022 •
edited
Loading

Updated with suggestion from #886 (comment)

EDIT: added some clarifying comments + questions.

@clux
remove ordering fn since it's simple
9b7e173 
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Make Discovery::groups follow kubectl sort order Add Discovery::groups_alphabetical following kubectl sort order Apr 24, 2022
@clux clux added changelog-add changelog added category for prs and removed changelog-fix changelog fix category for prs labels Apr 24, 2022
@clux clux removed this from the 0.72.0 milestone May 3, 2022
nightkr
nightkr approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

I'm still not really convinced that this is the right path to make ergonomic, but for what it is it should be fine.

@clux clux added this to the 0.73.0 milestone May 23, 2022
@clux
Copy link
Member Author

clux commented May 23, 2022

yeh, pretty meh, but at least it's done as a purely additive opt-in now. anyway, moving on to something more interesting.

@clux clux merged commit 56b173f into master May 23, 2022
@clux clux deleted the discovery-fix branch May 23, 2022 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nightkr nightkr nightkr approved these changes

Assignees
No one assigned
Labels
changelog-add changelog added category for prs
Projects
None yet
Milestone
0.73.0
Development

Successfully merging this pull request may close these issues.

Groups from api discovery returned in non-deterministic order
3 participants
@clux @codecov-commenter @nightkr