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

Add docker compose attach? (to support STDIN) #11153

Closed
g0t4 opened this issue Nov 2, 2023 · 15 comments
Closed

Add docker compose attach? (to support STDIN) #11153

g0t4 opened this issue Nov 2, 2023 · 15 comments

Comments

@g0t4
Copy link
Contributor

g0t4 commented Nov 2, 2023

Description

Similar to docker container attach, would docker compose attach be a reasonable thing to add? I'd like to attach to STDIN/OUT simultaneously and send input to my service container(s) and see their output. AFAICT, docker compose up only attaches STDOUT.

For example, I have a compose project w/ a single service that runs dotnet watch. If changes can't be hot reloaded then I can send Ctrl+R to the dotnet watch process to trigger a rebuild.

I can use docker container attach to send Ctrl+R currently as a workaround with stdin_open: true and tty: true in my service definition.

Ideally this would also be combined with docker compose up to bring up and then attach STDIN right away.

  • perhaps docker compose up -i/--interactive?
  • and either a single service limitation
  • or if multi service then broadcast to all STDIN
  • I would also understand if this is too confusing to merge with up in which case attach alone would be great
@g0t4 g0t4 changed the title Add docker compose attach? Add docker compose attach? (to support STDIN) Nov 2, 2023
@ndeloof
Copy link
Contributor

ndeloof commented Nov 2, 2023

I don't have any objection to a new docker compose attach command.
Adding --attach to up would introduce requirement to select a single service and then would break the logic of the command IMHO, better have a dedicated one (not to mention code would be easier to read then :P)

@g0t4
Copy link
Contributor Author

g0t4 commented Nov 3, 2023

I'll take a stab at the new subcommand and see where I can get with it.

@g0t4
Copy link
Contributor Author

g0t4 commented Nov 3, 2023

Nevermind for now, I just found an existing attach service in docker/compose, I'll see if it works for my use case before we worry about using docker/cli's impl

@ndeloof I noticed for docker compose exec you modified docker/cli to make RunExec public: docker/cli@2d26839

Given that runAttach in docker/cli(https://github.com/docker/cli/blob/814f70749abd0e131da6456d8895b5695950a4af/cli/command/container/attach.go#L73-L74) is not public, what would be the best course of action to implement docker compose attach? How long would changes to docker/cli take to get through to docker/compose?

@g0t4
Copy link
Contributor Author

g0t4 commented Nov 4, 2023

@ndeloof got a first pass working!

main...g0t4:compose:11153-compose-attach

IMO the best course is to make docker/cli's runAttach => public as RunAttach and invoke that from compose attach. Just like compose exec defers to RunExec which is a very similar use case to attach in terms of I/O.

Doubly so I am convinced of this given docker compose attachdocker container attach $(docker compose ps -q foo)

@ndeloof
Copy link
Contributor

ndeloof commented Nov 4, 2023

we for sure can make runAttach public for the same reason (and ideally update func to pass command context) to avoid too much code duplication. But if you have a short-term implementation that's also fine, just need to note this to be removed as long as docker/cli is updated for our needs.

@ndeloof
Copy link
Contributor

ndeloof commented Nov 6, 2023

see docker/cli#4637

@g0t4
Copy link
Contributor Author

g0t4 commented Nov 6, 2023

Thanks to your comment about command context... I just realized NewAttachCommand is already exposed and I got a prototype working with just that. I will start a new branch with that and submit a PR if that sounds good to you?

Here's the prototype commit: g0t4@41950d5

This can be a stop gap until we can call RunAttach or maybe this is preferred given it attaches to the stable contract of the docker container attach.

@g0t4
Copy link
Contributor Author

g0t4 commented Nov 6, 2023

@ndeloof Here we go, working great for my test cases, is this ready to start a PR and refine?
main...g0t4:compose:11153-compose-attach-override-container-attach-cmd

@ndeloof
Copy link
Contributor

ndeloof commented Nov 6, 2023

please open a PR

@ndeloof
Copy link
Contributor

ndeloof commented Jan 18, 2024

closed by #11181

@ndeloof ndeloof closed this as completed Jan 18, 2024
@Likqez
Copy link

Likqez commented Apr 12, 2024

Is this supposed to only work on swarm?

@ndeloof
Copy link
Contributor

ndeloof commented Apr 12, 2024

@Likqez this is docker compose, nothing related to swarm here

@Likqez
Copy link

Likqez commented Apr 12, 2024

Thanks for the fast reply. Yes, I agree, it should not have to do anything with swarm.

I was facing an issue where docker compose attach always responded with is not running.
I thought about stacks when it was services. Asked here before creating a stupid new issue.

Got it working. Thanks and great work!
Feature was needed.

@g0t4
Copy link
Contributor Author

g0t4 commented May 7, 2024

@ndeloof now that we have docker compose up --watch, which is fantastic, should attach be added to up too? And/or perhaps just adjust the existing --attach to also attach to STDIN?

@ndeloof
Copy link
Contributor

ndeloof commented May 7, 2024

@g0t4 no. attach to stdin require a single container being command target, which is what compose run should be used for

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

No branches or pull requests

3 participants