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

Fixes #398: exclude deps on up if --no-deps #1108

Merged
merged 2 commits into from
Jan 15, 2025
Merged

Conversation

bailsman
Copy link
Contributor

Fixes #398 by excluding dependent services if --no-deps is given on the command line.

@p12tic
Copy link
Collaborator

p12tic commented Jan 14, 2025

Thanks for the PR. It looks good.

Please a release note to the newsfragments directory (you can look here for inspiration on how release note looks like).

Also the PR needs an integration test. In this particular case I think tests/integration/test_podman_compose_deps.py is most relevant for enhancement. Note that it uses compose files from tests/integration/deps.

@bailsman bailsman force-pushed the fix-398 branch 4 times, most recently from 088ff46 to baf2e48 Compare January 15, 2025 07:53
@bailsman
Copy link
Contributor Author

bailsman commented Jan 15, 2025

Thanks for the hints! The integration test exposed that it is a bit more involved than I thought initially (my local setup didn't trigger these).

  • Run (through container_to_args) adds --requires parameters to the podman run command line, which does not work if the dependency is not created/started. Worked around this by adding extra boolean parameter to container_to_args, as is already done for detach.
  • Up tries to check if dependencies have successfully started, worked around this by passing in an empty deps set if no-deps is given.

if podman_command == "run" and subproc is not None:
await run_container(compose, cnt["name"], cnt["_deps"], ([], "start", [cnt["name"]]))
await run_container(compose, cnt["name"], deps, ([], "start", [cnt["name"]]))
Copy link
Collaborator

@p12tic p12tic Jan 15, 2025

Choose a reason for hiding this comment

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

Add

def deps_from_container(args, cnt):
    if args.no_deps:
        return set()
    return cnt['deps']

and call it here and in the call site below.

@p12tic
Copy link
Collaborator

p12tic commented Jan 15, 2025

The integration test exposed that it is a bit more involved than I thought initially

Great to see tests had good impact :)

The PR looks good overall, I had one nit comment.

Another nit comment, could you move Fixes #issue number from commit title to commit description? This is the style used throughout recent history.

Thanks a lot!

@p12tic p12tic merged commit a177603 into containers:main Jan 15, 2025
8 checks passed
@bailsman bailsman deleted the fix-398 branch January 15, 2025 21:57
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.

option "--no-deps" is ignored for "podman-compose up"
2 participants