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 Api fns for arbitrary subresources and approval subresource for CertificateSigningRequest #773

Merged
merged 11 commits into from
Jan 9, 2022

Conversation

ChinYing-Li
Copy link
Contributor

Motivation

This PR tackles two tasks in #127.

Solution

Implement Api functions for arbitrary subresource and Api approve/deny.
One test case of Api is added.
Should I try to come up with test cases for those Api subresource-related functions?
IMO, testing the logic of Request.*_subresource should be sufficient.
Any suggestion is appreciated!

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2021

Codecov Report

Merging #773 (16aefc7) into master (4d991fe) will decrease coverage by 0.08%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
- Coverage   71.99%   71.90%   -0.09%     
==========================================
  Files          54       54              
  Lines        3631     3677      +46     
==========================================
+ Hits         2614     2644      +30     
- Misses       1017     1033      +16     
Impacted Files Coverage Δ
kube-client/src/api/subresource.rs 55.96% <21.05%> (-7.38%) ⬇️
kube-client/src/api/util.rs 81.57% <66.66%> (-4.63%) ⬇️
kube-client/src/lib.rs 93.70% <100.00%> (+0.90%) ⬆️
kube-client/src/client/mod.rs 70.10% <0.00%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d991fe...16aefc7. Read the comment docs.

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

hey thanks again for all these prs!

i think the general setup here is ok, but have left a few comments on the main Api<CSR> abstraction. I feel it is trying to make a few too many assumptions for what should be in kube-client, and it's probably better to keep this simple and just wrap the newly added patch_subresource method.

(however, there may be an argument here for replicating what kubectl certificate approve as a small certificate helper in kube-runtime - but that is not necessary for this pr - and not even sure we need to do it).

kube-client/src/lib.rs Outdated Show resolved Hide resolved
kube-client/src/api/util.rs Outdated Show resolved Hide resolved
kube-client/src/api/util.rs Outdated Show resolved Hide resolved
kube-client/src/api/subresource.rs Show resolved Hide resolved
@clux clux changed the title Implement Api functions for arbitrary subresource and Api<CertificateSigningRequest> approve/deny Implement Api fns for arbitrary subresources and approval subresource for Api<CertificateSigningRequest> Jan 1, 2022
@clux clux added the changelog-add changelog added category for prs label Jan 1, 2022
kube-client/src/lib.rs Outdated Show resolved Hide resolved
@clux
Copy link
Member

clux commented Jan 8, 2022

This looks great now. Thanks for doing all the minor tweaks here to get it into a great state! I'll merge this in tomorrow if you are happy with it :-)

@ChinYing-Li
Copy link
Contributor Author

I think this is ready for merge. Thanks for helping!

@clux clux added this to the 0.66.0 milestone Jan 9, 2022
@clux clux merged commit f076f61 into kube-rs:master Jan 9, 2022
@clux clux mentioned this pull request Jan 9, 2022
11 tasks
@clux clux changed the title Implement Api fns for arbitrary subresources and approval subresource for Api<CertificateSigningRequest> Add Api fns for arbitrary subresources and approval subresource for Api<CertificateSigningRequest> Jan 15, 2022
@clux clux changed the title Add Api fns for arbitrary subresources and approval subresource for Api<CertificateSigningRequest> Add Api fns for arbitrary subresources and approval subresource for CertificateSigningRequest Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants