add more testing for mergeAlternativeBackends #3446
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@clandry94 could you rebase with master? |
clandry94
force-pushed
the
add_tests_merge_canary
branch
4 times, most recently
from
748654a
to
81849d0
Compare
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
you can simply do
return primary.Name != alternative.Name && !primary.NoServer
| } | ||
|
|
||
| // Performs the merge action and checks to ensure that one two alternative backends do not merge into each other | ||
| func mergeAlternativeBackend(priUps *ingress.Backend, altUps *ingress.Backend) bool { |
There was a problem hiding this comment.
Why are you referring backends as upstreams? When you log below you correctly call them alternative and primary backend.
There was a problem hiding this comment.
Never mind - looks like they're interchangeably used everywhere in this file. We can revisit this later.
There was a problem hiding this comment.
however it'd be great if you can make the signature of this function consistent with https://github.com/kubernetes/ingress-nginx/pull/3446/files#diff-86ac9ff9d75a0c5005c116e766a6127dR1118
- fix edge cases caught by new testing
clandry94
force-pushed
the
add_tests_merge_canary
branch
from
81849d0
to
988189c
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clandry94, 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 |
k8s-ci-robot
added
the
approved
|
I've retried CI several times already maybe rebase with master and see if it helps since #3488 is merged. |
What this PR does / why we need it:
This adds more tests for the merge function when creating alternative backends. It also covers the scenarios where an alternative ingress merges with itself, which was fixed in #3417
I also fixed a few edge cases I found while adding the new tests.
Special notes for your reviewer: