puremourning / vimspector

vimspector - A multi-language debugging system for Vim
http://puremourning.github.io/vimspector-web
Apache License 2.0
4.08k stars 171 forks source link

Docker workdir support #819

Closed jordanpwalsh closed 3 weeks ago

jordanpwalsh commented 8 months ago

This adds an additional optional parameter to the "remote" configuration allowing the working directory to be set during invocation of docker exec. I was originally working around needing to execute the runCommand in an appropriate working directory by running a shell script but thought an option in the config would be beneficial to prevent needing to add those scripts to the source repository.

Testing output:

Relevant config:

"launch": {
    "remote": {
        "container": "${ContainerID}",
        "workdir": "/opt/app-root/src/pyluglite",
        "runCommand": ["pwd"]
    },
...

Output:

 /opt/app-root/src/pyluglite
puremourning commented 8 months ago

This change is Reviewable

jordanpwalsh commented 8 months ago

Closing due to accidental code reformatting.

jordanpwalsh commented 8 months ago

Re-opened with the file not completely reformatted.

puremourning commented 8 months ago

Thanks for sending a PR but I fear this might be a slippery slope to implementing the entire compose file specification, or at least every argument to docker exec. I would prefer either:

jordanpwalsh commented 8 months ago

If you prefer, I can implement your option 2 instead; I personally prefer that over a custom command. I would like to push back a little though. I can see your point about not wanting to implement a bunch of the compose spec, but I’d argue this isn’t quite that. The docker exec options are quite small — user id, env vars, and working directory — and I could easily see the other two being something to include as well. Working directory is just the one I happened to need at the time.

All that said. Your option of docker_args would suit me just fine too. I’m happy to help.

puremourning commented 8 months ago

That's sort of my point. It's a bit whack-a-mole. I'd rather have option 2 which is just a list of additional args passed to docker exec. Then I never have to change it again if/when docker or podman inevitably change or add something.

jordanpwalsh commented 8 months ago

Sounds good. I’ll update the PR.

jordanpwalsh commented 8 months ago

I updated the branch per your feedback. I thought a format like below would be more intuitive, but opted to use docker_args from your original suggestion so existing configs aren't broken.

What I wanted:

"container": {
    "name": "string",
    "args": ["string", "string"]
}

What I did:


{
    "container": "string"
    "docker_args": ["string", "string"]
}
jordanpwalsh commented 8 months ago

@puremourning I fixed the couple linter complaints. Anything needing to be done about the failing test? Looks like the mac os test failing has been an ongoing thing.

puremourning commented 5 months ago

I've kicked the CI. Fingers crossed 🤞

puremourning commented 5 months ago

Looks like a lint issue

jordanpwalsh commented 5 months ago

@puremourning I don't have your flake8 config and no errors on my end, but I think I found what it was complaining about and fixed it.

puremourning commented 4 months ago

@mergifyio rebase

mergify[bot] commented 4 months ago

rebase

❌ Base branch update has failed

Git reported the following error: ``` Rebasing (1/8) Auto-merging python3/vimspector/debug_session.py CONFLICT (content): Merge conflict in python3/vimspector/debug_session.py error: could not apply c3c2503... Added workdir support for docker exec hint: Resolve all conflicts manually, mark them as resolved with hint: "git add/rm ", then run "git rebase --continue". hint: You can instead skip this commit: run "git rebase --skip". hint: To abort and get back to the state before "git rebase", run "git rebase --abort". Could not apply c3c2503... Added workdir support for docker exec ```
mergify[bot] commented 3 weeks ago

Thanks for sending a PR!