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

dockercompose: add wait_for_healthy optional argument to docker_compose (#2239) #6329

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

kinland
Copy link
Contributor

@kinland kinland commented Mar 5, 2024

This is a tentative solution for #2239

I imagine the ideal case would be to do something fancy in internal/dockercompose/state.go to check the status, but as far as I could tell, the library that is being used under the hood there doesn't support checking whether a container is healthy.

Simply adding --wait to the docker compose up command seems to make my health checks work properly. This might have side effects, but in the project that I've been submitting all these PRs for, it makes the race condition problems go away and I don't see any issues.

Tests seem to pass locally.

@@ -245,7 +245,7 @@ def docker_build(ref: str,
"""
pass

def docker_compose(configPaths: Union[str, Blob, List[Union[str, Blob]]], env_file: str = None, project_name: str = "", profiles: Union[str, List[str]] = []) -> None:
def docker_compose(configPaths: Union[str, Blob, List[Union[str, Blob]]], env_file: str = None, project_name: str = "", profiles: Union[str, List[str]] = [], wait_for_healthy = False) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of changing the param to wait to match the flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial choice, actually. I was concerned in the context of Tilt it might not be obvious, but I'll chalk that up to "coding in the middle of the night," as it literally says docker_compose right there 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the delay - this is done now

Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

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

LGTM!

@nicks nicks merged commit 71299e1 into tilt-dev:master Mar 13, 2024
9 checks passed
@christian-michallek
Copy link

Great! i was waiting for this to work.
does this mean, that the container is now ready, when the healthcheck succeeds?
Just wanna check, because the docs still says otherwise.
https://docs.tilt.dev/resource_dependencies

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.

3 participants