Return non-zero exit_code on failure when doing up -d
#1181
Conversation
|
The unitest failed, but the Related pr: #964 |
podman_compose.py
Outdated
| @@ -772,6 +772,27 @@ def container_to_cpu_res_args(cnt, podman_args): | |||
| str(mem_res).lower(), | |||
| )) | |||
|
|
|||
| # Handle pids limit from both container level and deploy section | |||
| pids_limit = cnt.get("pids_limit") | |||
| deploy_pids = None | |||
There was a problem hiding this comment.
limits.get("pids") would be equivalent, no?
podman_compose.py
Outdated
| f"deploy.resources.limits.pids ({deploy_pids}) must be the same" | ||
| ) | ||
|
|
||
| # Use whichever value is set (prioritizing pids_limit if both are set) |
There was a problem hiding this comment.
(prioritizing pids_limit if both are set) is invalid comment, because due to above exception this situation can't happen.
podman_compose.py
Outdated
| 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) |
podman_compose.py
Outdated
| @@ -2802,12 +2807,21 @@ async def compose_up(compose: PodmanCompose, args): | |||
| compose, cnt, detached=args.detach, no_deps=args.no_deps | |||
| ) | |||
| subproc = await compose.podman.run([], podman_command, podman_args) | |||
| if subproc is not None and subproc != 0: | |||
There was a problem hiding this comment.
Let's name subproc_exit_code
podman_compose.py
Outdated
| if podman_command == "run" and subproc is not None: | ||
| await run_container( | ||
| container_result = await run_container( |
There was a problem hiding this comment.
Let's name container_exit_code
podman_compose.py
Outdated
| log.error("Build command failed") | ||
| if not args.dry_run: | ||
| return build_result |
There was a problem hiding this comment.
This return needs a separate release note, because it fixes unrelated issue that podman-compose would continue even if build step fails.
There was a problem hiding this comment.
Looks good overall.
Please add release notes to the newsfragments directory (you can look here for inspiration on how release note looks like).
|
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 |
zeyugao
changed the title
up -d, implement pids_limitup -d
|
Tests are currently failing, do they fail for you locally? |
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 |
|
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. |
Format Rename variables related to exit_code Move variable location Workaround for running test rootfully format Signed-off-by: Elsa <[email protected]>
|
I have changed the expected exit code accordingly |
Signed-off-by: Elsa <[email protected]>
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