-
Notifications
You must be signed in to change notification settings - Fork 2k
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 support for IngressClass resources #1133
Conversation
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.
@lucacome This looks great!
Just one thing to make you aware of, is that you can set a compatible kubernetes version in your Chart.yaml. https://helm.sh/docs/topics/charts/#the-chartyaml-file
Example: kubeVersion: 1.15-1.19
.
This would reject the installation of a helm release at the helm client level. I think this could be a nice improvement to include as it shortens the feedback loop for a user. (They don't have to install incompatible resources into their cluster to see logs that their cluster is incompatible.)
$ helm install test deployments/helm-chart
Error: chart requires kubeVersion: 1.15-1.19 which is incompatible with Kubernetes v1.14.0
$ helm ls
$
Feel free to ignore, we can always refer back to: https://docs.nginx.com/nginx-ingress-controller/technical-specifications/#supported-kubernetes-versions and hope most users read that.
Co-authored-by: Dean Coakley <dean.s.coakley@gmail.com>
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 @lucacome
Please see my comments and suggestions. Most of them about docs. Looks good overall.
Additionally,
(1) We're changing behavior for VirtualServer/VirtualServerRoutes, right? Previously, if `"-use-ingress-class-only" was enabled, the IC would ignore VS/VSRs with empty class. Now it won't ignore them. If that's the case, could you add the change label to the PR? We will also need to document that change in the changelog.
(2) Could you include in the description that you're fixing this #924 ?
docs-web/configuration/global-configuration/command-line-arguments.md
Outdated
Show resolved
Hide resolved
docs-web/configuration/ingress-resources/basic-configuration.md
Outdated
Show resolved
Hide resolved
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 pending #1133 (comment)
(Sorry if my example misled you @lucacome 😓 )
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
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.
@lucacome looks good. Could you fix the mentioned link problems before merging?
Supporting
IngressClass
resource andingressClassName
for versions of k8s >= 1.18.When using version of k8s >= 1.18 the
use-ingress-class-only
is now ignored and the Ingress Controller will only handle resources with the appropriate class set.Fixes #924