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

By default, ExecShell will add an io.Discard #20

Merged
merged 2 commits into from
Jun 30, 2020

Conversation

beholders-eye
Copy link
Contributor

CombinedOutput will be io.Discard except when someone passed
IgnoreResults. Seems like a good way of throwing "async" command calls
that doesn't need to be checked after execution.

CombinedOutput will be `io.Discard` except when someone passed
IgnoreResults. Seems like a good way of throwing "async" command calls
that doesn't need to be checked after execution.
@beholders-eye beholders-eye changed the title By default, ExecShell will add and io.Discard By default, ExecShell will add an io.Discard Jun 29, 2020
@zombiezen
Copy link
Contributor

I dug into the fsouza/go-dockerclient source and discovered that the underlying issue is that without attaching stdout/stderr handles, there's a synchronization bug. I've reported upstream as fsouza/go-dockerclient#838. You can see for yourself by adding the following to the end of ExecShell:

	if results.Running {
		return fmt.Errorf("execute shell command in container %s: yup it's a sync problem", containerID)
	}

My recommendation is that this shouldn't be a separate option. If the caller of ExecShell passes CombinedOutput: nil, it should pass ioutil.Discard to StartExec. We're effectively patching around the problem at a higher level.

No "IgnoreResults" option, too confusing.
We only use `ioutil.Discard` when starting the exection

Tests don't check `err`, as asked in #17

[ch1592]
@beholders-eye
Copy link
Contributor Author

PTAL @zombiezen

Copy link
Contributor

@jamesnaftel jamesnaftel left a comment

Choose a reason for hiding this comment

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

LGTM based on Ross's suggestion.

@beholders-eye beholders-eye merged commit 9b945bc into master Jun 30, 2020
@beholders-eye beholders-eye deleted the beholders_eye/feature/auto-add-combinedoutput branch June 30, 2020 14:35
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.

3 participants