Skip to content

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 17 commits behind head on master.

❌ Your patch status has failed because the patch coverage (20.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

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 a team and thaJeztah 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.

}

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?

@vvoland
Copy link
Collaborator

vvoland commented Feb 24, 2025

Yes, we will be having a patch release this week (Wednesday is the initial plan).

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