-
Notifications
You must be signed in to change notification settings - Fork 44
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 attribute support for certificate subject #228
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @nzbr. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
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.
First of all: thank you for picking this up. And thanks obviously too for the initial work to @cornfeedhobo !
Second of all: Sorry it took so long for us to pick this up!
I just spotted this by chance. This looks awesome, and I think we should get this merged so it's not hanging in limbo. It looks like there are a couple of CI issues - if you could fix those, I'd be happy to merge.
Would it make sense to replace the splitList function with SplitWithEscapeCSV as well?
It would, but I'm gonna say we shouldn't do that in this PR. I actually spotted the issues with splitList earlier today by pure coincidence when I raised #254 - we should fix that, but it's pretty low impact because the only affected field is URIs as far as I can think and I just can't imagine the bugs there being hit that often.
I've raised #256 to track that - but that should be its own PR for sure!
Also I think #254 will merge before this and create a merge conflict - sorry about that, but it should be an easy fix! |
Fixes cert-manager#128 Signed-off-by: cornfeedhobo <[email protected]>
Signed-off-by: nzbr <[email protected]>
Signed-off-by: nzbr <[email protected]>
Signed-off-by: nzbr <[email protected]>
Signed-off-by: nzbr <[email protected]>
Signed-off-by: nzbr <[email protected]>
Signed-off-by: nzbr <[email protected]>
/retest |
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.
/lgtm
/approve
I think this is an improvement and I really appreciate you doing this! I think this could use a few more tests - ideally one for each subject key - but I don't think we need to block this any more. Thanks for your patience!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish 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 |
c1dbae3
into
cert-manager:main
Fixes #128
This PR is a continuation of the work in #129
I have changed the code of the
RequestForMetadata
function to use the CSV parser instead ofstrings.Split
as requested in the original PR.Would it make sense to replace the
splitList
function withSplitWithEscapeCSV
as well? I wasn't sure if that's wanted because that code is not from #129 but was already in there before, so I left it untouched for now.And what else needs to be done in order to get this merged?
I successfully tested it with the following manifest:
resulting in the following output: