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

[Storage] Drop RawTokenCredential from storage packages #4788

Merged
merged 10 commits into from
Sep 6, 2019

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Aug 18, 2019

Issue - #4636
@azure/core-http now provides a SimpleTokenCredential, RawTokenCredential can be dropped from storage-packages(blob and queue).

Update -
Removing RawTokenCredential completely.

@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Aug 18, 2019
@HarshaNalluru HarshaNalluru self-assigned this Aug 18, 2019
Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Please re-export SimpleTokenCredential in index.ts and index.browser.ts. Because storage SDK users should not take @core/core-http as direct dependency.

@@ -167,13 +167,6 @@ async function main() {
// SharedKeyCredential is only avaiable in Node.js runtime, not in browsers
const sharedKeyCredential = new SharedKeyCredential(account, accountKey);

// Use RawTokenCredential with OAuth token. You can find more
Copy link
Member

@XiaoningLiu XiaoningLiu Aug 19, 2019

Choose a reason for hiding this comment

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

We should leave a sample here for customers who want OAuth authentication.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe linking to the @azure/identity npmjs page. There's some documentation there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Should make a more clearer description in sample codes and code comments.

SimpleTokenCredential is from @azure/core-http while other TokenCredential implmenetations from @azure/identity. We'd better make the description clearer.

@@ -167,11 +167,11 @@ async function main() {
// SharedKeyCredential is only avaiable in Node.js runtime, not in browsers
const sharedKeyCredential = new SharedKeyCredential(account, accountKey);

// Use RawTokenCredential with OAuth token. You can find more
// Use SimpleTokenCredential with OAuth token. You can find more
// TokenCredential implementations in the @azure/identity library
Copy link
Member

Choose a reason for hiding this comment

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

About description "from @azure/identity".

SimpleTokenCredential is from @azure/core-http while other TokenCredential implmenetations from @azure/identity. We'd better make the description clearer.

Copy link
Member

Choose a reason for hiding this comment

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

This should be taking TokenCredential not SimpleTokenCredential

Copy link
Member

Choose a reason for hiding this comment

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

I opened issue #4968 to remove SimpleTokenCredential from core-auth

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. SimpleTokenCredential should be removed from core-auth and won't be exposed in our libraries.

@XiaoningLiu, you've recently added some tests that leverage SimpleTokenCredential a few days ago.
Should we implement this SimpleTokenCredential class on top of TokenCredential in the test/utils/index.ts to leverage it just for the tests and not expose it anywhere else from the library?

Copy link
Member

Choose a reason for hiding this comment

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

@HarshaNalluru Sounds good. One question about removing SimpleTokenCredential. This is the raw or last way for customers to create a TokenCredential. Does @azure/identity work with Node.js & browsers to get a TokenCredential?

Copy link
Member Author

@HarshaNalluru HarshaNalluru Sep 7, 2019

Choose a reason for hiding this comment

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

@azure/identity provides many types of TokenCredentials for both Node.js & browsers.
Created and issue to test and to add samples - #5051

@ramya-rao-a
Copy link
Contributor

I was not aware that we advocated users to use the older RawTokenCredential. Bringing @bterlson, @jonathandturner and @schaabs into this discussion for awareness. Are we ok with this from a guideline perspective and identity perspective?

@HarshaNalluru HarshaNalluru changed the title [Storage] Drop RawTokenCredential in favour of SimpleTokenCredential [Storage] Drop RawTokenCredential from storage packages Sep 6, 2019
@ramya-rao-a ramya-rao-a removed the request for review from daviwil September 6, 2019 19:20
Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

Looks good. One comment

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. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants