Upgrade Resource & Storage SDK to T2 #1050
Conversation
|
Changed quite a bit so I won't honor these previous approvals 😅 Relies on this PR https://github.com/microsoft/vscode-azure-account/pull/418/files |
| * @param options - The options used to configure any requests this | ||
| * TokenCredential implementation might make. | ||
| */ | ||
| getToken(scopes?: string | string[], options?: any): Promise<any | null>; |
There was a problem hiding this comment.
Is there a reason this returns Promise<any | null> instead of Promise<TokenResponse | AccessToken | null>? Even though we do the same thing for signRequest above and return Promise<any>
There was a problem hiding this comment.
It's to keep it more loose so that other versions of credentials could be accepted. For example, BasicAuthenticationCredentials returns WebResourceLike whereas the ServiceClientCredentials returns WebResource. We originally did that for signRequest because different versions of "@azure/ms-rest-js" would change the credential interface. I figured the same could happen with getToken.
Here's an example with the typing screwing up here: Azure/azure-sdk-for-js#10045
There was a problem hiding this comment.
Is it possible that version of signRequest that returns WebResource is older? It looks like the current version returns WebResourceLike: https://github.com/Azure/ms-rest-js/blob/f6f0d92d79a1dfa8d92ff0891b88bc6b7a349e69/lib/credentials/serviceClientCredentials.ts
ui/src/azure-account.api.d.ts
Outdated
|
|
||
| export type CloudShellStatus = 'Connecting' | 'Connected' | 'Disconnected'; | ||
|
|
||
| export interface UploadOptions { | ||
| contentLength?: number; | ||
| progress?: Progress<{ message?: string; increment?: number }>; | ||
| token?: CancellationToken; | ||
| } | ||
|
|
||
| export interface CloudShell { | ||
| readonly status: CloudShellStatus; | ||
| readonly onStatusChanged: Event<CloudShellStatus>; | ||
| readonly waitForConnection: () => Promise<boolean>; | ||
| readonly terminal: Promise<Terminal>; | ||
| readonly session: Promise<AzureSession>; | ||
| readonly uploadFile: (filename: string, stream: ReadStream, options?: UploadOptions) => Promise<void>; | ||
| } |
There was a problem hiding this comment.
I vote we remove these lines (and the CloudShell related line in AzureAccountExtensionApi above) since they aren't used right now in the UI package
There was a problem hiding this comment.
Ah, sure. I just copied the new api from the Azure Account extension because I didn't really want to mess with it 😅
There was a problem hiding this comment.
I don't feel strongly, but unless these are reexported from vscode-azureextensionui, I'd just leave them...
ui/index.d.ts
Outdated
| @@ -1409,7 +1420,8 @@ export interface IMinimumServiceClientOptions { | |||
| requestPolicyFactories?: any[] | ((defaultRequestPolicyFactories: any[]) => (void | any[])); | |||
| } | |||
|
|
|||
| export type AzExtGenericClientInfo = AzExtServiceClientCredentials | { credentials: AzExtServiceClientCredentials; environment: Environment; } | undefined; | |||
| export type AzExtGenericCredentials = BasicAuthenticationCredentials | TokenCredentials | AzExtServiceClientCredentials; | |||
There was a problem hiding this comment.
I'm confused why BasicAuthenticationCredentials and TokenCredentials are introduced here. They implement the ServiceClientCredentials interface and that interface just gives us signRequest which is already included in the AzExtServiceClientCredentials type.
There was a problem hiding this comment.
Because we have two different createClient helper functions, createGenericClient and createAzureClient.
Originally the azureCredentials were similar enough to basic credential types, but since we're introducing getToken as a requirement to create azure clients to ensure T1&T2 compatability, it would force those requirements on any generic client we create
For example, we used to make a GitHub client with createGenericClient but it wouldn't have required getToken.
I'm not sure if that 100% made sense so feel free to ping me and I can explain it better 😓
Co-authored-by: Brandon Waterloo [MSFT] <[email protected]>
A couple of changes to implement this:
list()now uses anPagedAsyncIterableIteratorinstead ofPromise<IPartialList<T>. This means I can't just do anawait list()anymore, which is why I have the helper function to help load all of the resources. Might be worth to do aloadByPage(numOfResources)helper function later too.coreAuth.TokenCredential. This usesgetTokenrather thansignRequestso I made a type compatible with both socreateAzureClientcould be used with both T1 and T2ManagementClients.Modelsnow to import individual models rather than the entire Model namespace.Tested this with
Functions Create (Advanced)and Container Apps. It still lists and creates resource groups and storage accounts, so I think everything is working? Container Apps uses the new T2 appservice package and those seem to be working as well, meaning the credentials are compatible with T1 and T2 azure sdk packages.