Implement GetConnectionProperties for new connection string formatting #916
Conversation
…ng resources Co-authored-by: aaronpowell <[email protected]>
| /// <inheritdoc /> | ||
| public ReferenceExpression ConnectionStringExpression => | ||
| ReferenceExpression.Create( | ||
| $"{scheme}://{UserNameReference}:{PasswordParameter}@{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}"); |
There was a problem hiding this comment.
At what point will it be able to use the encoding from 13.0 (Password:uri)?
There was a problem hiding this comment.
You can add :uri formatter suffix on values that need to be url-encoded:
ReferenceExpression.Create(
$"{scheme}://{UserNameReference}:{PasswordParameter:uri}@{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}");
* Initial plan * Replace IDistributedApplicationLifecycleHook with event-based patterns Co-authored-by: aaronpowell <[email protected]> * Add tests for installer resources and update Dapr tests Co-authored-by: aaronpowell <[email protected]> * Refactor Dapr to use IDistributedApplicationEventingSubscriber Co-authored-by: aaronpowell <[email protected]> * Fixing build issue * Updating the endpoint port * Disabling some tests so we can move forward in other parts of the Aspire 13 update --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Aaron Powell <[email protected]>
* Initial plan * Remove AddViteApp and npm-specific functionality (moved to Aspire 13) Co-authored-by: aaronpowell <[email protected]> * Update comments and tests to reflect npm removal Co-authored-by: aaronpowell <[email protected]> * Update MONOREPO.md to remove npm references Co-authored-by: aaronpowell <[email protected]> * Adding missing types * Bad copilot code * Fixing missing namespace * Handling port forwarding * removing more stuff that has been migrated to aspire core * Reworking how package installing is handled * daily update * Adding back the annotation that is no longer moving to core * Forgot to install packages * adding a bunch more annotations to provide enough metadata * Removing old demo apps * Disabling python tests * Removing a testing change * Updating to the latest nightly and dealing with type name changes * Removing type that was can repurpose from aspire core * breaking the tests down so they are easier to read * Removing legacy docs * Fixing failing tests --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aaronpowell <[email protected]> Co-authored-by: Aaron Powell <[email protected]>
|
@copilot the tests are for Hosting.Minio and Hosting.SurrealDb, with example logs https://github.com/CommunityToolkit/Aspire/actions/runs/19058298930/job/54432972325?pr=916 and https://github.com/CommunityToolkit/Aspire/actions/runs/19058298930/job/54432972317?pr=916 |
Co-authored-by: aaronpowell <[email protected]>
Co-authored-by: aaronpowell <[email protected]>
Fixed the failing tests. The issue was that Added implementations in commits:
All resources now properly implement the connection properties interface following the same patterns as the upstream Aspire resources. |
|
|
||
| builder.Append($";AccessKey={RootUser}"); | ||
| builder.Append($";SecretKey={PasswordParameter}"); | ||
| builder.Append($";SecretKey={PasswordParameter:uri}"); |
There was a problem hiding this comment.
This shouldn't be URL-encoded, right?
There was a problem hiding this comment.
My understanding from this thread #916 (comment) with @sebastienros was that passwords should be.
There was a problem hiding this comment.
This one should not be "URL" encoded because it's not an url. We are not encoding connection string segments for now.
aaronpowell
requested review from
FullStackChef,
WhitWaldo,
martinjt and
paule96
as code owners
| /// Gets the connection string expression for the Solr server. | ||
| /// </summary> | ||
| public ReferenceExpression ConnectionStringExpression => ReferenceExpression.Create( | ||
| $"http://{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}/solr/{CoreName}"); |
There was a problem hiding this comment.
Maybe but it would also depend on whether the client is aware that it's URL encoded and knows to perform a decode. Since we don't have a Solr client we can't control the expectation.
There was a problem hiding this comment.
What do you think a client would do with a URL? Split by '/' ? Also what if a client was actually reading the URL, this would break with any incompatible char then. This can always be changed later, not a big issue for now IMO
|
|
||
| IEnumerable<KeyValuePair<string, ReferenceExpression>> IResourceWithConnectionString.GetConnectionProperties() => | ||
| Parent.CombineProperties([ | ||
| new("Database", ReferenceExpression.Create($"{DatabaseName}")) |
There was a problem hiding this comment.
If the parent has a Uri then this resource should too. And a good pattern is to then have the parent expression expose a BuildUri(string? database name) method.
There was a problem hiding this comment.
Umm, I guess so? TBH I won't get that done this release. If you can create an issue and point it to the relevant changes in aspire I'll see if copilot can sort it out.
| private ReferenceExpression ConnectionString => | ||
| ReferenceExpression.Create( | ||
| $"Server={SchemeUri}://{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}/rpc;User={UserNameReference};Password='{PasswordParameter}'"); | ||
| $"Server={SchemeUri}://{PrimaryEndpoint.Property(EndpointProperty.Host)}:{PrimaryEndpoint.Property(EndpointProperty.Port)}/rpc;User={UserNameReference};Password={PasswordParameter}"); |
There was a problem hiding this comment.
I didn't think you are using the latest version of aspire because this wouldn't compile anymore. Check BuildUri in MySql resource.
|
In the latest commit the title mentions an encode is removed because the client doesn't support it. To be clear it was used in a connection string for the property Password which is not a URL, so this shouldn't be url-encoded independently of what the client would be or do. |
right - and I think this PR is now muddied in terms of what it needs to tackle and what would be good to tackle, so I'm going to strip it back and if we can get issues on the other stuff then I'll get that tackled elsewhere |
Minimum allowed line rate is |
aaronpowell
temporarily deployed
to
azure-artifacts
GitHub Actions
Inactive
Adapt connection string generation to support new formatting
This PR implements the new
GetConnectionProperties()method from the upstream Aspire PR #11938 for all CommunityToolkit resources that implementIResourceWithConnectionString.Progress
IResourceWithConnectionStringGetConnectionProperties()for each resource following the patterns from upstreamOriginal prompt
Fixes #889
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.