-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Implement docker compose attach
#11181
Implement docker compose attach
#11181
Conversation
Hello @g0t4 |
|
cmd/compose/attach.go
Outdated
opts.Service = args[0] | ||
} | ||
|
||
containerID, err := backend.LookupContainer(ctx, projectName, opts) |
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.
please mimic the exec
commands, and have api.Backend to declare a new Attach
command, then rely on docker/cli for implementation if relevant.
This for sure requires to duplicate flags definition, but those don't change much and this is a general UX-homogeneity issue we should address in a global way
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.
Ok so you would prefer that I not reuse NewAttachCommand
and instead hook into RunAttach
like exec
does with RunExec
?
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.
And btw, if I hook into RunAttach
I can easily add back the ability to docker compose attach
(w/o service name) and have it grab the first container in the project. I removed that when using NewAttachCommand
due to some friction with overriding validation.
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.
Yes indeed, I'd prefer not to rely on NewAttachCommand
here (sharing command and flags definition with docker CLI is a larger effort we need to investigate)
I can easily add back the ability to docker compose attach (w/o service name) and have it grab the first container in the project
please don't, containers in a project are not ordered, so this won't be deterministic. Better rely on the same selection mechanism used by compose exec
to enforce an homogeneous UX
|
cmd/compose/attach.go
Outdated
return err | ||
} | ||
|
||
// TODO? --dry-run? would it be helpful to print the matching container name/ID and not attach? |
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.
--dry-run
apply to commands which could mutate the state of your stack
95f0ea1
to
2480c44
Compare
@ndeloof ok I finally got a chance to rework things to use RunAttach, I had to update to Let me know what you think. |
nice! please amend your commit to sign them off, this is required for any contribution by legals. |
c7a93d4
to
ccbe733
Compare
done... rebased with sign off do you want me to open a separate PR for v1.25? |
ccbe733
to
44e653d
Compare
@ndeloof rebased |
run |
44e653d
to
e105f16
Compare
Ok I made a rebase mistake, brb with a fix. |
Ok, @ndeloof fixed (praise the reflog gods)... and ran |
0da8c94
to
057ba8e
Compare
signoff done too |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11181 +/- ##
==========================================
- Coverage 56.57% 56.21% -0.36%
==========================================
Files 134 137 +3
Lines 11454 11707 +253
==========================================
+ Hits 6480 6581 +101
- Misses 4351 4476 +125
- Partials 623 650 +27 ☔ View full report in Codecov by Sentry. |
Please rebase and run |
…er attach) Signed-off-by: Wes Higbee <wes.mcclure@gmail.com>
…rlying docker container attach Signed-off-by: Wes Higbee <wes.mcclure@gmail.com>
Signed-off-by: Wes Higbee <wes.mcclure@gmail.com>
057ba8e
to
dcf6bd7
Compare
done! |
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
We should add an e2e test as a follow up PR |
@glours is there a similar e2e test in mind that you think would work well with |
What I did
I implemented
docker compose attach
by hooking into the underlyingdocker container attach
using it'sNewAttachCommand
with some overrides to support lookup of service name/index => container ID.Usage:
docker compose attach foo # STDIN/STDOUT/ERR connected
In my case, I had a development environment that uses
dotnet watch
inside of a container (started with compose). And then I needed to sendCtrl+R
to reload my server (as needed). And thus the need to attach to the container's STDIN.