-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
attach: wait for exit code from ContainerWait
#5297
attach: wait for exit code from ContainerWait
#5297
Conversation
Oop I forgot to commit something 😅 |
f4bb939
to
4ecb706
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5297 +/- ##
==========================================
- Coverage 61.45% 61.00% -0.46%
==========================================
Files 299 296 -3
Lines 20856 20847 -9
==========================================
- Hits 12818 12717 -101
- Misses 7122 7212 +90
- Partials 916 918 +2 |
5a22c77
to
cf42238
Compare
I've spent too long trying to figure out why I'd broken it on the |
cf42238
to
3529c25
Compare
3529c25
to
8310aec
Compare
@thaJeztah @vvoland PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; although I'd love to have some tests that would also verify if the returned exit code came from the right place (CLI vs container process), for example:
- docker run --name asdf -d alpine sh -c 'trap "exit 33" SIGINT; sleep 5; exit 34'
- docker attach asdf
- send SIGINT to the cli process
- check if the exit code is 33 and not 130 (SIGINT)
@vvoland Oh! That was my idea with this test, since if the CLI was exiting the exit code would not be 130 but |
8310aec
to
9853b17
Compare
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 <[email protected]>
9853b17
to
7b46bfc
Compare
- What I did
Follow up to #5295
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 fromContainerWait
, instead of just returning.(see
docker attach
docs for a clear explanation of the desired behavior)- How I did it
Used an uncancellable context for the
ContainerWait
call.- How to verify it
Run the test:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)