The Wayback Machine - https://web.archive.org/web/20240814120611/https://github.com/docker/cli/pull/5302
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

[27.1 backport] attach: wait for exit code from ContainerWait #5302

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Jul 26, 2024

backport #5297

- Description for the changelog

Fix `docker attach` exiting on `SIGINT` instead of forwarding the signal to the container and waiting for it to exit.

@laurazard laurazard self-assigned this Jul 26, 2024
@vvoland vvoland added the kind/bugfix PR's that fix bugs label Jul 26, 2024
@vvoland vvoland added this to the 27.1.2 milestone Jul 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.50%. Comparing base (fd3157b) to head (1cf3637).

Additional details and impacted files
@@           Coverage Diff           @@
##             27.x    #5302   +/-   ##
=======================================
  Coverage   61.50%   61.50%           
=======================================
  Files         299      299           
  Lines       20866    20867    +1     
=======================================
+ Hits        12834    12835    +1     
  Misses       7116     7116           
  Partials      916      916           

@vvoland
Copy link
Collaborator

vvoland commented Jul 26, 2024

Failures seem related to: #5229 (comment)

It added the "exit status " message, but wasn't backported to the 27.0 branch

@laurazard
Copy link
Member Author

Ah, I guess it doesn't make since to backport #5229, so I'll adjust the expected output. WDYT @vvoland?

@vvoland
Copy link
Collaborator

vvoland commented Jul 26, 2024

I'm still unsure if we should have that message at all 😅 I think we still haven't decided if this is the expected behavior or not.
@thaJeztah WDYT?

@vvoland vvoland changed the title [27.0 backport] attach: wait for exit code from ContainerWait [27.x backport] attach: wait for exit code from ContainerWait Jul 26, 2024
@vvoland vvoland changed the title [27.x backport] attach: wait for exit code from ContainerWait [27.1 backport] attach: wait for exit code from ContainerWait Jul 26, 2024
@thaJeztah
Copy link
Member

Oh! Good one; yes, I was considering if we need an explicit option on cli.StatusError to mark it as "silent" or something; effectively cli.StatusError should (probably?) be used as a wrapper for existing errors to add "status code" to them. Or at least, considering something like

err := foo()
if errdefs.IsNotFound(err) {
    // wrap the error with a atatus-code (and message?)
} 

For this branch I would update the tests to not check for the error-message if that works while we work out some of that.

Such as with `docker run`, if a user CTRL-Cs while attached to a
container, we should forward the signal and wait for the exit from
`ContainerWait`, instead of just returning.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
(cherry picked from commit 7b46bfc)
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard
Copy link
Member Author

For this branch I would update the tests to not check for the error-message if that works while we work out some of that.

Updated.

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

@vvoland vvoland merged commit a35c363 into docker:27.x Jul 26, 2024
87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bugfix PR's that fix bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants