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

Provide support for conditional dependencies #1082

Merged
merged 5 commits into from
Dec 2, 2024
Merged

Conversation

flixman
Copy link
Contributor

@flixman flixman commented Dec 1, 2024

Currently all services dependencies are treated unconditionally. However, docker-compose supports specifying three possible conditions (started, healthy, successfully finished). This PR provides such support, also for all podman conditions, extending the existing unit tests for dependencies.

Additionally: statements to retrieve values with defaults from dictionaries have been cleaned up.

Fixes #866

@flixman
Copy link
Contributor Author

flixman commented Dec 1, 2024

@p12tic I hope you'll find this PR more according to the project standards :-)

@p12tic
Copy link
Collaborator

p12tic commented Dec 2, 2024

I hope you'll find this PR more according to the project standards :-)

Thanks, way easier to review.

@p12tic
Copy link
Collaborator

p12tic commented Dec 2, 2024

There's a formatting check failing, but I think you've done enough bending to the project requirements, I will finish the rest :-)

if not start_point:
start_point = service_name

start_point = start_point or service_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think previous style was more obvious to understand.

@@ -2546,7 +2648,8 @@ async def compose_up(compose: PodmanCompose, args):
podman_args = await container_to_args(compose, cnt, detached=args.detach)
subproc = await compose.podman.run([], podman_command, podman_args)
if podman_command == "run" and subproc is not None:
await compose.podman.run([], "start", [cnt["name"]])
# run the container
Copy link
Collaborator

Choose a reason for hiding this comment

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

The next line says the same in the function name, no need for the comment.

Signed-off-by: Felix Rubio <felix@kngnt.org>
Modified-by: Povilas Kanapickas <povilas@radix.lt>
Signed-off-by: Felix Rubio <felix@kngnt.org>
Modified-by: Povilas Kanapickas <povilas@radix.lt>
Signed-off-by: Felix Rubio <felix@kngnt.org>
Modified-by: Povilas Kanapickas <povilas@radix.lt>
Signed-off-by: Felix Rubio <felix@kngnt.org>
Signed-off-by: Felix Rubio <felix@kngnt.org>
@p12tic
Copy link
Collaborator

p12tic commented Dec 2, 2024

There were a couple of pylint errors, which was more time consuming to fix. Now it's all good, thanks for your contribution.

By the way, I also split the commits more, partly so that I could review them better, partly for mentioned bisecting benefits. The only more complicated commits now are 3ba0396 and a67fa0b. You can see how easier it's to review them when they contain only minimal changes needed. The rest of commits can be reviewed with much less focus.

@p12tic p12tic merged commit d9fc8e9 into containers:main Dec 2, 2024
7 checks passed
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.

Fails to handle the service_healthy condition of a depends_on element
2 participants