Add e2e Tests for Proxy Annotations #3149
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
diazjf
force-pushed
the
proxy-e2e-tests
branch
from
85e1e2f
to
81434a1
Compare
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
diazjf
force-pushed
the
proxy-e2e-tests
branch
6 times, most recently
from
d820a63
to
d279415
Compare
|
@diazjf looks like most of these tests share the same logic, can you extract the logic into a helper function and reuse the function instead of duplication? |
| func(server string) bool { | ||
| return Expect(server).Should(ContainSubstring("proxy_read_timeout 20s;")) | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Any reason why you aren't asserting all in the same place? i.e something like
err = f.WaitForNginxServer(host,
func(server string) bool {
return strings.Contains(server, "proxy_connect_timeout 50s;") && strings.Contains(server, "proxy_send_timeout 20s;") && strings.Contains(server, "proxy_read_timeout 20s;")
})
Expect(err).NotTo(HaveOccurred())
There was a problem hiding this comment.
I wanted to keep things consistent. Your suggestion makes more sense, it'll make the tests a little faster and its still readable. Thanks 💯
diazjf
force-pushed
the
proxy-e2e-tests
branch
2 times, most recently
from
ba5690a
to
7fcd0b8
Compare
|
/hold remove |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
| }, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(ing).NotTo(BeNil()) |
There was a problem hiding this comment.
I think all these ingress creations with annotations can be extracted into a local helper function to avoid duplication:
somethine like:
func ensureIngress(f *framework.Framework, annotations: map[string]string) *extensions.Ingress
There was a problem hiding this comment.
Done, thanks. Will refactor and use this as a util function for other tests in another PR.
Adds e2e tests for the following annotations: - proxy-body-size - proxy-connect-timeout - proxy-send-timeout - proxy-read-timeout - proxy-buffering - proxy-buffer-size - proxy-request-buffering - proxy-next-upstream - proxy-next-upstream-tries - proxy-cookie-domain - proxy-cookie-path and also updates some documentation.
diazjf
force-pushed
the
proxy-e2e-tests
branch
from
7fcd0b8
to
c981a65
Compare
Removes quotes from nginx directives which my cause issues with their functionality Fixes kubernetes#3152
|
@ElvinEfendi good suggestion. |
|
@ElvinEfendi @aledbf this one is good to go 👍 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, diazjf 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 |
k8s-ci-robot
added
the
approved
|
@diazjf thanks! |
Adds e2e tests for the following annotations:
and also updates some documentation.
Fixes #3152