-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
cert-manager controller args: #10049
cert-manager controller args: #10049
Conversation
|
Welcome @phunyguy! |
Hi @phunyguy. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
@phunyguy Thank you for the PR, could you check the fail
I was going to say that you should add a check for undefined var but the job catch it first ;) |
Yep... Silly move on my part, I know better. I'll fix it up. |
@floryut It looks like on a rerun there was an unrelated error. Is there something else I should be doing here? |
@@ -947,6 +947,9 @@ spec: | |||
- --v=2 | |||
- --cluster-resource-namespace=$(POD_NAMESPACE) | |||
- --leader-election-namespace={{ cert_manager_leader_election_namespace }} | |||
{% for extra_arg in cert_manager_controller_extra_args | default([]) %} |
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.
Hi quick nits, could you rather default this new var to empty array in roles/kubernetes-apps/ingress_controller/cert_manager/defaults/main.yml
, apart from that lgtm. Thanks!
Also for the CI to work please rebase the PR, apart from that it's unfortunately a matter of if you're lucky or not (or rather the CI is not running too many tasks at once)... |
Hi! Last thing could you squash your commits? Apart from that looks good :)! |
- Adding in the ability to feed extra-args to cert-manager-controller.
44567a1
to
6336c74
Compare
I think I figured that out. Still new to this. Thanks for the feedback! |
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 for doing this.
/ok-to-test
/approve
Could you rebase this pull request on the latest master branch? |
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 for the contribution! Next time you should use git fetch origin && git rebase origin master
instead of git merge
to pull latest changes from master but as we squash while merging should be fine this time, let's not wait for another CI run IMO.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrFreezeex, oomichi, phunyguy 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 |
- Adding in the ability to feed extra-args to cert-manager-controller.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: