-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add remote debugging support for Node.js on Linux App Service plans #1458
Conversation
return; | ||
} | ||
|
||
throw new Error('Azure Remote Debugging is currently only supported for Node.js Function Apps running on Linux App Service plans.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the PR description - this only works on dedicated plans? I feel like that should be highlighted in the messaging since consumption is so much more popular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this only works on dedicated plans so far. I have tried to highlight that in all of the messaging, including this error message:
Azure Remote Debugging is currently only supported for Node.js Function Apps running on Linux App Service plans.
All of the Azure Portal UI I have seen refers to dedicated plans as App Service plans, so I am trying to use that same terminology. For example, in the UI to create a Function App:
I'm happy to update the messaging if you have a suggestion about how to improve it. @fiveisprime Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah didn't realize you were using "App Service Plan" to differentiate. I see what you mean about the dropdown, but even the portal is inconsistent. It lists consumption plans as app service plans (after creating):
What about something like this:
Azure Remote Debugging is currently only supported for Node.js Function Apps running on Linux App Service plans (excluding consumption plans).
I know it's kinda ugly, but again it seems like a super important distinction since the majority of plans will be consumption.
We could also consider splitting the message out into separate cases:
- Remote Debugging is currently only supported for Linux Function Apps.
- Remote Debugging is currently only supported for Node.js Function Apps.
- Remote Debugging is currently not supported for Function Apps running on consumption plans.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into the conditional messaging approach but I wasn't able to find a good solution.
I went ahead and changed the messaging to be very clear, for this message and also for the description of the feature flag. Let me know what you think.
51e9b52
to
46d641a
Compare
I rebased on master and force-pushed to resolve merge conflicts. |
Thanks! |
This will address #1344 and it is the culmination of the refactoring work already done in microsoft/vscode-azuretools#522 and microsoft/vscode-azureappservice#1045
Given that the refactoring has already been done, this code is fairly straightforward. The main concern is around the fact that there is already a feature in place that supports Java remote debugging on Windows.
A few months ago (sorry for the delay) @fiveisprime @nturinski and I discussed this issue and we decided to go with 2 different remote debugging feature flags so that we can maintain both experiences for the time being. Since we expect this Node.js experience to be the main experience moving forward, I have reappropriated the
enableRemoteDebugging
flag (Python support is coming soon). Java debugging support will now be enabled usingenableJavaRemoteDebugging
. The settings page will now look like:I'm happy to hear any feedback on how to make that experience less confusing.
More notes:
remoteDebuggingEnabled
setting. This is feasible but probably low priority.