-
Notifications
You must be signed in to change notification settings - Fork 521
Fix service_healthy condition enforcing #1184
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
Conversation
ac97fdf
to
48add0e
Compare
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.
Overall looks good, thank you very much. I had several comments.
Please add release note to the newsfragments directory (you can look here for inspiration on how release note looks like). |
ddd4b8a
to
f72a18c
Compare
@p12tic Thanks for your insightful reviews. You comments have been addressed with the amended commit. Could you please take another look at your earliest convenience? Tons of thanks! |
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.
Amazing works, thanks!
Signed-off-by: Justin Zhang <[email protected]>
Signed-off-by: Justin Zhang <[email protected]>
The test did fail on my laptop with podman 5.4.1. Signed-off-by: Justin Zhang <[email protected]>
Signed-off-by: Povilas Kanapickas <[email protected]>
Skip dependency health check to avoid compose-up hang for podman prior to 4.6.0, which doesn't support --condition healthy. Signed-off-by: Justin Zhang <[email protected]>
Signed-off-by: Justin Zhang <[email protected]>
I split the PR into smaller commits so that I myself would understand better what part causes what change of behavior and for that to be encoded into git history. Authorship has been preserved. |
Thank you so much for the merge and the split! |
This PR fixes #1176, #1178, and #1183 by employing a create-and-start approach, where the containers are created in the
first pass, then they are started using the
run_container()
method to make sure the dependencies'conditions are checked. The second improvement is to add a version check to skip "podman wait
--condition=healthy" in the
check_dep_conditions()
function to prevent podman-compose hang. BTW,this PR also fixes a minor problem that podman-compose attempts to stop and remove the containers
defined in the compose file when the
--force-recreate
option is specified when there are norunning containers at all.
Specific changes are as follows:
4.6.0 seems to be the first version to support --condition=healthy, as discovered by this script:
Finally, there are a few integration test cases mentioning this bug:
podman-compose/tests/integration/env-file-tests/test_podman_compose_env_file.py
Line 129 in 342a39d
podman-compose/tests/integration/ulimit/test_podman_compose_ulimit.py
Line 37 in 342a39d