A few changes to federated-service e2e test. #27972
Merged
+96
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mml
added
priority/critical-urgent
|
The DNS changes here are just code readability changes and no functional change, right @mml ? |
k8s-github-robot
added
the
size/L
|
and debug logs, but no functionality. |
| Expect(clSvc.Name).To(Equal(service.Name)) | ||
| // Some fields are expected to be different, so make them the same before checking equality. | ||
| clSvc.Spec.ClusterIP = service.Spec.ClusterIP | ||
| clSvc.Spec.ExternalIPs = service.Spec.ExternalIPs | ||
| clSvc.Spec.DeprecatedPublicIPs = service.Spec.DeprecatedPublicIPs | ||
| clSvc.Spec.LoadBalancerIP = service.Spec.LoadBalancerIP | ||
| clSvc.Spec.LoadBalancerSourceRanges = service.Spec.LoadBalancerSourceRanges | ||
| for i := range clSvc.Spec.Ports { |
There was a problem hiding this comment.
minor nit: Prefer "for i, port := ....". Then you can just use "port.NodePort below.
There was a problem hiding this comment.
That actually doesn't work, as port is a value type and ends up being a copy of the object and your changes get lost.
I'll add a comment explaining this, as I banged my head against it for quite awhile.
|
Thanks @mml. Only nits, which can be addressed in a followup PR if you prefer. |
ghost
added
the
lgtm
|
PS: Feel free to remove the LGTM label to avoid auto-merge if you prefer to address the nits in this PR. |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
lgtm
ghost
added
the
lgtm
k8s-github-robot
added
the
needs-rebase
mml
force-pushed
the
fed-backend2
branch
3 times, most recently
from
bb0fafa to
c33d929
Compare
|
I managed to drop some changes during my git antics, but I think it is in good shape now. |
| if _, ok := kd.federations[path[2]]; ok { | ||
| return true | ||
| if len(path) != 4+len(kd.domainPath) { | ||
| glog.Infof("len(%q) != 4+len(%q)", path, kd.domainPath) |
There was a problem hiding this comment.
It might be better to move these glog.Infos to glog.V(2).Info.
mml
removed
the
needs-rebase
k8s-github-robot
removed
the
lgtm
mml
added
the
lgtm
Most of the changes that get the test to pass have been made already or elsewhere. Here we restructure a bit fixing a nesting problem, extend the timeouts, and start creating distinct backend pods that I'll delete in the non-local test (coming shortly). Also some extra debugging info in the DNS code. I made some upstream changes to skydns in skynetservices/skydns#283
k8s-github-robot
removed
the
lgtm
ghost
added
the
lgtm
|
"Jenkins GKE Smoke Test" is something I've not seen before. It seems to be borked. @lavalamp ? @k8s-bot test this issue: #IGNORE |
ghost
mentioned this pull request
|
GCE e2e build/test passed for commit e2021ef. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit e2021ef. |
|
Automatic merge from submit-queue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most of the changes that get the test to pass have been made already or
elsewhere. Here we restructure a bit fixing a nesting problem, extend the
timeouts, and start creating distinct backend pods that I'll delete in the
non-local test (coming shortly).
Also some extra debugging info in the DNS code. I made some upstream
changes to skydns in skynetservices/skydns#283
For #27739
Includes a commit from @madhusudancs that I will remove once his merges.