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
ServiceBus SDK: Rbac support #6393
ServiceBus SDK: Rbac support #6393
Changes from 1 commit
885eee2
2b52261
74f1416
f8d3f5f
ecdb825
47c385e
9579185
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.
If you're moving forward with the rename, you'll probably want to consider updating this comment as well.
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.
Do we need this rename? If we rename the public method then it becomes a breaking change.
If there's enough justification for the rename, lets update the major version in the csproj. #Closed
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.
Respectfully, I'm not sure that these comments offer much context or clarity for users who are unfamiliar. You may want to consider revising them to be more explicit or, if not, just removing them.
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 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 also accept
connectionStringBuilder
. So lets not duplicate the APIsIn reply to: 286696363 [](ancestors = 286696363)
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 curious as to the purpose of not just documenting that the consumer should create and use an
CreateAzureActiveDirectoryTokenProvider
orCreateAzureActiveDirectoryTokenProvider
and pass them to theITokenProvider
overload. Is there a benefit to consumers that I'm overlooking?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 want to block on this, but I'm still curious as to the answer. I feel that I'm missing some important context...
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.
It's removed and will no longer part of this change. It was an effort to keep consistency with the EventHubs SDK but it seems like there are good reasons to avoid these redundant APIs.
In reply to: 292678097 [](ancestors = 292678097)
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.
Even if this API is needed, this should not be static. This SDK has slightly different API syntax compared to the older one. Here you should be able to do a
new QueueClient()
, and notQueueClient.CreateWith*
#ClosedThere 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.
If we do constructor, the signature will collide with the one below. Eventhubs is also using Create() methods
In reply to: 286696743 [](ancestors = 286696743)
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.
Same as above #Closed
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
nit: convert to a
const string
#ClosedThere 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 it supposed to be *!*string.Equals?? #Closed
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.
Curious as to why this is a string? As compared to other options of - Enum or bool #Closed
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'd suggest the enum approach, personally. Asking consumers to pass a magic string seems like it would offer a challenge to discovering usable values via intellisense.
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.
In the case that you're set on a string, maybe consider adding a pseudo-enumeration to allow discovery and help prevent typos, then? Something like:
That also gives you the advantage of being able to throw doc comments on the type and member to guide the user experience.
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 it expected to extend methods of authentication in the near future?
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.
This is another area that I've not seen a response and have concerns about user experience. I don't want to block on it, as it is consistent with the direction that the Event Hubs client went, but I would love to see some thoughts after usability testing.
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.
nit: Instead of calling during ToString(), you could just do validation within the setter of each of the property..
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.
Given that you have validation within the setter, you don't need this method anymore.
In reply to: 286699955 [](ancestors = 286699955)