-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Docs: Specify ingressClass
for multi-controller setup.
#11493
Conversation
The committers listed above are authorized under a signed CLA. |
Welcome @kiblik! |
Hi @kiblik. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
✅ Deploy Preview for kubernetes-ingress-nginx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@kiblik thanks for your contribution. The md file you are changing, actually needs many more fixes. Please look at this https://kubernetes.github.io/ingress-nginx/faq/#how-can-i-easily-install-multiple-instances-of-the-ingress-nginx-controller-in-the-same-cluster Is it possible for you to fix your MD file as per the info in this FAQ section link Thanks |
Also please take care that this argument is for the old way of assigning Ingress resources via an annotation and not via the IngressClass resource. Not sure if we should still support it and tell users to configure it as this might lead to the assumption that this way is still fully supported and ok. |
@longwuyuan pinged me and we had a quick talk about my comment. Could you please add a small note about this flag / the annotation way of assigning Ingresses is still supported but not the preferred way? That would be awesome! |
@kiblik annotation for ingressClass is not preferred but supported |
/triage accepted |
ingressClass
for multi-controller setup.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Gacko, kiblik 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 |
/cherry-pick release-1.10 |
@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you. In response to this:
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-sigs/prow repository. |
@Gacko: new pull request created: #11520 In response to this:
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-sigs/prow repository. |
What this PR does / why we need it:
For correct handling,
ingressClass
needs to be mentioned as well. Becauseingress-nginx/charts/ingress-nginx/templates/_params.tpl
Line 19 in 5784be2
.Values.controller.ingressClass
, not based on.Values.controller.ingressClassResource.name
Types of changes
Which issue/s this PR fixes
Not mentioned in any issue
How Has This Been Tested?
Only documentation
Checklist: