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

Don't raise exception on inexistent services in 'down' command #1117

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rjeffman
Copy link
Contributor

When running 'podman-compose down ', if service is not part of the compose, a KeyError exception was raised it function 'get_excluded'.

By only allowing evaluation of services that exist in the compose provides a cleaner and gentler exit for this case.

When running 'podman-compose down <service>', if service is not part of
the compose, a KeyError exception was raised it function 'get_excluded'.

By only allowing evaluation of services that exist in the compose
provides a cleaner and gentler exit for this case.

Signed-off-by: Rafael Guterres Jeffman <[email protected]>
@rjeffman rjeffman force-pushed the no_exception_if_service_is_not_in_compose branch from 4e7a973 to cc9faed Compare January 23, 2025 02:56
@rjeffman rjeffman marked this pull request as draft January 23, 2025 15:54
@@ -2606,7 +2606,7 @@ def get_excluded(compose, args):
if args.services:
excluded = set(compose.services)
for service in args.services:
if not args.no_deps:
if service in compose.services and not getattr(args, "no_deps", False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to change args.no_deps to use getattr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the reasons I kept this PR in "work in progress", there were some cases (which I don't know if are valid or not) that args.no_deps was undefined.

I'm sorry, but I've been drowning in work lately. I plan to get back to podman-compose and try to help with its development, but it will still take a few weeks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see - compose_up_parse configures no-deps argument, but compose_down_parse doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it makes sense to add a comment saying that compose_down_parse does not configure no-deps command argument. This would make it clear why the code is how it is.

@p12tic
Copy link
Collaborator

p12tic commented Jan 30, 2025

Looks good. Please also add:

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.

2 participants