-
Notifications
You must be signed in to change notification settings - Fork 731
Support simplified service url ENV format #12141
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
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12141Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12141" |
368ccc6 to
41399a4
Compare
a269804 to
5863688
Compare
|
|
||
| ### Experiencing the app | ||
|
|
||
| Before starting the app host run `npm install` in each javascript folder. |
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.
Will fix this.
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.
Are there not calls to .WithNpmPackageInstallation in the AppHost?
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.
No, didn't know about it then Damian told me. Should I add it in this PR? @IEvangelist note that the Vite sample doesn't work at all, issues with authenticating on npm, not sure if the extension method will take care of this.
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.
No, dont' add it to this PR.
|
This approach looks solid. Since we're not trying to do any fancy service discovery, it's a very simple name based scheme. |
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.
Pull Request Overview
This PR introduces simplified service URL environment variable format that is more polyglot-friendly. It adds the ability to generate environment variables in the format {RESOURCENAME}_{ENDPOINT} (e.g., WEATHERAPI_HTTPS) alongside the existing .NET service discovery format.
Key changes:
- Expands
ReferenceEnvironmentInjectionFlagsenum to support new endpoint injection patterns - Updates environment variable generation logic to emit both service discovery and simplified endpoint formats
- Modernizes existing playground JavaScript applications to use the new simplified format
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ApplicationModel/ReferenceEnvironmentInjectionFlags.cs |
Adds new flags for ServiceDiscovery and Endpoints injection |
src/Aspire.Hosting/ResourceBuilderExtensions.cs |
Updates environment variable generation to support both formats and renames method |
src/Aspire.Hosting.DevTunnels/DevTunnelResourceBuilderExtensions.cs |
Updates dev tunnel references to use new injection patterns |
tests/Aspire.Hosting.Tests/WithReferenceTests.cs |
Adds tests for new simplified environment variable format |
playground/AspireWithJavaScript/* |
Updates JavaScript applications to use simplified environment variables |
| Various test snapshots | Updates expected output to include new environment variable format |
src/Aspire.Hosting/ApplicationModel/ReferenceEnvironmentInjectionFlags.cs
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ReferenceEnvironmentInjectionFlags.cs
Show resolved
Hide resolved
|
Update this template aspire/src/Aspire.ProjectTemplates/templates/aspire-py-starter/13.0/frontend/vite.config.ts Line 11 in 6733971
|
|
Dev tunnels as a custom WithReference method that should be updated. |
I thought I did, or did I miss another one? |
davidfowl
left a comment
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.
Looks good!
|
Simingly unrelated tests are failing on macos now, tried 3 times. These work locally on macos though: |
|
Those are my bugs #12160, there are still more to delete. |
| /// Specifies which connection or endpoint information should be injected into environment variables when <c>WithReference()</c> is invoked. | ||
| /// </summary> | ||
| [Flags] | ||
| public enum ReferenceEnvironmentInjectionFlags |
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.
We can follow up on naming.
| /// <summary> | ||
| /// Each endpoint defined on the resource will be injected using the format "{RESOURCENAME}_{ENDPOINTNAME}". | ||
| /// </summary> | ||
| Endpoints = 1 << 3, |
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.
I'm not sure Endpoints is the right name here. Maybe SimplifiedEndpoints? or SimpleEndpoints?
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.
Follow up API review, I think this whole enum is unclear.
| /// Each endpoint defined on the resource will be injected using the format "services__{resourceName}__{endpointName}__{endpointIndex}". | ||
| /// </summary> | ||
| All = ConnectionString | ConnectionProperties | ||
| ServiceDiscovery = 1 << 2, |
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.
Is ServiceDiscovery enough in this context? Is it really .NET ServiceDiscovery? Even using the Endpoints format is still "service discovery", right? Just not the Microsoft.Extensions.ServiceDiscovery format.
|
|
||
| /// <summary> | ||
| /// Both connection string and connection properties will be injected as environment variables. | ||
| /// Each endpoint defined on the resource will be injected using the format "services__{resourceName}__{endpointName}__{endpointIndex}". |
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.
endpointIndex
What is the reason we have an index at the end? How are those scenarios going to work with the Endpoints format?
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.
We don't need index because each endpoint gets a single env variable. The service discovery format has a one to many relationship with endpoints. You can have 5 endpoints named differently with an http scheme and use the resource name (e.g. http://foo to refer to it). That will load balance across those 5 addresses at runtime.
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.
You can have 5 endpoints named differently with an http scheme and use the resource name (e.g. http://foo/ to refer to it). That will load balance across those 5 addresses at runtime.
In that case is endpointName correct above? Or is it uri.scheme?
If you have 5 endpoints named differently, why isn't services__{resourceName}__{endpointName} enough?
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.
The service discovery implementation relies on IConfiguration and that is how you represent arrays:
{
"services": {
"foo": [
"http://locahost:5006",
"http://locahost:5002"
"http://locahost:5004"
]
}
}"services__foo__http__0" The system does some munging to support different configuration formats but they are all ugly to support this model.
cc @ReubenBond
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.
and that is how you represent arrays
But in this new format, we aren't going to support arrays?
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.
There isn't any ENV defined by aspire that is not __0. We only have it because it's an array, but always use a single value right now. So for the new ENV there is no reason to have it used at all.
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.
Correct. We don't need it here.
| builder.WithEnvironment(envVarName, uri.ToString()); | ||
| if (flags.HasFlag(ReferenceEnvironmentInjectionFlags.Endpoints)) | ||
| { | ||
| var envVarName = $"{externalService.Resource.Name.ToUpperInvariant()}"; |
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.
Does this need the uri.Scheme in it?
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.
I only added these if it was necessary to discriminate from other resources. Here the resource is ExternalServiceResource and has a single scheme, so there could only be a conflict if the same resource was also referenced using another interface WithReference call. And in these cases the endpoint name will be used as a discriminator. So I think it's safe to not include it here.
| // In publish mode we can't read the parameter value to get the scheme so use 'default' | ||
| envVarName = $"services__{externalService.Resource.Name}__default__0"; | ||
| discoveryEnvVarName = $"services__{externalService.Resource.Name}__default__0"; | ||
| endpointEnvVarName = externalService.Resource.Name.ToUpperInvariant(); |
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.
Does this need _default in it?
Also, how is someone supposed to code against this scenario where it changes from uri.Schema to default after publish?
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.
I don't fully understand why we need the scheme at all. We just need to decide if we should have another prefix for external services so they don't collide (though resource names are unique).
src/Aspire.ProjectTemplates/templates/aspire-py-starter/13.0/frontend/vite.config.ts
Outdated
Show resolved
Hide resolved
eerhardt
left a comment
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.
LGTM. Except one fat fingers.
…rontend/vite.config.ts Co-authored-by: Eric Erhardt <[email protected]>
Description
WEATHERAPI,WEATHERAPI_HTTPSReferenceEnvironmentInjectionAnnotationto allow the customization per resource or per resource type which should get Service Discovery endpoints or simple ones.Fixes #12069
Checklist
<remarks />and<code />elements on your triple slash comments?