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

Convert all non-string configuration properties in debug #115

Closed
Fedma123 opened this issue Nov 18, 2021 · 16 comments · Fixed by #134
Closed

Convert all non-string configuration properties in debug #115

Fedma123 opened this issue Nov 18, 2021 · 16 comments · Fixed by #134
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@Fedma123
Copy link

Requested feature

Support port numbers in attach debug configurations as strings rather than numbers exclusively.

{
    "name": "Python: Current File",
    "type": "python",
    "request": "attach",
    "connect": {
        "host": "localhost",
        "port": "1234"
    }

Motivation

The attach debug configurations expect a number as port number rather than a string. This prevents variable substitution in launch.json. As an example, the following configuration raises an error on the port key: Incorrect type. Expected "number".

{
    "name": "Python: Current File",
    "type": "python",
    "request": "attach",
    "connect": {
        "host": "localhost",
        "port": "${env:DEFAULT_DEBUGGER_PORT}"
    },
    "console": "integratedTerminal"
}

Use case

Assume that you have to debug your application by attaching to a running process. Assume that you package and debug the application into a container by attaching VSCode to the running container. Finally, assume that you have to debug parallel instances of the application on your local machine. In this scenario each debugger must use a distinct port (this is the case for containers in a podman pod at least because they share the same network). The port could be supplied as an environment variable to each container.

@Fedma123 Fedma123 added triage-needed Needs assignment to the proper sub-team feature-request Request for new features or functionality labels Nov 18, 2021
@karthiknadig karthiknadig added needs community feedback and removed triage-needed Needs assignment to the proper sub-team labels Nov 18, 2021
@karthiknadig
Copy link
Member

Thanks for the feature request! We are going to give the community 60 days from when this issue was created to provide 7 👍 upvotes on the opening comment to gauge general interest in this idea. If there's enough upvotes then we will consider this feature request in our future planning. If there's unfortunately not enough upvotes then we will close this issue.

@karthiknadig
Copy link
Member

/cc @int19h

@int19h
Copy link

int19h commented Nov 18, 2021

Note that we already did this for processId before, and for the same exact reason.

Given that it's the second property like that, and that it's hard to predict all the scenarios in which variable substitution might be useful for one property or another, I feel like this should be generalized to all numeric properties. On debugpy side this would be easy, since there's a common path for all typed property queries. However, connect.port for attach is a separate thing, because it's handled entirely by vscode-python.

@brettcannon
Copy link
Member

Thank you to everyone who upvoted this issue! Since the community showed interest in this feature request we will leave this issue open as something to consider implementing at some point in the future.

We do encourage people to continue 👍 the first/opening comment as it helps us prioritize our work based on what the community seems to want the most.

@dyohn
Copy link

dyohn commented Sep 13, 2022

I found this issue as I was preparing to make the exact same feature request! I have a situation where a team of developers needs to actively debug multiple containerized micro-services at once. This works just fine, but the caveat is that that everyone has to modify their local launch.json file to hard code their various debug ports. Obviously, those changes can't be checked into source control without causing tons of useless and contentious changes. Having the ability to specify these ports via environment variables would be much cleaner and less error prone! I really hope this story gets worked; if I knew how/where to make the change I would be willing to pitch in. Alas, I am a poor excuse for a TypeScript developer.

@TheDouglasMann
Copy link

Definitely need this!

@int19h
Copy link

int19h commented Sep 26, 2022

Isn't this a dupe of microsoft/vscode-python#18841?

@dyohn
Copy link

dyohn commented Sep 26, 2022

Isn't this a dupe of microsoft/vscode-python#18841?

18441 handled this case for the listen configuration only. I can't speculate on the how/why of that, but I know for sure that the connect configuration will only accept an integer for port, at least as of VS Code 1.71.1 and Python Extension 2022.14.0. I get the following when I try:

launchJsonError

@int19h
Copy link

int19h commented Sep 27, 2022

I see that I actually commented to that above and forgot about it! 🤦‍♂️

For "listen", the fix had to be on debugpy side - it was actually more extensive that that and covered all numeric debug configuration properties, not just the port.

But for "connect" (or the legacy top-level "port"), it's vscode-python that has to parse the port to establish the connection, and it currently just assumes that it's a valid int:
https://github.com/microsoft/vscode-python/blob/3698950c97982f31bb9dbfc19c4cd8308acda284/src/client/debugger/extension/adapter/factory.ts#L48-L52

This is the part that needs fixing. Although I would recommend doing the same thing that debugpy did, and fix this for all non-string configuration properties.

@paulacamargo25 paulacamargo25 changed the title Support port numbers in attach debug configurations as strings Convert all non-string configuration properties in debug Oct 11, 2023
@paulacamargo25 paulacamargo25 added this to the October 2023 milestone Oct 11, 2023
@paulacamargo25 paulacamargo25 transferred this issue from microsoft/vscode-python Oct 23, 2023
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Oct 23, 2023
@int19h
Copy link

int19h commented Oct 23, 2023

I'm confused as to why this got transferred - it's already implemented on debugpy side for all numeric properties that debugpy handles, and the remaining work is for the extension handling of port number in connect scenarios.

@brettcannon
Copy link
Member

@int19h because all debugger stuff related to VS Code and Python should end up here. Why do you think it stay on the core extension side?

@int19h
Copy link

int19h commented Oct 24, 2023

Sorry, I got confused about which repo it is, I thought it ended up in the core debugpy repo where it was filed originally. You're right, this is the proper tracker for it.

@paulacamargo25 paulacamargo25 added the verification-needed Verification of issue is requested label Nov 27, 2023
@andreamah andreamah added the verification-steps-needed Steps to verify are needed for verification label Nov 28, 2023
@andreamah
Copy link

Are there short verif steps for this feature?

@rzhao271
Copy link
Contributor

I still can't specify the port as a string in a Python attach launch configuration, so it looks like the original request still isn't fixed.

Python launch configuration indicating a type error when the port is "1234" because it's a string

@rzhao271 rzhao271 reopened this Nov 29, 2023
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Nov 29, 2023
@rzhao271 rzhao271 added verification-found Issue verification failed and removed triage-needed Needs assignment to the proper sub-team labels Nov 29, 2023
@paulacamargo25
Copy link
Contributor

paulacamargo25 commented Nov 29, 2023

This issue was solved in the Python Debugger Extension.
Please use the "type": "debugpy" for the configuration.

@paulacamargo25 paulacamargo25 removed verification-steps-needed Steps to verify are needed for verification verification-found Issue verification failed labels Nov 29, 2023
@rzhao271
Copy link
Contributor

LGTM. Thanks!

@rzhao271 rzhao271 added the verified Verification succeeded label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants