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.
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.
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.
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.
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.
--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! |
|
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 attachby hooking into the underlyingdocker container attachusing it'sNewAttachCommandwith some overrides to support lookup of service name/index => container ID.Usage:
docker compose attach foo # STDIN/STDOUT/ERR connectedIn my case, I had a development environment that uses
dotnet watchinside of a container (started with compose). And then I needed to sendCtrl+Rto reload my server (as needed). And thus the need to attach to the container's STDIN.