[Isidor Nikolic (isidorn)]

fixes #612

This PR simplifies the generated launch.json in the following ways:

• The snippets do not have the default fields any more, making them more concise. The missing fields should be nicely added by node
• When the initial configuraiton is provided we ask the user for the remoteRoot, making it clearer what is the attribute that users change the most

What still needs to be done imho:

• We should read the dockerfile, such that we see the value of the dockerfile WORKDIR and take that as the default, not the hardcoded /usr/src/app
• Read the dockerfile and see if the userconfigured a remote port, if he did than insert the port in the configuration. Currently we do not set any because the node debug will corectly pick the port based on node version
• Tune the wording of the input box, not sure if Please enter your Docker remote root is correct here, maybe WORKDIR instead of remote root.
• Simplify the docker .net launch configuraiton in a similar way

Stephen Weatherford (@StephenWeatherford) let me know what you think. Thanks a lot
fyi Chris Dias (@chrisdias)

[Stephen Weatherford (StephenWeatherford)]

Adding Phillip Hoff (@philliphoff) as he owns the .NET Core debugging part of the code.

[Phillip Hoff (philliphoff)]

The scaffolded launch configuration for Docker .NET Core debugging is fairly lean; it adds only the optional dockerBuild and dockerRun properties, which were an explicit ask when adding the feature. All other properties are inferred by its DebugConfigurationProvider (DCP).

We should read the dockerfile, such that we see the value of the dockerfile WORKDIR and take that as the default, not the hardcoded /usr/src/app

That's not a bad idea, though multi-stage Dockerfiles (such as scaffolded by .NET Core tooling) could complicate that (as you'd want the last WORKDIR specified by the most-descendent stage). An alternative may be to use a DCP to identify the ultimate WorkingDir of the built image/container (via docker inspect) and use that. The Docker .NET Core DCP does this to identify the exposed HTTP port (if any), in order to know where to point the browser on debug.

That said, the /app convention is fairly embedded within the .NET Core tooling. The VS Code DCP was patterned after the VS equivalent, which also makes that assumption.

[Prashanth (PrashanthCorp)]

For any new code added, we tend to avoid thenable in favor of async-await.

Debugging this code has seemed easier when the code is written as :

let remoteRoot = await vscode.window.showInputBox({ value: '/usr/src/app', prompt: 'Please enter your Docker remote root'}); 
return [{
name: 'Docker: Attach to Node',
type: 'node',
request: 'attach',
remoteRoot
}]; 

You would need to make the function async however, so I'll defer to Stephen Weatherford (@StephenWeatherford) whether requiring this standard is worth it here. :)

[Stephen Weatherford (StephenWeatherford)]

Yes, please do convert. Also, please use ext.ui.showInputBox instead of vscode.window.showInputBox. Note that this will throw on user cancel, which I assume makes sense here?

[Isidor Nikolic (isidorn)]

Thanks for the review and suggestions I have pushed a commit which takes care of this.

[Isidor Nikolic (isidorn)]

Phillip Hoff (@philliphoff) thanks for your reply.
Reading fields from the docker file when possible makes good sense to me. And if you get some prototpye of this to work let me know and I will be glad to try it out and give feedback. Note that we do not have to be right 100% of the time, my suggestion is just to present a good default to the user in QuickPick and he can always change it.

In the end I leave making this leaner and more simply to you since I do not know what are the biggest pain points that users face when setting up docker debugging. Let me know if there is something on the vscode side we can do to improve this.

Just for reference what is the scaffolded launch configuration for Docker .NET?

[Isidor Nikolic (isidorn)]

Is there something else that needs to be done on my end here?
Thanks

[Phillip Hoff (philliphoff)]

Isidor Nikolic (@isidorn) For reference, this is the scaffolded Docker .NET Core launch configuration:

{
     "name": "Docker: Launch .NET Core (Preview)",
     "type": "docker-coreclr",
     "request": "launch",
     "preLaunchTask": "build",
     "dockerBuild": {},
     "dockerRun": {}
}

[Stephen Weatherford (StephenWeatherford)]

Phillip Hoff (@philliphoff) I'm assuming you're signing off on this PR... Thx.

[Phillip Hoff (philliphoff)]

Looks ok to me, though I'm less knowledgable about the Node debugging side of things versus the .NET Core debugging side.

[Isidor Nikolic (isidorn)]

Great thanks. Should I click the merge button or will you, not sure what is the procedure here. Thanks!
Also thanks for the scaffolded configuration.

[Phillip Hoff (philliphoff)]

Stephen Weatherford (@StephenWeatherford) Are you ok with the changes?

[Stephen Weatherford (StephenWeatherford)]

Isidor Nikolic (@isidorn), Phillip Hoff (@philliphoff) Thanks!