-
Notifications
You must be signed in to change notification settings - Fork 720
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
Changes from all commits
5863688
e3f71d2
a47fc40
3da02bd
2ce645e
a7dbbf8
4868cc9
a54147d
9d12378
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| namespace Aspire.Hosting.ApplicationModel; | ||
|
|
||
| /// <summary> | ||
| /// Specifies which connection information should be injected into environment variables when <c>WithReference()</c> is invoked. | ||
| /// 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 commentThe reason will be displayed to describe this comment to others. Learn more. We can follow up on naming. |
||
|
|
@@ -25,7 +25,17 @@ public enum ReferenceEnvironmentInjectionFlags | |
| ConnectionProperties = 1 << 1, | ||
|
|
||
| /// <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}". | ||
sebastienros marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What is the reason we have an index at the end? How are those scenarios going to work with the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
In that case is If you have 5 endpoints named differently, why isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe reason will be displayed to describe this comment to others. Learn more. There isn't any ENV defined by aspire that is not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. We don't need it here. |
||
| /// </summary> | ||
| All = ConnectionString | ConnectionProperties | ||
| ServiceDiscovery = 1 << 2, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is |
||
|
|
||
| /// <summary> | ||
| /// Each endpoint defined on the resource will be injected using the format "{RESOURCENAME}_{ENDPOINTNAME}". | ||
sebastienros marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// </summary> | ||
| Endpoints = 1 << 3, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow up API review, I think this whole enum is unclear. |
||
|
|
||
| /// <summary> | ||
| /// Connection string, connection properties and service endpoints will be injected as environment variables. | ||
| /// </summary> | ||
| All = ConnectionString | ConnectionProperties | ServiceDiscovery | Endpoints | ||
| } | ||
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
.WithNpmPackageInstallationin 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.