integration: test LiveUpdate multiple images one manifest [ch3115]#2014
Conversation
|
|
||
| ctx, cancel = context.WithTimeout(f.ctx, time.Minute) | ||
| defer cancel() | ||
| fourthPods := f.WaitForAllPodsReady(ctx, "app=twoimages") |
There was a problem hiding this comment.
should probably do this after the Curl call, like you did on line 36. right now this will probably execute before the live-update even has a change to run
| FROM golang:1.11-alpine | ||
| WORKDIR /go/src/github.com/windmilleng/integration/live_update_two_images_one_manifest/sparkle | ||
| ADD sparkle . | ||
| ADD start.sh . |
There was a problem hiding this comment.
fwiw, i noticed that Chuck's Dockerfiles loaded this directly from github
ADD https://raw.githubusercontent.com/windmilleng/rerun-process-wrapper/master/start.sh .
and that might be a pattern using
There was a problem hiding this comment.
hmm interesting. do we think that will speed up run time of our integ. tests?
There was a problem hiding this comment.
no, just reduces the amount of copying to do
|
@nicks added a lil more functionality (now testing crash rebuild as well) if you wanna take a look |
|
actually, hold off reviewing, I discovered a bug: we report that we've operated on all containers that we know about, even if we only operated on a subset of them (cuz only a subset of the images had changed files) |
b705e75 to
b879955
Compare
|
bump - would love re-review, i added some stuff |
| } | ||
|
|
||
| func (f *k8sFixture) WaitForAllPodsInPhase(ctx context.Context, selector string, phase v1.PodPhase) []string { | ||
| func (f *k8sFixture) WaitForOnePodWithAllContainersReady(ctx context.Context, selector string, timeout time.Duration) string { |
There was a problem hiding this comment.
This new n parameter adds a bunch of complexity, but I couldn't find any justification for it. What's wrong with waiting on all pods? Can you leave some comments on what this is for?
There was a problem hiding this comment.
it was to address a blip i was seeing locally (where I'd get multiple ready pods back, presumably from catching the crash rebuild at exactly the wrong time) -- but I can leave this out unless other people start seeing that blip/it starts happening on CI
| msg = "Timed out waiting for all pods to be ready." | ||
| } | ||
|
|
||
| for { |
There was a problem hiding this comment.
could this use the WaitUntil helpers?
| ) | ||
|
|
||
| const ( | ||
| sparkleUrl = "http://localhost:8100" |
There was a problem hiding this comment.
can you move these constants into the function? otherwise we'll run into issues where other functions use them accidentally
|
|
||
| const ( | ||
| sparkleUrl = "http://localhost:8100" | ||
| tadaUrl = "http://localhost:8101" |
There was a problem hiding this comment.
go style is tadaURL, not tadaUrl
https://github.com/golang/go/wiki/CodeReviewComments#initialisms
There was a problem hiding this comment.
Whoops, you're right!
…y machine)" This reverts commit cdff681.
|
Gonna run tests again to see if they pass again or are flakey 🤞 |
2 participants
Hello @,
Please review the following commits I made in branch maiamcc/integ-test-two-images-one-manifest:
3886567 (2019-08-08 11:24:36 -0400)
integration: test LiveUpdate multiple images one manifest [ch3115]
Code review reminders, by giving a LGTM you attest that: