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

Use docker’s stdcopy to ensure we don’t emit garbage bytes to stdout #57

Merged
merged 1 commit into from
May 23, 2019

Conversation

aidansteele
Copy link
Contributor

As per the ContainerAttach() docs, a container not using a TTY has stderr and stdout multiplexed into a single stream in a Docker-specific format.

When running act and its stdout is not a terminal (e.g. when it's redirected to a file), DockerExecutorInput.logDockerOutput() is invoked -- I've changed this to use github.com/docker/docker/pkg/stdcopy.StdCopy as per Docker's suggestion.

Right now the issue is the tests pass when output is redirected to a file and fail when run directly in a terminal! ❗️ This is because of the following two conflicting pieces:

isTerminal := terminal.IsTerminal(int(os.Stdout.Fd()))
config := &container.Config{
Image: input.Image,
Cmd: input.Cmd,
Entrypoint: input.Entrypoint,
WorkingDir: input.WorkingDir,
Env: input.Env,
Tty: isTerminal,

act/container/docker_run.go

Lines 170 to 171 in f2cb9e3

isTerminal := terminal.IsTerminal(int(os.Stdout.Fd()))
if !isTerminal || os.Getenv("NORAW") != "" {

Honestly, I'm not sure what the purpose of the NORAW env var is. If NORAW is passed in, the tests will fail in even more spectacular ways. I would like to do the following, but wanted to run it by you first before submitting this PR:

  • Instead of terminal.IsTerminal() in attachContainer(), we use the Docker API client to introspect the container and see if it is in TTY mode or not.
  • Additionally, add Tty *bool to NewDockerRunExecutorInput and only fall back to terminal.IsTerminal() if it is nil.

What are your thoughts?

@cplee cplee merged commit d85cabf into nektos:master May 23, 2019
@aidansteele aidansteele deleted the fix-nontty branch March 12, 2020 04:34
makrsmark pushed a commit to makrsmark/act that referenced this pull request Aug 3, 2023
The runner's `privileged` config can be bypassed. Currently, even if the runner's `privileged` config is false, users can still enable the privileged mode by using `--privileged` in the container's option string. Therefore, if runner's config is false, the `--privileged` in options string should be ignored.

Reviewed-on: https://gitea.com/gitea/act/pulls/57
Reviewed-by: Jason Song <i@wolfogre.com>
Co-authored-by: Zettat123 <zettat123@gmail.com>
Co-committed-by: Zettat123 <zettat123@gmail.com>
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