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

make the launch.json generation leaner #618

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented Nov 13, 2018

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

@StephenWeatherford let me know what you think. Thanks a lot
fyi @chrisdias

@StephenWeatherford
Copy link
Contributor

Adding @philliphoff as he owns the .NET Core debugging part of the code.

@philliphoff
Copy link
Contributor

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.

request: 'attach',
remoteRoot
}];
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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 @StephenWeatherford whether requiring this standard is worth it here. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@isidorn
Copy link
Contributor Author

isidorn commented Nov 15, 2018

@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?

@StephenWeatherford StephenWeatherford added this to the 0.5.0 milestone Nov 19, 2018
@isidorn
Copy link
Contributor Author

isidorn commented Nov 20, 2018

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

@philliphoff
Copy link
Contributor

@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": {}
}

@StephenWeatherford
Copy link
Contributor

@philliphoff I'm assuming you're signing off on this PR... Thx.

Copy link
Contributor

@philliphoff philliphoff left a comment

Choose a reason for hiding this comment

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

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

@isidorn
Copy link
Contributor Author

isidorn commented Nov 27, 2018

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.

@philliphoff
Copy link
Contributor

@StephenWeatherford Are you ok with the changes?

@StephenWeatherford StephenWeatherford merged commit ef4f0ad into master Nov 28, 2018
@StephenWeatherford
Copy link
Contributor

@isidorn, @philliphoff Thanks!

@ejizba ejizba removed this from the 0.6.0 milestone May 8, 2019
@ejizba ejizba deleted the isidorn/leanerLaunchJson branch July 4, 2019 00:12
@microsoft microsoft locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify generated launch.json
5 participants