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

Update ports properties and README #23

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Update ports properties and README #23

merged 1 commit into from
Apr 16, 2021

Conversation

bamurtaugh
Copy link
Member

Based on discussion in microsoft/vscode-remote-release#4838, updating the ports properties in devcontainer.json and the README.

  • Add otherPortsAttributes to devcontainer.json and a comment description above it, but leave it commented out
  • Group all the ports properties together sequentially in the devcontainer.json: forwardPorts, portsAttributes, otherPortsAttributes
  • Update portsAttributes:
    • Replace "3000" with regex
    • Remove "notify" behavior on autoforward since it led to two ports being shown, which could be confusing to users
    • Update in-line comment describing the property
  • Update README:
    • In the section about container rebuild to see changes take effect, it used to describe uncommenting forwardPorts. But with portsAttributes and dynamic port forwarding, it feels like making changes to attributes in this property might be the most interesting to users, so I've updated the section to describe how to open a browser on autoforward.

Please let me know any thoughts you have on the above changes - happy to discuss further, make some modifications, or describe things more clearly for users via comments or the README.

Once we decide on a format that works well for the devcontainer.json and README here, I'll extend the same to the other remote samples and merge in changes there.

@bamurtaugh bamurtaugh requested review from alexr00 and Chuxel April 14, 2021 18:29
Copy link
Member

@Chuxel Chuxel left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@wattsjohnny2020 wattsjohnny2020 left a comment

Choose a reason for hiding this comment

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

I

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.

4 participants