Skip to content

Return non-zero exit_code on failure when doing up -d #1181

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

Merged
merged 1 commit into from
May 10, 2025

Conversation

zeyugao
Copy link
Contributor

@zeyugao zeyugao commented Apr 8, 2025

When run podman-compose up -d, it returns directly without handling the error.

Related issues:

#806
#626

pids_limit spec:

https://github.com/compose-spec/compose-spec/blob/main/05-services.md#pids_limit and https://github.com/compose-spec/compose-spec/blob/main/deploy.md#pids

@zeyugao
Copy link
Contributor Author

zeyugao commented Apr 8, 2025

The unitest failed, but the up -d returns stderr: Error: keep-id is only supported in rootless mode. So it seems that the original unittest failed unexpectedly

Related pr: #964

if not args.no_build:
# `podman build` does not cache, so don't always build
build_args = argparse.Namespace(if_not_exists=(not args.build), **args.__dict__)
if await compose.commands["build"](compose, build_args) != 0:
build_result = await compose.commands["build"](compose, build_args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's name build_exit_code

Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Looks good overall.

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

@p12tic
Copy link
Collaborator

p12tic commented Apr 8, 2025

I think it's worth moving pids_limit change to separate PR, because it will take some time to understand why tests fail for your up -d changes. Also they will need an integration test, so splitting PRs will not delay the pids_limit part.

@zeyugao zeyugao mentioned this pull request Apr 9, 2025
@zeyugao
Copy link
Contributor Author

zeyugao commented Apr 9, 2025

#1182

@zeyugao zeyugao changed the title Handle up failure when doing up -d, implement pids_limit Return non-zero exit_code on failure when doing up -d Apr 9, 2025
@p12tic
Copy link
Collaborator

p12tic commented Apr 14, 2025

Tests are currently failing, do they fail for you locally?

@zeyugao
Copy link
Contributor Author

zeyugao commented Apr 14, 2025

image

I think that the failure reason is mentioned before. Locally I use rootless podman, but in the github action, podman is run as root. In this unit test expects using keep-id which is only supported in rootless mode as shown in the stderr

@p12tic
Copy link
Collaborator

p12tic commented Apr 21, 2025

I think that the failure reason is mentioned before. Locally I use rootless podman, but in the github action, podman is run as root. In this unit test expects using keep-id which is only supported in rootless mode as shown in the stderr

@zeyugao Sounds fair, thanks for explaining. Could we detect root by e.g. doing def is_root(): return os.geteuid() == 0 and then adjusting expected return code based on that? This would make integration test work in both rootful and rootless mode.

@zeyugao
Copy link
Contributor Author

zeyugao commented Apr 22, 2025

I have considered it as a solution, but this will make the CI on GitHub Actions unable to test the functionality correctly, making it less usefully. We have to test it locally.

@zeyugao zeyugao force-pushed the main branch 2 times, most recently from b588910 to 915994f Compare April 24, 2025 01:58
@zeyugao
Copy link
Contributor Author

zeyugao commented Apr 24, 2025

I have changed the expected exit code accordingly

Signed-off-by: Elsa <[email protected]>
Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@p12tic
Copy link
Collaborator

p12tic commented May 10, 2025

I rebased the PR and fixed merge conflict.

@p12tic p12tic merged commit cda84f4 into containers:main May 10, 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.

None yet

2 participants