Skip to content

Improve auto-forwarding for GitHub Codespaces and devcontainers.#6780

Merged
mitchdenny merged 11 commits intomainfrom
mitchdenny/local-devcontainer-autoforward
Dec 5, 2024
Merged

Improve auto-forwarding for GitHub Codespaces and devcontainers.#6780
mitchdenny merged 11 commits intomainfrom
mitchdenny/local-devcontainer-autoforward

Conversation

@mitchdenny
Copy link
Copy Markdown
Member

@mitchdenny mitchdenny commented Nov 25, 2024

Description

This PR improves support for automatically forwarding ports for the following dev container scenarios:

  1. GitHub Codespace via browser
  2. GitHub Codespace via VSCode
  3. Local Devcontainer via VSCode

This works by introducing a lifecycle hook that executes after endpoints are allocated. If execution within a codespace or local devcontainer is detected the hook will trigger the update to the machine wide settings.json file based on the configuration of endpoints within resources. In codespaces only http and https endpoints will be forwarded (developers can always set manual forwarding).

There is some timing sensitivity here. The settings file needs to be updated before it sees the endpoint in the console logs, if it doesn't there is a chance that the wrong protocol/label will be applied.

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?
Microsoft Reviewers: Open in CodeFlow

@mitchdenny mitchdenny changed the title Mitchdenny/local devcontainer autoforward Improve auto-forwarding for GitHub Codespaces and devcontainers. Nov 25, 2024
@mitchdenny mitchdenny requested a review from davidfowl November 25, 2024 01:00
@mitchdenny mitchdenny force-pushed the mitchdenny/local-devcontainer-autoforward branch from 55d1717 to ce5666a Compare November 25, 2024 01:09
@davidfowl
Copy link
Copy Markdown
Contributor

davidfowl commented Nov 25, 2024

We can’t mutate global machine state from the apphost.

@mitchdenny
Copy link
Copy Markdown
Member Author

We can’t mutate global machine state from the apphost.

I think this is the only way we can do it unless we are given an API.

@davidfowl davidfowl added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 25, 2024
@mitchdenny mitchdenny force-pushed the mitchdenny/local-devcontainer-autoforward branch from fef7f8b to 9875f94 Compare December 2, 2024 00:35
@mitchdenny
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny mitchdenny removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 2, 2024
@mitchdenny mitchdenny self-assigned this Dec 2, 2024
@mitchdenny mitchdenny added this to the 9.1 milestone Dec 2, 2024
@mitchdenny
Copy link
Copy Markdown
Member Author

@davidfowl

@mitchdenny
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Copy Markdown
Member Author

Disabling a few flaky tests:
#6867
#6866

@mitchdenny mitchdenny requested a review from davidfowl December 5, 2024 08:27
_devcontainersOptions = devcontainersOptions;
}

public async Task AfterEndpointsAllocatedAsync(DistributedApplicationModel appModel, CancellationToken cancellationToken)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we want to use this event instead of BeforeResourceStart? That way it's not blocking startup of the dashboard.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is important to dump out the port forwarded URLs as soon as possible after the ports are allocated so that no other log message triggers port forwarding before the port attributes details are written to the log file. Otherwise they'll be forwarded with incorrect attributes (label missing, incorrect protocol etc).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also even though this executes before the dashboard starts its a local I/O operation so not a huge impact to overall start performance. In my experimentation the dashboard is up and running before I've decided to interact with it.

@mitchdenny mitchdenny merged commit c331e53 into main Dec 5, 2024
@mitchdenny mitchdenny deleted the mitchdenny/local-devcontainer-autoforward branch December 5, 2024 09:28
@davidfowl
Copy link
Copy Markdown
Contributor

Great work @mitchdenny !

@github-actions github-actions Bot locked and limited conversation to collaborators Jan 5, 2025
@github-actions github-actions Bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants