Update nginx ingress chart #391
Conversation
k0da
requested review from
donovanmuller,
kuritka,
somaritane and
ytsarev
as code owners
|
Fixes https://github.com/AbsaOSS/k8gb/issues/388, note this is not a chart maintained by Nginx, but rather official for ingress controller on k8s side. Advantage over official is, it supports setting udp-services-configmap which is crucial in our setup. |
k0da
force-pushed
the
nginx_upgrade
branch
2 times, most recently
from
edb1516 to
e6027b6
Compare
|
@kuritka for terratest, it fails randomly |
|
@k0da, must be fixed otherwise, when we merge, master branch will fail randomly 😬 . |
|
@kuritka sure, I'm working on it. |
k0da
force-pushed
the
nginx_upgrade
branch
2 times, most recently
from
da87386 to
228fe43
Compare
k0da
force-pushed
the
nginx_upgrade
branch
3 times, most recently
from
3b7d752 to
0efd0a8
Compare
|
mergable |
Makefile
Outdated
| @@ -326,10 +326,10 @@ define deploy-local-cluster | |||
| --set k8gb.log.level=$(LOG_LEVEL) | |||
|
|
|||
| @echo "\n$(YELLOW)Deploy Ingress $(NC)" | |||
| helm repo add --force-update stable https://charts.helm.sh/stable | |||
| helm repo add nginx-stable https://kubernetes.github.io/ingress-nginx | |||
There was a problem hiding this comment.
--force-update was added to not fail locally when the repo is already present. Why was it removed?
There was a problem hiding this comment.
> helm repo add nginx-stable https://kubernetes.github.io/ingress-nginx
"nginx-stable" already exists with the same configuration, skipping```There was a problem hiding this comment.
Option is deprecated and no longer needed
There was a problem hiding this comment.
$ helm repo add nginx-stable https://kubernetes.github.io/ingress-nginx
Error: repository name (nginx-stable) already exists, please specify a different name
$ echo $?
1
There was a problem hiding this comment.
@ytsarev I just reverted it, since option is still there.
| admissionWebhooks: | ||
| enabled: false | ||
| patch: | ||
| enabled: false |
There was a problem hiding this comment.
Would be great to understand what this options do. Update of the ref about would help it I guess. Please fix the reference with the link to up-to-date documentation you used for this change
There was a problem hiding this comment.
@ytsarev this is something new chart adds, to prevent new features piled up, I just disable them to keep things as they were. In a nutshell, this prevents validation webhook being installed: https://github.com/kubernetes/ingress-nginx/blob/master/charts/ingress-nginx/templates/admission-webhooks/validating-webhook.yaml so far it supports networking.k8s.io only
There was a problem hiding this comment.
I won't pull it into this PR. We can enable it later. So IMO this is not a blocker
|
Any other comments? |
|
@k0da please rebase with master |
* https://charts.helm.sh/stable/ is outdated * Switch to https://github.com/kubernetes/ingress-nginx provided chart Signed-off-by: Dinar Valeev <dinar.valeev@absa.africa>
|
@ytsarev done |
|
@kuritka pls rereview |
Signed-off-by: Dinar Valeev dinar.valeev@absa.africa