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

feat: add a new tutor dev attach command to attach running containers for debugging #1152

Open
wants to merge 5 commits into
base: release
Choose a base branch
from

Conversation

Danyal-Faheem
Copy link
Contributor

Edx-platform developers regularly utilize debuggers inside their codebase and the tutor dev start command does not allow attaching to containers on certain OSes such as MacOS. This command will allow users to attach a single running container using the docker compose attach command.

Caveats:

  • Can only attach one service in one running terminal

@kdmccormick kdmccormick self-requested a review November 7, 2024 21:39
@kdmccormick
Copy link
Collaborator

Thanks for the PR Danyal! This is a problem I have noticed on Ubuntu as well. I'm able to attach to an individual container's stdout by doing, for example, tutor dev start cms, which is what the docs recommend. But I'm not able to run any debugger commands, so it seems that stdin is detached.

I'm not super keen to add a new command, but breakpoint debugging is definitely something we all need. I'll test this out and leave a review soon.

@regisb
Copy link
Contributor

regisb commented Nov 8, 2024

I was hoping I was the only one who observed that issue... The problem is that, in my case, docker compose attach does not work either. I need to run docker attach <containerID>. Is it the case for you as well?

EDIT: actually, tutor dev dc attach lms does allow me to stop at breakpoints. But when I attempt to attach to a running container with tutor dev start lms, adding a breakpoint yields the following error:

WARNING: your terminal doesn't support cursor position requests (CPR).

@Danyal-Faheem
Copy link
Contributor Author

@regisb, I get the same error as you do which prompted me to make this PR.

I also initially thought that docker compose attach <container> wouldn't work and I would need to do docker attach <container> instead, but to my surprise, it works perfectly.

Copy link
Collaborator

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working great for me. Can you update those dev docs that I linked in my comment above?

EDIT: In the dev docs, please also mention that the way to detach from the container without shutting it down is to send Ctrl-p Ctrl-q.

@Danyal-Faheem Danyal-Faheem force-pushed the danyalfaheem/add-dev-attach-command branch from cf149e6 to e54b670 Compare November 13, 2024 09:15
@Danyal-Faheem
Copy link
Contributor Author

Hey @kdmccormick, I've updated the docs as well. Can you take another look and let me know what you think?

@Danyal-Faheem Danyal-Faheem force-pushed the danyalfaheem/add-dev-attach-command branch from 6f2a518 to 5848679 Compare November 13, 2024 09:23
@regisb
Copy link
Contributor

regisb commented Nov 13, 2024

Based on my experience, it seems that docker compose up no longer attaches stdin to the specified containers. I'm wondering what the right fix should be. Is it to add a new command? Or should we automatically run docker compose attach when we run tutor dev start <a single container>?

@kdmccormick
Copy link
Collaborator

@regisb , I really like that idea, especially since I cannot imagine any situation where I run tutor dev start lms without wanting stdin to be attached.

There is only one minor problem:

  • To escape from docker compose up without stopping containers, the sequence is Ctrl-z
  • To escape from docker compose attach without stopping containers, the sequence is Ctrl-p Ctrl-q

If we went with your proposal, that would mean that tutor ... start would have different escape sequences depending on whether you provided zero/multiple argument versus 1 argument. Perhaps we could mitigate the confusion with good docs and by printing a helpful message, like: ℹ️ To detach without stopping the platform, type: .....

What do you think @regisb , go forward with modifying tutor ... start, or stick with tutor dev attach?

@regisb
Copy link
Contributor

regisb commented Nov 19, 2024

Perhaps we could mitigate the confusion with good docs and by printing a helpful message, like: ℹ️ To detach without stopping the platform, type: .....

Yes, I think we should do that. I honestly didn't know about escape commands, as I just ctrl+c all the time. In the process I just discovered that we also need to ctrl+p+q on tutor dev run.

So my vote goes to changing the "start" command. Especially since it's so easy to run tutor dev dc attach, I don't really see a point of adding another command.

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

Successfully merging this pull request may close these issues.

3 participants