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 a kubectl like example #885

Merged
merged 19 commits into from May 3, 2022
Merged

Add a kubectl like example #885

merged 19 commits into from May 3, 2022

Conversation

clux
Copy link
Member

@clux clux commented Apr 18, 2022

Fun project for #884

Found a couple of minor api snags that can be improved, reported separately in:

NB: This branch does not rely on these PRs (however the behaviour is erratic without the determinism fix).

For #884

finding lots of little api snags that can be improved.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Apr 18, 2022 that may be closed by this pull request
11 tasks
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #885 (89f3e62) into master (fa11091) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   70.35%   70.37%   +0.02%     
==========================================
  Files          62       62              
  Lines        4264     4264              
==========================================
+ Hits         3000     3001       +1     
+ Misses       1264     1263       -1     
Impacted Files Coverage Δ
kube-runtime/src/wait.rs 70.00% <0.00%> (+2.00%) ⬆️

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux marked this pull request as ready for review April 18, 2022 18:11
Signed-off-by: clux <sszynrae@gmail.com>
examples/kubectl.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-exclude changelog excluded prs label Apr 27, 2022
@clux clux added this to the 0.72.0 milestone Apr 27, 2022
@nightkr
Copy link
Member

nightkr commented May 3, 2022

Sorry, I've been sick for the past week and haven't had the energy to look deeper here.. :/

@clux
Copy link
Member Author

clux commented May 3, 2022

Don't worry. This is pretty inconsequential.
Take care of yourself and get some rest :-)

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'll push up a branch with suggestions in a moment...

type KindMap = BTreeMap<String, (ApiResource, ApiCapabilities)>;
async fn discover_valid_inputs(client: Client) -> Result<KindMap> {
let mut kinds = BTreeMap::new(); // valid inputs (keys are plural or kind)
let discovery = Discovery::new(client).run().await?;
Copy link
Member

Choose a reason for hiding this comment

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

Not specific to this, but looks like Discovery::run fails if any aggregated service is unavailable, even if it's something relatively inconsequential like metrics.

Kubectl seems to be robust against this.

Copy link
Member Author

Choose a reason for hiding this comment

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

kubectl creates a cache of the discovery results in ~/.kube/cache. i didn't want to overdo this example though.

Copy link
Member

@nightkr nightkr May 3, 2022

Choose a reason for hiding this comment

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

Yeah, but even then I think kubectl will just log failures and keep going with the rest. Either way this is more of a kube-client issue than anything related to the kubectl example in particular.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, i misunderstood. you're not talking about general failures, but aggregated ones going out to separate services. that might be something we could tweak in the discovery interface in the future, yeah.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, exactly.

examples/kubectl.rs Outdated Show resolved Hide resolved
examples/kubectl.rs Outdated Show resolved Hide resolved
examples/kubectl.rs Outdated Show resolved Hide resolved
@nightkr
Copy link
Member

nightkr commented May 3, 2022

Ok, uploaded my suggestions to kubectl-example...teozkr:kubectl-example-/teozkr-suggestions, feel free to cherry pick or merge as you think makes sense.

@nightkr
Copy link
Member

nightkr commented May 3, 2022

I'd definitely include at least a minor note for this in the changelog, even if it's not something that affects the library crates directly this kind of dynamism is something that we've done a relatively poor job documenting and highlighting in our examples.

nightkr and others added 8 commits May 3, 2022 10:58
single-use index

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
kubectl actually gives preference to --all over -n
so they can technically be used together

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented May 3, 2022

Have cherry-picked (grabbed all) and updated as much as possible. Ended up reverting the namespace stuff to keep that part smaller (and deal with a kubectl edge case), but kept everything else. Great stuff!

I would ideally like to merge #887 first to be able to get deterministic discovery in here. But beyond that, am happy here. Pulled in the big env_logger to tracing_subscriber change as well, but can pull that out if needed.

@nightkr
Copy link
Member

nightkr commented May 3, 2022

FWIW the new resolution should be deterministic without sorting the list, because of the min_by_key.

@clux
Copy link
Member Author

clux commented May 3, 2022

Ah, that's true. I missed that part. Looks perfectly equivalent in this case. Happy to merge without it then.

@nightkr
Copy link
Member

nightkr commented May 3, 2022

Hm, might be worth adding a comment calling it out.

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented May 3, 2022

Hm, might be worth adding a comment calling it out.

something like this?

@clux clux merged commit bed451d into master May 3, 2022
@clux clux deleted the kubectl-example branch May 3, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-exclude changelog excluded prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a kubectl like example supporting core verbs
3 participants