simplify customhttperrors e2e test and add regression test and fix a bug #3765
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
size/L
| for i, codeSet := range errorCodes { | ||
| annotations := map[string]string{ | ||
| "nginx.ingress.kubernetes.io/custom-http-errors": strings.Join(codeSet, ","), | ||
| By("updating configuration when only custom-http-error value changes") |
There was a problem hiding this comment.
This is the regression test covering bug.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold @aledbf I've not pushed the fix yet :) Wanted to make sure e2e test fails without it. |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
removed
the
lgtm
|
/hold cancel @aledbf now it's ready. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
cc @jasongwartz |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi 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 |
|
|
||
| ing := framework.NewSingleIngress(host, "/test", host, f.IngressController.Namespace, "http-svc", 80, &annotations) | ||
| f.EnsureIngress(ing) | ||
| By("turning on proxy_intercept_errors directive") |
There was a problem hiding this comment.
Didn't know about this test format, much cleaner
|
@ElvinEfendi It's possible I've seen this behaviour but I wouldn't have known where to fix it. thanks! |
What this PR does / why we need it:
Currently when
custom-http-errorsannotation change, Nginx configuration won't be regenerated. The PR fixes the bug, includes respective regression test and also simplifies the whole test file to not deploy and delete ingress-nginx instance for every case.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: