Increase compatibility for tty/no-tty options#1418
Conversation
|
When I run the integration tests from the |
|
For the reviewers:
|
p12tic
left a comment
There was a problem hiding this comment.
Overall looks good. I would have expected 4 commits for easier review and easier bisection if the PR breaks something.
What are the errors? You should be able to run all tests successfully on a Debian trixie container. See https://github.com/containers/podman-compose/blob/main/.github/workflows/test.yml for how it's setup -- you'll need |
Thanks! Adding These are the failing tests: These failing tests seem unrelated, however, they pass fine on main. So I'll have to dig a bit deeper... |
These are line ending issues. Seems related to the fact that you're running on MacOS. I would ignore these tests for now. While fixing them would allow to run tests on MacOS in CI, it does not make sense for me to ask this on your unrelated PR. |
|
@p12tic from reading the Docker source code, I now believe that docker compose up simply forward the --no-tty/--tty option to docker even when detach is used. I made a few more changes, so that podman-compose no longer passes Could you perhaps check what is going on in function Another thing I do not understand: in if args.tty and not args.detach:
podman_args.insert(1, "-i")
if args.rm:
podman_args.insert(1, "--rm")should either be changed to (podman allows --rm and detach): <---- chose solution in 6b272b4 if args.tty and not args.detach:
podman_args.insert(1, "-i")
if args.rm:
podman_args.insert(1, "--rm")or (podman does not allow --rm and detach): if args.tty and not args.detach:
podman_args.insert(1, "-i")
if args.rm and not args.detach:
podman_args.insert(1, "--rm")WDYT? |
The branch from this PR shows that these tests fail the same way on the github runner. 🤔 I don't understand. Both mac and unix use LF endings, Besides, I am also running the tests in a linux container. Could it be that how you clone the repository affects what newline character is used? |
|
@p12tic This PR is ready for merging. |
cab9ef9 to
70d5af3
Compare
|
Re. line endings: I can only conclude that the failing test are written to be run under windows.
Answering myself: no, a fresh git clone from within a container running Ubuntu gives the same results as running the tests against a git clone taken from the Mac host. |
|
@p12tic Is there anything you need so that this can get merged? |
Increase compatibility with docker compose for exec and run commands: - Add support for long option `--no-tty` besides `-T`. - Add support for hidden options `-t` and `--tty`. - Make default for tty auto-detected. - For the `exec` command: run `podman exec` without `--interactive` when no-tty is specified (the `run` command already does this). - For `run`, fix adding --rm option when detached. The default for terminal/no-terminal is based on terminal detection, mimicking docker compose code for [run](https://github.com/docker/compose/blob/3371227794f5f3645f4f19829c60a741635ed329/cmd/compose/run.go#L178) and [exec](https://github.com/docker/compose/blob/3371227794f5f3645f4f19829c60a741635ed329/cmd/compose/exec.go#L72). Hidden options `-t` and `--tty` are mimicking docker compose code for [run](https://github.com/docker/compose/blob/3371227794f5f3645f4f19829c60a741635ed329/cmd/compose/run.go#L195-L196) and [exec](https://github.com/docker/compose/blob/3371227794f5f3645f4f19829c60a741635ed329/cmd/compose/exec.go#L77-L78). NOTE: some of the added integration tests require the `script` command to emulate a TTY. This command is available in most unix environments, but might not be present in a minimal docker images. When `script` is not available, these tests pass without doing anything. Fixes containers#900. Signed-off-by: Erik van Oosten <e.vanoosten@grons.nl>
70d5af3 to
70a2f72
Compare
Increase compatibility with docker compose for
execandruncommands for the tty options:--no-ttybesides-T.-tand--tty.podmanwithout-i(interactive).runcommand, do not add-iwhen detach is used, even when a tty is specified/detected.runcommand, forward the--rmoption, even when detach is used.The default for terminal/no-terminal is based on terminal detection, mimicking docker compose code for run and exec.
Hidden options
-tand--ttyare mimicking docker compose code for run and exec.The added integration tests require the
scriptcommand to emulate a TTY. This command is available in most unix environments, but might not be present in a minimal docker image. Whenscriptis not installed the test will be skipped.Fixes #900.