Skip to content

Fix some issues around port forwarding:#7152

Merged
davidfowl merged 5 commits intomainfrom
davidfowl/codespace-fixes
Jan 19, 2025
Merged

Fix some issues around port forwarding:#7152
davidfowl merged 5 commits intomainfrom
davidfowl/codespace-fixes

Conversation

@davidfowl
Copy link
Copy Markdown
Contributor

@davidfowl davidfowl commented Jan 19, 2025

Description

  • Make sure there aren't duplicate labels when written to settings files

  • Mark auto forward action for dashboard as open browser and other endpoints as silent.

  • Default to hybrid mode for auto port forward. We want to detect ports from the output, but we also want to tear down the portforward when the process dies.

Fixes #7147
Fixes #7148

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

- Make sure there aren't duplicate labels when written to settings files

- Mark auto forward action for dashboard as open browser and other endpoints as silent.

- Default to hybrid mode for auto port forward. We want to detect ports from the output, but we also want to tear down the portforward when the process dies.

Fixes #7148
Fixes #7147
@davidfowl davidfowl requested a review from mitchdenny January 19, 2025 05:16
- Attempt to make the end to end more reliable by adding a delay between the file writing and logging the output urls.
- Don't forward any ports that aren't described in settings.
- Rename methods, made AddPortForward sync.
- Added more comments around hacks
- Made a code a little more efficient
Copy link
Copy Markdown
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

Looks good. Tested it and is very responsive to port changes. Would be good to avoid the pause but we'll need to wait to hear from the VSCode team.

@mitchdenny
Copy link
Copy Markdown
Member

One thing we might want to consider is whether the port label needs some kind of apphost prefix to make it unique per apphost. in theory someone can two two apphosts within a Codespace just fine.

@davidfowl
Copy link
Copy Markdown
Contributor Author

Looks good. Tested it and is very responsive to port changes. Would be good to avoid the pause but we'll need to wait to hear from the VSCode team.

Sleep always works 😉

One thing we might want to consider is whether the port label needs some kind of apphost prefix to make it unique per apphost. in theory someone can two two apphosts within a Codespace just fine.

I was going to do that (put the apphost name in there as a suffix), but then we would need to attempt to clean up the file. It turns out the VS debugger does not gracefully exit the process so there's no change to clear the file. It could be done on the next run though.

@davidfowl davidfowl merged commit adc401b into main Jan 19, 2025
@davidfowl davidfowl deleted the davidfowl/codespace-fixes branch January 19, 2025 17:20
@davidfowl
Copy link
Copy Markdown
Contributor Author

Failures unrelated to the this (cosmos db)

@davidfowl
Copy link
Copy Markdown
Contributor Author

Oh one other idea I had in general was to clean up finish modeling the dashboard cleanly as a resource. That would fix a few things:

  • Logic to add endpoints would be in a single place
  • We could use health checks to wait until the endpoint is valid (codespace health check)

@mitchdenny
Copy link
Copy Markdown
Member

🤔

@github-actions github-actions Bot locked and limited conversation to collaborators Feb 19, 2025
@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

2 participants