Skip to content
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

Support condition: service_healthy #72

Closed
wants to merge 3 commits into from
Closed

Support condition: service_healthy #72

wants to merge 3 commits into from

Conversation

EricHripko
Copy link
Collaborator

What this PR does / why we need it:
This PR imports condition: service_healthy functionality from Compose v2. The problem at hand was described and is being discussed in #68. Both the definition and the example were lifted from Compose v2 docs.

Which issue(s) this PR fixes:
Fixes #68.

Signed-off-by: Eric Hripko <ehripko@bloomberg.net>
Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but the description of depends_on should clarify both a simple "short notation" list and nested structures "long notation" are valid, so that it can introduce condition

This PR is missing update on the json schema. See https://github.com/docker/compose/blob/master/compose/config/config_schema_v2.4.json#L122-L141

Signed-off-by: Eric Hripko <ehripko@bloomberg.net>
@EricHripko
Copy link
Collaborator Author

@ndeloof I've addressed your feedback 🙂

@EricHripko
Copy link
Collaborator Author

Hey @ndeloof! Thank you for approving my PR - really appreciate it 🙌 Is there anything else I could do to get this landed?

@ndeloof
Copy link
Collaborator

ndeloof commented May 14, 2020

rebased and merged

@ndeloof ndeloof closed this May 14, 2020
@EricHripko EricHripko deleted the condition-healthy branch May 14, 2020 17:04
@EricHripko
Copy link
Collaborator Author

Thank you for merging this 🙌 😄

@ulgens
Copy link

ulgens commented Jun 25, 2022

@ndeloof @EricHripko What happened here? The branch seems not merged and deleted but the commits are in the main branch. Did someone apply an external patch instead of merging the PR?

@ulgens
Copy link

ulgens commented Jun 25, 2022

Also, is this ever implemented in docker-compose?

@ndeloof
Copy link
Collaborator

ndeloof commented Jun 25, 2022

we don't use merge commits but rebase, so the commit to appear on master without an explicit merge commit

@ulgens
Copy link

ulgens commented Jun 25, 2022

we don't use merge commits but rebase

Rebase merge changes PR status to "merged", not "closed". In the repo history there are multiple PRs whose latest status is "merged" but this one in particular shows as "closed", that's where I am lost.

@EricHripko
Copy link
Collaborator Author

Also, is this ever implemented in docker-compose?

Yes, this has been supported by Docker Compose implementation since version 1.27.0 (although V2 potentially might have broken compatibility in places).

@ulgens
Copy link

ulgens commented Jul 24, 2022

@EricHripko Thank you. Is this documented in the changelog? I read it three times but couldn't see anything related to condition keys.

@EricHripko
Copy link
Collaborator Author

EricHripko commented Sep 3, 2022

It's under the first point - Merge 2.x and 3.x compose formats and align with COMPOSE_SPEC schema. There's a bunch of other V2 functionality that became mainstream again with the arrival of Compose spec, which is why condition: service_healthy isn't called out explicitly. I appreciate it's not as explicit as it perhaps could be 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support condition: service_healthy from v2
3 participants