-
Notifications
You must be signed in to change notification settings - Fork 685
Initial integration of codespace URL rewriting logic into hosting. #6183
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
Conversation
{ | ||
var uri = new Uri(originalUrlSnapshot.Url); | ||
|
||
if (!originalUrlSnapshot.IsInternal && (uri.Scheme == "http" || uri.Scheme == "https") && uri.Host == "localhost") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why these values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know what `IsInternal' really means - do you?
As for only exposing http
and https
schemed endpoints - from what I could tell that is the only option it supported. It doesn't seem to support anything other than HTTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know what `IsInternal' really means - do you?
No
await Task.Delay(5000, stoppingToken).ConfigureAwait(false); | ||
} while (!stoppingToken.IsCancellationRequested); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you doing here? If WatchAsync
errors then won't it throw an error right out of this method? Also, there should be a log message if there was an error and you need to recover.
Add a test that covers this situation to verify it works.
I assume this is also post 9.0 |
Co-authored-by: James Newton-King <[email protected]>
I think so. This change is about the app host changes required to support codespaces better. But there are still some gotchas around the codespace devcontainer itself which I don't think are fully fleshed out. For example you need a devcontainer which supports Docker-in-Docker (ideally) which the default universal one does. But then you want to install the C# DevKit. I don't know if we've got options to streamline this experience for customers. |
One option would be to have a GitHub template with the DevContainer pre-configured with the right decontainer file and the recommended VSCode extensions and then the workflow would be that people create their repo based on that template, and then do dotnet new themselves. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
var gitHubCodespacesPortForwardingDomain = GetRequiredCodespacesConfigurationValue(GitHubCodespacesPortForwardingDomain); | ||
var codespaceName = GetRequiredCodespacesConfigurationValue(CodespaceNameEnvironmentVariable); | ||
|
||
do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment that this runs for the lifetime of the host so it can react to resources as they start and new resources being added?
Co-authored-by: James Newton-King <[email protected]>
Description
Adds support for GitHub Codespaces by automatically rewriting the URLs in the dashboard to point to the GitHub Codespaces proxied events.
Fixes #1178
Checklist
<remarks />
and<code />
elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow