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
client-go: extract new keyutil package from util/cert #71896
Conversation
/sig auth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @awly
/approve
/priority important-longterm
@awly @neolit123 is the package name |
@krmayankk |
I plan to end up with Clearly separating the two domains is valuable, otherwise it will turn back into what |
/assign @caesarxuchao @mikedanese |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quibble on the package name, otherwise lgtm.
/approve
I'll leave the lgtm to @mikedanese
@@ -0,0 +1,348 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have named the package as "key" instead of "keyutil", just like the "cert" package is not named as "certutil".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that key
has a much higher likelihood of name collision with types/vars.
You can see that https://github.com/kubernetes/kubernetes/search?q=k8s.io%2Fclient-go%2Futil%2Fcert+certutil&unscoped_q=k8s.io%2Fclient-go%2Futil%2Fcert+certutil cert
package is frequently aliased to certutil
for this reason.
ed81c60
to
9a3b50f
Compare
9a3b50f
to
4291690
Compare
I guess retesting will work |
1580449
to
402d8da
Compare
Thanks! /lgtm |
For approval. /assign @liggitt |
This package contains public/private key utilities copied directly from client-go/util/cert. All imports were updated. Future PRs will actually refactor the libraries. Updates kubernetes#71004
402d8da
to
1845839
Compare
Rebased. |
/skip |
@awly: The following tests failed for commit 1845839, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awly, caesarxuchao, liggitt, mikedanese, neolit123 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:
Approvers can indicate their approval by writing |
This package contains public/private key utilities copied directly from
client-go/util/cert. All imports were updated.
Future PRs will actually refactor the libraries.
Updates #71004
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: