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

fix: missing tty options in StartExecOptions #339

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

leon3s
Copy link
Contributor

@leon3s leon3s commented Sep 29, 2023

@fussybeaver
Copy link
Owner

Thanks, I think we never added this parameter, because in my testing it didn't seem to affect the API output in any way - you'd need to wrap this endpoint in a pseudo tty library and a command-line interface to actually make it act like a tty.

Now I'm checking the underlying moby code, it looks like the tty parameter sets a different response content type? https://github.com/moby/moby/blob/fa2f6f98beb4d9d6fd814e63c28cefac7f90ccbc/api/server/router/container/exec.go#L122

Are you finding this parameter useful?

@leon3s
Copy link
Contributor Author

leon3s commented Oct 2, 2023

Hey yes, instead of returning a Stream where docker write for each stdout and stderr the number 0 or 1 to specify what kind of output is. It write all the output directly. It also add colors and detach keys as the behavior differt when the command write in a tty.
You can test the behavior of this using docker cli.

docker run -dt --name test-exec alpine:latest
# No colors
docker exec test-exec ls -l --color=auto /
# With colors
docker exec -t test-exec ls -l --color=auto /

Also any reason to not use this structure ? because it's seems unused

pub struct ExecStartConfig {

@fussybeaver
Copy link
Owner

fussybeaver commented Oct 2, 2023

I guess the same could be asked about other options like

pub struct ContainerConfig {
and there are probably others..

It's probably a good idea to use the generated type in these cases 👍

Edit: I think it's because of the use of T: Into<String>... something we should probably remove in the long run

@fussybeaver
Copy link
Owner

I'll merge this for now, can think about using the generated types

@fussybeaver fussybeaver merged commit 38f5d4c into fussybeaver:master Oct 3, 2023
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.

2 participants