[Identity] Samples aligned with other languages (v1 compatible) #15031
Conversation
ghost
added
the
sadasant
changed the title
|
@maorleger Can you please help in reviewing this? |
This comment has been minimized.
This comment has been minimized.
| In this example, `StaticTokenCredential` implements the `TokenCredential` abstraction. It takes a pre-fetched access token in its constructor as an `AccessToken` (defined on `@azure/core-http`), and simply returns that from its implementation of `getToken()`. | ||
|
|
||
| ```ts | ||
| import { TokenCredential, AccessToken } from "@azure/core-http"; |
There was a problem hiding this comment.
We have been very intentional when showing samples to use JavaScript rather than TypeScript. We have the TS samples only in our samples folder where we can afford to keep both TS & JS samples. Can we do the same here?
There was a problem hiding this comment.
For this PR I’d like to keep this in typescript, as it will mean double the files, and if we want to cater specifically to either audience, it may make sense to give more context on what typescript dependencies to install, for example.
There was a problem hiding this comment.
Well, let’s do this: I can add the two documents as part of this PR, but let’s get one of them approved first (the typescript one, which will be more complex), then I’ll do the JS one.
There was a problem hiding this comment.
On second thought, two documents won’t work well with the samples v2 approach, since these won’t be auto-generated. There must be a clean way to do this, but we also have an issue to migrate to samples v2. I believe we can have one file for now and improve over time.
Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
|
@ramya-rao-a I reviewed it and updated it! I read it carefully. Some things are missing, like the accommodations for a proper TypeScript project. Still, it seems that this file is generally about providing an idea of how to move forward rather than giving prescriptive solutions for our users. My intention with this PR is to make these specific samples easy to follow. We still have to add many other cases based on the rest of the features that we’ll be releasing. In parallel, we’ll be figuring out how to make the Identity samples play well with the Samples V2 architecture. I think there’s a lot of work pending that will touch the sample files, so we’ll have many opportunities to revisit this. |
Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
|
@scottaddie hello, Scott! There’s one answer to one of your feedback entries here: #15031 (comment) It’s hidden from plain view because of all of the collapsed messages. Let me know how it goes! Your time is appreciated. |
|
|
||
| If the application gets notified of certificate rotations and can directly respond, it might choose to wrap the `ClientCertificateCredential` in a custom credential which provides a means for rotating the certificate. | ||
|
|
||
| > You'll need to install the [@azure/core-auth][core_auth] package for this sample. |
There was a problem hiding this comment.
What is needed from the perspective of somebody new to TypeScript? We do have some of that information in our main Identity readme, should we have something similar here?
You could list TypeScript as a prerequisite and link to https://www.typescriptlang.org/download. That information is probably better off in the main README. No need to duplicate the content then.
What I meant by adding a Prerequisites section was to add a heading before something like the following:
> You'll need to install the [@azure/core-auth][core_auth] package for this sample.
This example would become:
**Prerequisites**
Install the [@azure/core-auth][core_auth] package.
Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
|
@scottaddie regarding the pre-requisites section, I did this: 69c56ab let me know how it looks? |
Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>
|
Hello @sadasant! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit Fix swagger correctness for appplatform 2021-06-01-preview (Azure#15031)
In this PR we're aligning the samples of Identity with other languages. The samples I'm adding in this PR are:
@azure/keyvault-certificatesbecause of the possible issues converting certificates from one format to another in Node).New samples related to the v2-specific features will be added after the Identity package is separated.
Related to #14435
I’ve decided to use this opportunity to:
Fix #15324
This PR also implicitly:
Fixes #15573