Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow specifying an Event Hub to use as the default EntityPath for namespace connection string #7105
base: main
Are you sure you want to change the base?
Allow specifying an Event Hub to use as the default EntityPath for namespace connection string #7105
Changes from 17 commits
50ee437
d398cf3
e236685
dd3146f
1f84b3a
0484621
6c0e11c
c5a98fa
dd17cba
f438fd8
82c7622
c7069f8
6ff7a82
8155a3b
bdccf10
4f08582
af0ac0c
a2249a8
5098705
c37c109
6a9c272
d23a81d
ff66528
3235d54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 there a reason we'd want to use this without having WithHub? Example: connecting to an existing azure resource without the need to call
WithHub
for each of the existing ones.After all
WithHub("foo")
doesn't ensure it actually exists on the resource either. Almost like we are asking users twice in that case.And related to this, as a user it might be nice to be able to call it twice, the second being one winning. That would mean store the hub name in the resource itself, instead of having a bool on each resource to verify the integrity (might be simpler too).
Is "Default" in
DefaultEntity
something concrete? Why not calledWithEntityPath()
? (ignore me if I already asked ;) )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.
When you mentioned this first, I had to think about it and I don't believe there's a scenario where we'd want to set a default entity path without having any associated hubs in the resource. If we want to use a pre-existing resource that already has hubs, we would use AddConnectionString. Anything that uses AddAzureEventHubs needs to provide at least one hub, or health checks will fail. I understand it is possible to want to create a namespace without hubs and have hubs created dynamically (i.e. some dapr patterns encourage dthis but you have to grant it management permissions.)
The resource itself doesn't neccessarily exist either -- all this does is mutate the model. At runtime, it's either projected into the emulator config, or emitted in the manifest. I don't think this is our concern?
I'm not sure how this scenario ("nice to be able to call it twice") would arise beyond a mistake?
I say "default" because you can override it in the client settings.
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.
@sebastienros -- you're right -- it does make sense to allow someone to call it multiple times with different hubs, with last one winning. I've had a head cold all week and for some reason I didn't quite get it, lol. I'll fix that now.
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 renaming
WithDefaultEntity
toWithEntityPath
since this now opens up:@davidfowl @eerhardt ^ ?
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.
Although ... this approach could be troublesome due to it mutating the parent. Example:
So here, both get pointed at hub2. Oops. What are your thoughts? I like that With vs Add has well defined semantics. Is it clear enough though to dissuade people doing things like this? Maybe this isn't the only API that could fall foul to this, so it's an accepted tradeoff?
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.
simply put, if you use
https://myeventhub.servicebus.windows.net:443/?EntityPath=hub&ConsumerGroup=foo
as a format, you won't break pure azure sdk, and you can still deconstruct into settings.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.
Urgh -- I'm getting embrace, extend, extinguish vibes here but Microsoft own both sides of the contract, so maybe it's not so bad lol. On a completely different tangent -- if we're prepared to accept Aspire lock-in on both sides of the contract, why not make it a choice and have resources implement an additional "enhanced" binding interface, e.g. IResourceWithConnectionProperties and leave IResourceWithConnectionString alone? The latter would be a downlevel binding for using the Azure SDK natively, or for components that don't require out of band properties. The former could do whatever the hell it likes. /cc @davidfowl @eerhardt @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.
You can override it in the settings callback. For configuration, the "client integrations" rules of order are:
ConnectionStrings:<name>
So if the info is in both config and ConnectionsStrings, ConnectionStrings wins.
It was already incompatible with the pure Azure SDK client. You can't just pass a URL string (with no shared keys) into the "connectionString" parameter of the client ctor. You already had to check if it wasn't a connection string and pass it into the "fullyQualifiedNamespace" parameter (and fill out "credential") and not the "connectionString" parameter.
It isn't technically a breaking change because in 9.0 you could never
.WithReference
on the child EventHub. You could only.WithReference
on the top-level EH namespace.This doesn't work for the local emulator. You'd still need to parse out ConsumerGroup of the local emulator connection string. The thinking is to "go all in" on the connection string format instead of one-off formats for each azure service. We already build up our own connection strings for things like Azure OpenAI and Search, as well as Milvus, Qdrant, Elastic. A similar change was made for CosmosDB's Database and Containers. So if we need to encode multiple pieces of information into a single string, we prefer to put it in a connection string format.
I agree this isn't perfect. I've been pushing for Add support for Connection Properties (dotnet/aspire#1765), but we haven't prioritized it. So single strings is what we have today.
My hope is that in the future the Azure SDK supports IConfiguration natively and we can just bind to a configuration section where the Azure SDK loads whatever information is necessary to make the connection.
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 very aware of this - this entire PR that is being abandoned implements all this functionality for both SAS connectionstrings and FQNS/tokencredential.
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 developer experience is going to be really nice when we're done here. We're also discussing exposing an api to parse and reason about connection info on the application side of things.