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

adding TokenCredential as part of the credential type accepted #846

Merged
merged 24 commits into from
Feb 2, 2021

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Jan 21, 2021

This PR adds support to v4 to accept @azure/identity's TokenCredential. Most of the work for that support is already in ms-rest-js v2.2.0.

Things to note:

  • we need typescript v3.6 to be able to work with abort-controller type definitions, a dependency of core-auth, which in turn is the home of TokenCredential.
  • versions of ms-rest-js and ms-rest-azure-js are bumped to bring in the support for TokenCredential in them.

Todo:

@@ -588,7 +591,8 @@ public virtual void ConstructRuntimeImportForModelIndex(TSBuilder builder)

public virtual void PackageDependencies(JSONObject dependencies)
{
dependencies.StringProperty("@azure/ms-rest-js", "^2.0.4");
dependencies.StringProperty("@azure/ms-rest-js", "^2.2.0");
dependencies.StringProperty("@azure/core-auth", "^1.1.4");
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this dependency should not be added unless there is a credential, right? Is there a simple way to check for this condition here? cc @joheredi

Copy link
Member

Choose a reason for hiding this comment

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

I think you could check either Settings.AddCredentials or Settings.AzureArm

@deyaaeldeen deyaaeldeen marked this pull request as ready for review January 28, 2021 14:42
Copy link
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for taking on this work, this is going to make things smoother for ARM users

@ramya-rao-a
Copy link
Contributor

Are the changes from Azure/azure-sdk-for-js#13403 going to be part of this PR or a separate one?
Where can I see a diff of the resulting changes?

@deyaaeldeen
Copy link
Member Author

@ramya-rao-a I will address feedback and generate the updated README.md in this PR. I will mention you in the diffs after I finish.

@@ -1,88 +1,83 @@
## An isomorphic javascript sdk for - AutoRestComplexTestService

This package contains an isomorphic SDK for AutoRestComplexTestService.
This package contains an isomorphic SDK (runs both in node.js and in browsers) for AutoRestComplexTestService.
Copy link
Member Author

Choose a reason for hiding this comment

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

@ramya-rao-a , here is an updated readme file. @joheredi how can I get a display name for a package? I currently just use a normalized package name.

@deyaaeldeen deyaaeldeen merged commit 277f301 into Azure:v4x Feb 2, 2021
@deyaaeldeen deyaaeldeen deleted the identity-adapter branch February 2, 2021 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants