-
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 health status uri customisation #750
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.
@Dean-Coakley thx for the PR!
please see a few comments
additionally, If the configured URI is not a valid NGINX location, the IC must exit reporting an error.
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.
As @pleshakov says we need validation, other than that lgtm! 👍
Sorry for missing that. Good catch. Should be ok now. |
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.
@Dean-Coakley
thx.
I left a suggestion.
Additionally, just realized that the update of the helm chart doc https://github.com/nginxinc/kubernetes-ingress/blob/master/deployments/helm-chart/README.md is missing :( no doc for healthStatusURI
param.
Update. sorry, just realized that /
should not be allowed as the location, because we have /
in the default server config -- https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/version1/nginx-plus.tmpl#L115
Make sure to cover that case. I suggest you also add a unit test for validateLocation
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 once addressed what @pleshakov pointed, approving! 🚀
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.
👍
Proposed changes
Fixes: #721
Allow customisation of health check uri via cli arg.
-health-check-uri
cli argument.controller.healthCheckURI
field to helm chart.Checklist
Before creating a PR, run through this checklist and mark each as complete.