Skip to content
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

Event Hubs SDK: Adding connection string support for AAD Managed Identity #6287

Merged
merged 11 commits into from
Jun 1, 2019

Conversation

serkantkaraca
Copy link
Member

  • Adding connection string support for AAD Managed Identity
  • Removing dependency on ADAL

@serkantkaraca serkantkaraca changed the title Adding connection string support for AAD Managed Identity Azure Event Hubs SDK: Adding connection string support for AAD Managed Identity May 15, 2019
@serkantkaraca serkantkaraca changed the title Azure Event Hubs SDK: Adding connection string support for AAD Managed Identity Event Hubs SDK: Adding connection string support for AAD Managed Identity May 15, 2019
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. Event Hubs labels May 15, 2019
@jsquire jsquire requested a review from schaabs May 15, 2019 16:22
@bainian12345
Copy link
Contributor

        var tenantId = "";

Doesn't look like this is needed


Refers to: sdk/eventhub/Microsoft.Azure.EventHubs/tests/Client/TokenProviderTests.cs:117 in 20420b8. [](commit_id = 20420b8, deletion_comment = False)

@jsquire jsquire self-requested a review May 22, 2019 22:34
Copy link
Member

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3 the use of the ITokenProvider! I approve.

@schaabs, do you have any guidance to impart for future Azure.Identity integration?

@schaabs
Copy link
Member

schaabs commented May 30, 2019

We discussed this last week. The callback pattern that they are using here aligns well with future plans for azure identity.

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a couple of minor suggestions that I left, but otherwise, things look good to me.

/// <param name="audience">The service resource URI for token acquisition.</param>
/// <param name="authority">Address of the authority to issue token.</param>
/// <param name="state">State to be delivered to callback.</param>
/// <returns></returns>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If this won't be populated, better to remove it to reduce noise.

@@ -62,7 +63,7 @@ public static EventHubClient CreateFromConnectionString(string connectionString)
/// <param name="operationTimeout">Operation timeout for Event Hubs operations.</param>
/// <param name="transportType">Transport type on connection.</param>
/// <returns></returns>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If this won't be populated, better to remove it to reduce noise.

@jsquire jsquire merged commit a1f8705 into Azure:master Jun 1, 2019
@serkantkaraca serkantkaraca deleted the ConnectionStringMsiSupport branch August 12, 2020 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants