Skip to content

Conversation

@bamurtaugh
Copy link
Member

@bamurtaugh bamurtaugh commented Apr 16, 2021

Update ports attributes following practices from the Node PR: microsoft/vscode-remote-try-node#23.

Main difference is instead of using a regex (I couldn't find a great regex for this app):

I kept "9000" but updated the in-line comment to describe the ability to use a range or regex.

Since we recently updated the vscode-dev-containers repo definitions to use Pylance, updated this sample to use Pylance as well (install language server and extension).

*I was initially just going to merge in changes to the other remote-samples following the format from the Node PR, but since it seems like not all samples may be as suited to using a regex, wanted to get another set of eyes on this PR (also since I'm updating to use Pylance).

@bamurtaugh bamurtaugh requested review from Chuxel and alexr00 April 16, 2021 20:19
@bamurtaugh
Copy link
Member Author

bamurtaugh commented Apr 19, 2021

In discussion with @egamma, I think there are a couple of updates in how we can approach this PR and the remaining samples.

  • The devcontainer.json's are getting longer, so rather than explaining via comment in each sample how the user can leverage portsAttributes, we point to docs: Use 'portsAttributes' to set default properties for specific forwarded ports. More info: https://code.visualstudio.com/docs/remote/devcontainerjson-reference.
  • I got the above "Learn more" format from the remoteUser comment in remote-try-node, so we should also update this sample (and any others missing it) to point to the root doc
  • remote-try-node was the only sample I updated to use a regex in its actual code, but we introduced the regex for cases where the port is dynamic, and in that case, the port is known to be 3000. Also, in all these other sample PRs I've opened, I didn't use a regex, so using actual port numbers in the samples would be most consistent (I'll update remote-try-node accordingly).

@Chuxel @alexr00 I know you've already approved based on the current status, but I've made slight updates based on the above.

@bamurtaugh
Copy link
Member Author

Updated Node PR to address the above (go back to "3000" instead of regex but add an example of regex in readme, simplify comment above portsAttribute to point to doc, reference non-root doc): microsoft/vscode-remote-try-node#25.

Also updated outstanding PRs in remaining samples to point to documentation rather than mentioning regex and range directly, and referenced the non-root doc to attempt to keep devcontainer.json simpler and more consistent in the samples:

Since I believe these changes are in line with what's already been discussed and approved in prior PRs, I think they should be safe to merge in next day or so. But if folks have other thoughts, definitely happy to discuss prior to merging. Thank you all!

@bamurtaugh bamurtaugh merged commit 89fdeff into main Apr 20, 2021
@bamurtaugh bamurtaugh deleted the ports branch April 20, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants