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

Handle SIGINT when running "up" command to shutdown gracefully #1148

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

mokazemi
Copy link
Contributor

This pull request enhances the podman compose behavior by handling the Ctrl+C (SIGINT) signal more gracefully. When Ctrl+C is pressed during the execution of podman compose up, it runs the down command to make sure containers shut down gracefully.

Solves: #51, #112, #411, #436, #457, #543, #676

@p12tic
Copy link
Collaborator

p12tic commented Feb 28, 2025

Thanks for the contribution. The PR looks good in principle, I'm just not confident enough with the down_args = argparse.Namespace(**dict(args.__dict__, volumes=False)) line yet.

Separately, pylint is complaining in https://github.com/containers/podman-compose/actions/runs/13497452404/job/37707798339?pr=1148. I think both warnings can be disabled in .pylintrc.

@mokazemi
Copy link
Contributor Author

mokazemi commented Mar 1, 2025

Thanks for the contribution. The PR looks good in principle, I'm just not confident enough with the down_args = argparse.Namespace(**dict(args.__dict__, volumes=False)) line yet.

About the line that you mentioned (down arguments) I actually used the same line already used somewhere else in the code:

down_args = argparse.Namespace(**dict(args.__dict__, volumes=False))

I guess it should work enough, but we can modify in case of improvements.

Separately, pylint is complaining in https://github.com/containers/podman-compose/actions/runs/13497452404/job/37707798339?pr=1148. I think both warnings can be disabled in .pylintrc.

Oh I didn't check for warning here. I changed the code about the second one so that it passes now, but for the first one, I added it to .pylintrc since IMO it could be ignored :)).

@pablo-tx
Copy link

@p12tic is there anything else blocking this merge?

@p12tic
Copy link
Collaborator

p12tic commented Mar 19, 2025

@pablo-tx @mokazemi Sorry for taking a long time to reply.

Code changes look good. Please rebase on top of latest main branch and fix merge conflicts. Also please add a release note to the newsfragments directory (you can look here for inspiration on how release note looks like). Also DCO test fails - it asks for Signed-off-by: Author Name <[email protected]> line in commit message.

@mokazemi
Copy link
Contributor Author

Done. @p12tic

@p12tic p12tic merged commit 0cf1378 into containers:main Mar 19, 2025
8 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.

3 participants