-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use containers we expect to start for wait condition #10209
Conversation
0834430
to
d9c5a2f
Compare
e44520e
to
7778b20
Compare
Hello @ndeloof. I've run some tests on your branch and I've found some possible issues that I wanted to let you know. If I should instead report this as part of the original issue ticket, please let me know, okay? Test 1version: "2.4"
services:
# Long-lived container.
hello1:
image: "alpine"
command: ["sh", "-c", "echo 'Long-lived container, no health check'; sleep 1h;"]
# Short-lived container.
hello2:
image: "alpine"
command: ["echo", "Short-lived container, no health check"] Execution:
Hitting ^C did not stop docker-compose which had to be killed. Test 2version: "2.4"
services:
# Short-lived container.
hello:
image: "alpine"
command: ["echo", "Short-lived container, no health check"] Execution:
Notice in this case the Test 3# Run multiple short-lived containers.
version: "2.4"
services:
hello1:
image: "alpine"
command: ["echo", "Short-lived container (1), no health check"]
hello2:
image: "alpine"
command: ["echo", "Short-lived container (2), no health check"]
hello3:
image: "alpine"
command: ["echo", "Short-lived container (3), no health check"] Execution:
Like with Test 1, hitting ^C did not stop docker-compose which had to be killed. |
@rborn-tx thanks, nice feedback. I'll try to convert those examples into e2e tests so we have better coverage for this feature |
7778b20
to
8912c5f
Compare
I fixed reported corner cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but we may have a potential side effect in the dependencies check loop
pkg/compose/convergence.go
Outdated
if err != nil { | ||
return err | ||
} | ||
containers = containers.filter(isService(dep)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use a dedicated variable to manage filtered containers? I wondering if we won't loose potential dependencies by re-assigning containers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, nice catch - I've been lazy here
8912c5f
to
3a89d8f
Compare
3a89d8f
to
b8b02d6
Compare
Codecov ReportBase: 73.89% // Head: 73.89% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## v2 #10209 +/- ##
=======================================
Coverage 73.89% 73.89%
=======================================
Files 2 2
Lines 272 272
=======================================
Hits 201 201
Misses 60 60
Partials 11 11 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Nicolas De Loof <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
b8b02d6
to
52478f0
Compare
I disagree. I have multiple services in my docker-compose.yml, that I do want compose to wait for. At the same time I have a service that does its job and exits with 0 code. This change broke my setup and I had to add artificial sleep at the end of the script. I think In other words: Test 1 should pass, but it now errors out with |
@kaos14 we miss an explicit way to define "jobs" or "tasks", i.e. container that are EXPECTED to complete |
@ndeloof What did you mean "I fixed reported corner cases."? None of them works in 2.16. Is this as expected? |
as we check dependency service is running/healthy, use containers we created for the service so that we can detect failure to start, and not wait with no end for a dead service to become healthy.
Related issue
closes #10200
(not mandatory) A picture of a cute animal, if possible in relation to what you did