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

cmd/docker: do not print error status on exec/run #5854

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

Benehiko
Copy link
Member

@Benehiko Benehiko commented Feb 21, 2025

- What I did
Do not print to std.Err when using docker exec.

- How I did it
Be explicit on printing the Cause/Status from StatusError instead of a fallback to a generic error message.

- How to verify it

- Human readable description for the release notes

Fix unintentionally printing exit status to stderr when `docker exec/run` returns a non-zero status

- A picture of a cute animal (not mandatory but encouraged)

@Benehiko Benehiko force-pushed the fix-exec-msg branch 4 times, most recently from 2ac3ab0 to bfe64df Compare February 21, 2025 10:46
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 59.27%. Comparing base (eb48cad) to head (0cff340).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5854      +/-   ##
==========================================
+ Coverage   58.89%   59.27%   +0.37%     
==========================================
  Files         350      353       +3     
  Lines       29682    29694      +12     
==========================================
+ Hits        17482    17601     +119     
+ Misses      11218    11113     -105     
+ Partials      982      980       -2     

@Benehiko Benehiko marked this pull request as ready for review February 21, 2025 11:09
@Benehiko Benehiko requested review from thaJeztah and a team February 21, 2025 11:09
@thaJeztah thaJeztah added this to the 28.0.1 milestone Feb 21, 2025
@Benehiko Benehiko self-assigned this Feb 21, 2025
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.

@@ -68,7 +68,6 @@ func TestRunAttach(t *testing.T) {
}

assert.Equal(t, c.ProcessState.ExitCode(), 7)
assert.Check(t, is.Contains(d.String(), "exit status 7"))
Copy link
Member

Choose a reason for hiding this comment

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

I guess if we want to be strict in our tests, we could consider checking for the error message to be empty (with a comment);

assert.Check(t, is.Equal(err.Error(), ""), "should not print our own error message, as the container's output is the error message")

Not urgent for how, but something we could consider

Copy link
Member

Choose a reason for hiding this comment

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

Derp; meant checking the d.String() for what we expect it to output (for that we could add a echo "something failed" to the container's command.

(but again, probably fine to look at in a follow up)

Copy link
Collaborator

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland
Copy link
Collaborator

vvoland commented Feb 21, 2025

Note: This also fixes run so the title/changelog should be adjusted

@Benehiko Benehiko changed the title cmd/docker: do not print error status on exec cmd/docker: do not print error status on exec/run Feb 21, 2025
@thaJeztah
Copy link
Member

Heh; probably best to update the commit message, not just the PR title 😂 😇

Screenshot 2025-02-21 at 12 54 28

Co-authored-by: Fabio Pugliese Ornellas <[email protected]>
Signed-off-by: Alano Terblanche <[email protected]>
@Benehiko
Copy link
Member Author

Heh; probably best to update the commit message, not just the PR title 😂 😇
Screenshot 2025-02-21 at 12 54 28

too many places to update 🙈
PR title, changelog, commit title

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.

LGTM

yeah, bit of a hassle always; sometimes help finding back changes in git history though!

@vvoland vvoland merged commit 77a8a8c into docker:master Feb 21, 2025
89 checks passed
@felixfontein
Copy link

Is it possible to get a 28.0.1 release soon with this fix included?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants