Review whether the recent port attributes improvements should be demonstrated in the samples

[bamurtaugh]

We recently updated the remote-try-* samples based on the new remote ports attributes.

With additional updates (i.e. https://github.com/microsoft/vscode-docs/blob/main/remote-release-notes/v1_55.md#use-a-regex-for-portsattributes, https://github.com/microsoft/vscode-docs/blob/main/remote-release-notes/v1_55.md#configure-default-port-detection-behavior), we should review whether any further changes should be demonstrated in the samples to help users and raise awareness of these settings.

[bamurtaugh]

Also related in ports attributes documentation and updates: Add portsAttributes to devcontainer.json doc #4711.

[bamurtaugh]

@alexr00 I'd love to get your input here.

We've already updated the remote-try-* samples that involve port forwarding to include the "portsAttributes" setting.

As an example, in the Go sample, the README references "portsAttributes" (and how the forwarded ports will appear in an organized table and with labels), and the devcontainer.json has the actual setting.

In the latest remote release notes (and #4623), I saw where we call out being able to use a regex for portsAttributes, along with the new remote.otherPortsAttributes.

Do you think we should demonstrate either of these in the samples?

The remote.otherPortsAttributes example included in the release notes seemed like a potential candidate to me:

"remote.otherPortsAttributes": {
    "onAutoForward": "silent"
}

I also saw in a test plan item from the iteration that the try-Node sample was a possible starting point for testers for the regex setting, so I wasn't sure if we wanted to add it to our samples for all users as a reference?

[alexr00]

I think including the regex match option for portsAttributes in the node sample and maybe other examples makes a lot of sense.

I'm less sure of otherPortsAttributes. If that's included, and users base their projects off of the samples, then they might not get good auto port forwarding behavior if their project doesn't use the same ports as the sample. Is there a president for including a property/setting in devcontainer.json and having it commented out?

[bamurtaugh]

Including the regex match option makes sense to me too! It'll help not only raise awareness but also provide a helpful example of how to implement it, since this may sound tricky to folks at first.

That's also a good point about otherPortsAttributes and potential unintended behavior; we wouldn't necessarily want to include it by default if users base their projects off the samples, or start modifying the samples and don't fully realize what otherPortsAttributes is doing.

Is there a president for including a property/setting in devcontainer.json and having it commented out?

When we adjusted the samples to use dynamic port forwarding, we kept the forwardPorts property in devcontainer.json as a comment. For example, in remote-try-node: https://github.com/microsoft/vscode-remote-try-node/blob/main/.devcontainer/devcontainer.json#L22.

We also explain this setting in the README if users would like to modify and rebuild their container:

---

Based on this discussion, here's a proposal for slightly updated port forwarding in the samples:

• Add the regex match option to portsAttributes
• Group all the ports properties together sequentially in the devcontainer.json:
  • forwardPorts, portsAttributes, otherPortsAttributes
  • Make sure each property has a comment in devcontainer.json explaining it
  • Only leave portsAttributes uncommented; forwardPorts and otherPortsAttributes serve as helpful references for users
• Ensure README is up-to-date about port forwarding
  • Looks like the property accidentally reads forwardedPorts instead of forwardPorts in current sample(s) README, so fix that
  • I don't think we necessarily need to call out otherPortsAttributes if we're already referencing it in the devcontainer.json and have a comment above it? But if you think it makes sense to include it in the README, I can add a sentence or two.

Does that sound like a reasonable plan? cc @Chuxel

[alexr00]

All that sounds great to me.

[Chuxel]

@bamurtaugh Lets also move the ports out of the settings object since this is not VS Code specific per-se and we continue to see interest in using the format elsewhere.

[bamurtaugh]

@Chuxel I want to make sure I understand what you mean here; the current ports properties are top-level properties not part of settings in the devcontainer.json samples. For example, in remote-try-node:

Is there something else we should update within the samples or docs?

[bamurtaugh]

@alexr00 I've tried different expressions in "portsAttribute," and when I run the app, the attributes are respected and the port forwards correctly, but I end up with 2 ports rather than one.

If I use "3000" instead of a regex, I get just one forwarded port.

Is this expected? Am I using the setting incorrectly?

[bamurtaugh]

Also opened microsoft/vscode-docs#4462 to update docs to reference the latest properties.

[alexr00]

@bamurtaugh this is actually expected. Here's a breakdown of what's happening:

1. You F5 or otherwise use the JS Debug extension to run your app.
2. VS Code notices that there are several new ports: the one that your app is running on (3000) and 3ish more that come from JS Debug.
3. VS Code asks the JS Debug extension for the attributes of all 4ish of these ports: should they be auto forwarded?
4. JS Debug says that all of the ports other than 3000 belong to JS Debug and the user shouldn't see them. They should be ignored.
5. VS Code then looks at your settings and sees the portsAttributes that are in the devcontainer.json. Two of the ports match your regex ./server.js. This is unavoidable because you app running on 3000 and one of the debug ports have almost identical command lines. Because these are considered machine specific settings, they take precedence over what the JS Debug extension said in step 4.
6. Both ports with the matching command line get the specified "notify" behavior.

Does that make sense? I think it is correct to prefer what is in the devcontainer.json over what JS Debug says, but if you feel otherwise please let me know. If you leave the onAutoForward out, then you'll get the behavior that you're looking for.

[bamurtaugh]

@alexr00 Thank you for this breakdown! I can see why this is happening and realize now how the regular expression matches the process from JS Debug.

I thought "onAutoForward" was going to be required to notify me about port 3000, but I see that is not the case, and I'm still notified about port 3000 even without it.

I think my remaining question is why I'm still notified about port 3000, even when leaving out "onAutoForward": "notify"? Is this because it's the default behavior if there's just one forwarded port the user should see?

I think it is correct to prefer what is in the devcontainer.json over what JS Debug says, but if you feel otherwise please let me know.

At first, I was thinking I'd prefer something different here, i.e. to still not see the debug ports by default.

However, the more I think about it, it feels like our general principle is for devcontainer.json to be the final/upper-level set of settings that apply, and where users can "override" other things, so I think this is correct. Also, microsoft/vscode#115616 feels like it'd end up addressing my main concern here anyways, which isn't necessarily with how settings in general apply but more so with confusion around debug ports.

[alexr00]

You get notified for 3000 because the default for all discovered ports is to notify.

microsoft/vscode#115616 is already implemented here, which is why by default you don't see the debug port.

[bamurtaugh]

Thank you for all of the collaboration here! Closing this issue since the ports updates have been merged into the remote samples. If you have any other feedback, please let me know.