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

Do not try to guess when to allocate a TTY and keep it as default #9035

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

ulyssessouza
Copy link
Contributor

What I did
Do not try to guess when to allocate a TTY and keep it as default

Related issue
Resolves #8908

@ulyssessouza ulyssessouza force-pushed the fix-out-with-redirection branch 4 times, most recently from eb418ff to 5321188 Compare December 14, 2021 18:52
@ulyssessouza ulyssessouza marked this pull request as ready for review December 14, 2021 19:02
@ndeloof
Copy link
Contributor

ndeloof commented Dec 15, 2021

I can't find a better fix for this, but still sad we have to give up with tty detection, and wonder this will trigger more issues from users who expect compose to automatically adjust to environment :-/

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
@ulyssessouza
Copy link
Contributor Author

@ndeloof Yep. I also tried to save the automatic behaviour, but without success...
But at least the possible issues will have just a "-T" as "fix"

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

SGTM

just to double-check; this brings it to the same behaviour / defaults as compose v1, correct?

@rfay
Copy link
Contributor

rfay commented Mar 8, 2022

Since panic is a result of this I don't think it's probably satisfactory, see https://stackoverflow.com/questions/70855915/fix-panic-provided-file-is-not-a-console-from-docker-compose-in-github-action

@ndeloof
Copy link
Contributor

ndeloof commented Mar 8, 2022

I don't like this either. I expect we get rid of this as we share more code with docker/cli, especially relying on the exact same RunExec function

@rfay
Copy link
Contributor

rfay commented Mar 8, 2022

Right... but not preventing a panic? that doesn't seem like the right thing...

@ndeloof
Copy link
Contributor

ndeloof commented Mar 8, 2022

We can burn some cycles trying to find a better way to manage this and avoid a panic, which is obviously the worst possible user experience to offer :'(
but with #9198 and docker/cli#3436 we are close to just drop all the custom exec code implemented by docker/compose to adopt the battle-tested RunExec code used by the docker CLI for years.

@rfay
Copy link
Contributor

rfay commented Mar 8, 2022

Sounds good. I had actually planned long ago and half-implemented a switch to using docker's exec for ddev exec instead of using docker-compose exec but didn't actually pull the trigger, just worried about unexpected results.

@rostislav-simonik-plc
Copy link

Hello, i just tested with version 2.6.1 of docker compose and still having the issue. Once i remove exec with tee it starts working.

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.

docker compose run hangs indefinitely if stdout/err were duplicated to a file
5 participants