Migrate to new Azure SDK packages #197

RMacfarlane · 2020-06-23T00:43:29Z

Migrate to using the new Azure SDK packages internally, but continue to expose the old credentials object in the API for backcompatability. Introduces a new "credentialsV2" property corresponding to the new credentials object

chrmarti

Change looks good. When I try to sign in I get the success page in the browser, but the VS Code statusbar and the App Services view remain empty. (This is on Mac.)

chrmarti · 2020-06-23T13:44:21Z

src/azure-account.api.d.ts

 	readonly userId: string;
 	readonly tenantId: string;
 	readonly credentials: ServiceClientCredentials;
+	readonly credentialsV2: DeviceTokenCredentials;


Is there a more general interface like ServiceClientCredentials in the old SDK? 'Device' refers to the device login flow.

Changed to TokenCredentialsBase, which seems to be the most general interface for the new SDK

chrmarti · 2020-06-23T13:45:43Z

src/cloudConsole.ts

@@ -380,7 +380,7 @@ export function createCloudConsole(api: AzureAccount, reporter: TelemetryReporte
 		// Additional tokens
 		const [graphToken, keyVaultToken] = await Promise.all([
 			tokenFromRefreshToken(session.environment, result.token.refreshToken, session.tenantId, session.environment.activeDirectoryGraphResourceId),
-			tokenFromRefreshToken(session.environment, result.token.refreshToken, session.tenantId, `https://${session.environment.keyVaultDnsSuffix.substr(1)}`)
+			tokenFromRefreshToken(session.environment, result.token.refreshToken, session.tenantId, `https://${session.environment.keyVaultDnsSuffix!.substr(1)}`)


I wonder if the is a good fallback for when keyVaultDnsSuffix is not available. Maybe CloudShell works without a KeyVault token? (It used to.)

Yeah, I just tried without a KeyVault token and it seemed to work fine without it. I'll change this to make the keyVault token optional

chrmarti · 2020-06-23T14:12:11Z

@RMacfarlane Actually everything works when running out-of-source, only when installing the packaged VSIX it somehow didn't. Can you give that a try?

RMacfarlane · 2020-06-23T19:40:49Z

Was it sign in that didn't work? I've just tried packaging and running it as a VSIX and have been able to sign in and open a cloud console.

Edit: Ah, you answered that above. I'll self host with it for the rest of today and see if I can repro

chrmarti · 2020-06-24T07:31:03Z

I see the following in the console when running with the VSIX, but not when running with yarn watch. Do you see that? Maybe the minifier somehow corrupts things.

[Extension Host] TypeError: Expected signal to be an instanceof AbortSignal
	at new B (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:12277)
	at /Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:14625
	at new Promise (<anonymous>)
	at G (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:14589)
	at t.<anonymous> (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:76748)
	at /Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:1768
	at Object.next (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:1873)
	at /Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:811
	at new Promise (<anonymous>)
	at a (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:548)
	at t.fetch (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:76672)
	at t.<anonymous> (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:364:74931)
	at /Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:1768
	at Object.next (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:1873)
	at a (/Users/chrmarti/Development/repos/vscode-azure-account/dist/extension.js:16:604)
	at processTicksAndRejections (internal/process/task_queues.js:93:5)

chrmarti · 2020-06-24T07:42:57Z

This appears to be node-fetch/node-fetch#784. Just not throwing the error by manually removing it makes it work. The solution on that issue was to not minify class names. How did you build the VSIX avoiding this issue?

RMacfarlane · 2020-06-24T15:53:32Z

Hmm, I'm just building with vsce package, but I still don't see that in the console

chrmarti · 2020-06-25T08:54:10Z

What do you get for regex searching for class.*EventTarget in dist/extension.js after compiling with npm run vscode:prepublish? My first match is the minified AbortSignal class:

class i extends r.EventTarget{constructor(){throw super(),new TypeError("AbortSignal cannot be constructed directly")} ...

Its name has been minified to i.

Searching for "AbortSignal" finds me the check that no longer works:

"AbortSignal"!==t.constructor.name)}(a))throw new TypeError("Expected signal to be an instanceof AbortSignal");

Do you have these too? I'd expect so. Maybe you just don't run through this code for some reason?

RMacfarlane · 2020-06-25T17:42:03Z

Ah, thanks, I do see the same minification in the output, so I must just not be hitting that code path for some reason.

I pushed a change to turn off minification, which unfortunately doubles the size of the .js file to 4MB, but retains the class name.

Preserve AbortSignal class name

RMacfarlane added 5 commits June 22, 2020 11:34

Expose new credentials object in API

b24e5b9

Use 'Environment' instead of 'AzureEnvironment'

81bd402

Migrate from 'azure-arm-resource' to new packages

78c0db9

Update third party notices

74c605f

Remove ms-rest-azure usage from tenantClientDetails

9e57b0b

vscodebot bot assigned chrisdias and RMacfarlane Jun 23, 2020

RMacfarlane requested a review from chrmarti June 23, 2020 00:43

chrmarti reviewed Jun 23, 2020

View reviewed changes

chrmarti self-requested a review June 23, 2020 14:09

Address feedback

a34fc0e

Turn of minification

5cde3c2

chrmarti and others added 2 commits June 25, 2020 21:07

Preserve AbortSignal class name

31595ff

Merge pull request #198 from microsoft/chrmarti/sdkUpdates

dad9462

Preserve AbortSignal class name

chrmarti approved these changes Jun 26, 2020

View reviewed changes

RMacfarlane merged commit 0fa0989 into master Jun 26, 2020

RMacfarlane deleted the rmacfarlane/sdkUpdates branch June 26, 2020 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to new Azure SDK packages #197

Migrate to new Azure SDK packages #197

RMacfarlane commented Jun 23, 2020 •

edited

Loading

chrmarti left a comment

chrmarti Jun 23, 2020

RMacfarlane Jun 23, 2020

chrmarti Jun 23, 2020

RMacfarlane Jun 23, 2020

chrmarti commented Jun 23, 2020

RMacfarlane commented Jun 23, 2020 •

edited

Loading

chrmarti commented Jun 24, 2020

chrmarti commented Jun 24, 2020

RMacfarlane commented Jun 24, 2020

chrmarti commented Jun 25, 2020

RMacfarlane commented Jun 25, 2020

Migrate to new Azure SDK packages #197

Migrate to new Azure SDK packages #197

Conversation

RMacfarlane commented Jun 23, 2020 • edited Loading

chrmarti left a comment

Choose a reason for hiding this comment

chrmarti Jun 23, 2020

Choose a reason for hiding this comment

RMacfarlane Jun 23, 2020

Choose a reason for hiding this comment

chrmarti Jun 23, 2020

Choose a reason for hiding this comment

RMacfarlane Jun 23, 2020

Choose a reason for hiding this comment

chrmarti commented Jun 23, 2020

RMacfarlane commented Jun 23, 2020 • edited Loading

chrmarti commented Jun 24, 2020

chrmarti commented Jun 24, 2020

RMacfarlane commented Jun 24, 2020

chrmarti commented Jun 25, 2020

RMacfarlane commented Jun 25, 2020

RMacfarlane commented Jun 23, 2020 •

edited

Loading

RMacfarlane commented Jun 23, 2020 •

edited

Loading