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

Refactor remote debugging commands into common library so we can support functions #522

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

mrcrane
Copy link
Contributor

@mrcrane mrcrane commented Jun 12, 2019

Remote debugging for Azure Functions will be very similar to remote debugging for Azure Functions, since Functions are built on top of the same App Service technology. This PR moves more of the remote debugging code into the vscode-azureappservice library so that Functions can take advantage of it without duplicating code.

This is a rough outline of the steps involved in remote debugging:

  1. Fetch site configuration and check to see if remote debugging is supported
    1. This is code is specific to app services and functions and is not refactored into the common library
  2. Check to see if remote debugging is enabled and enable it if necessary
    1. Functions will use the same "remoteDebuggingEnabled" configuration setting, so this is common.
  3. Start the tunnel proxy
    1. The TunnelProxy code is already in the common library
  4. When debugging ends, disable remote debugging if necessary.

See microsoft/vscode-azureappservice#1045 for more details about where the code is coming from and what the changes would be in the extension.

@nturinski
Copy link
Member

nturinski commented Jun 13, 2019

@mrcrane A lot of the language and functionality seems like it'd still be specific for web apps. Since we don't check the site config, how do we know if the function app supports remote debugging? Do all by default?

Are there any plans for enabling remote debugging for Functions in the near future? I want to try and keep code out of our shared package unless it is actively being used by multiple extensions.

@mrcrane
Copy link
Contributor Author

mrcrane commented Jun 14, 2019

@nturinski Yes my intention is to add support for functions once this PR is completed. This is something we've been talking about but I neglected to properly communicate to your team. I just created an issue covering this plan to give more context: microsoft/vscode-azurefunctions#1344

The intent is that every piece of the code you see in this PR will be used by both functions and web apps. Checking the site config for support is still important but it will be different for web apps and functions, so the implementation will live in the extension code. The associated PR that I mentioned above shows how I am proposing to do this. See startRemoteDebug.ts and the referenced checkForRemoteDebugSupport.ts in microsoft/vscode-azureappservice#1045

@mrcrane
Copy link
Contributor Author

mrcrane commented Jun 14, 2019

Even though I believe the functionality is not web app specific, you make a good point about the language. The word "app" is used a lot and may not be appropriate for functions. It is also used throughout TunnelProxy which is already in place in the common library. Even though functions run on apps, we probably want to hide that fact from customers.

My idea was to model this after the startStreamingLogs functionality which is something that lives in the common library where the implementation is mostly similar between apps and functions. It looks like it uses a logStreamLabel to refer to the thing being operated on. Perhaps we could do something like that?

@nturinski
Copy link
Member

@mrcrane I think the label is just being used for the output channel name
image

That being said, I noticed that the language is purposely vague. I think we could just do that too.

'The app configuration will be updated to enable remote debugging and restarted. Would you like to continue?'; for instance, we could just call it The configuration or something like that.

mrcrane added 2 commits June 25, 2019 13:04
- Change language to remove references to "web app"
- Check for explicitly stopped apps
@mrcrane mrcrane force-pushed the refactor-remotedebug branch from f4366b0 to c6d454a Compare June 25, 2019 23:26
@mrcrane
Copy link
Contributor Author

mrcrane commented Jun 25, 2019

@nturinski
The PR has been updated to reflect the conversation we had last week:

  • The "Stop remote debugging" command has been removed entirely. "Start remote debugging" is now grouped with "SSH into Web App".
  • Removed all references to "web app" in the language. There are still some references to "app" but we determined that Function App is accepted language so "app" could refer to either web apps or function apps.
  • Added a check and error for explicitly stopped apps in order to close out Ping/wake site prior to attempting to connect remote debugging/SSH session vscode-azureappservice#801

mrcrane added a commit to mrcrane/vscode-azureappservice that referenced this pull request Jun 25, 2019
@nturinski
Copy link
Member

Since this is going into the azuretools package, could you localize the strings?

Here is an example of how we localize: https://github.com/microsoft/vscode-azuretools/blob/master/appservice/src/editScmType.ts#L33

@mrcrane mrcrane force-pushed the refactor-remotedebug branch from 19a28b6 to 64eb62d Compare June 28, 2019 23:12
@mrcrane mrcrane force-pushed the refactor-remotedebug branch from 64eb62d to 1e0b26d Compare July 1, 2019 17:01
@mrcrane
Copy link
Contributor Author

mrcrane commented Jul 1, 2019

Sure, I localized all of the strings that can bubble up to the customer in the remote debug commands and in the tunnel proxy. There are still several unlocalized strings in the tunnel proxy code that only logged in the output window and are for low-level diagnostics. I don't think it makes sense to localize them and perhaps some of them should be removed altogether to clean up the output window (in another PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants