-
Notifications
You must be signed in to change notification settings - Fork 70
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
Management Plane API Switch #1121
Conversation
@@ -3,7 +3,7 @@ | |||
* Licensed under the MIT License. See License.txt in the project root for license information. | |||
*--------------------------------------------------------------------------------------------*/ | |||
|
|||
import { DatabaseAccount } from 'azure-arm-cosmosdb/lib/models'; | |||
import { DatabaseAccount } from '@azure/arm-cosmosdb/esm/models'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch these to use the Models
object from the base of the npm package, which is more of an npm package's "official" api. Something like this
import { ResourceManagementModels } from '@azure/arm-resources';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a newer preference? This has been how it's used everywhere.
For eg. appservice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it's not super new, but yeah it changed sometime after we originally created the extensions. For example the original app service extension referenced v1.0.0-preview of "azure-arm-website" which didn't even have the models object:
Now we're on a much later version that does:
Functions is already moved over: https://github.com/microsoft/vscode-azurefunctions/blob/795ab870b56e133e2c360b3bf08df9419b792d4a/src/tree/FunctionsTreeItem.ts#L6
Docker is moved over: https://github.com/microsoft/vscode-docker/blob/2fae7a525c41f6f8375f51fc6a3598e2d05e5ca4/src/tree/registries/azure/AzureTaskRunTreeItem.ts#L6
There's a tslint rule here that we can eventually turn on: https://palantir.github.io/tslint/rules/no-submodule-imports/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up.
I turned on the rule, it looks like there are other places where the rule is broken. Leaving those at the moment.
@@ -905,22 +905,25 @@ | |||
"webpack-cli": "^3.1.2" | |||
}, | |||
"dependencies": { | |||
"@azure/arm-cosmosdb": "^3.0.0", | |||
"@azure/ms-rest-azure-js": "^1.3.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the node modules that you're not using
package.json
Outdated
"node-uuid": "1.4.8", | ||
"socket.io": "^1.7.3", | ||
"socket.io-client": "^1.7.3", | ||
"underscore": "^1.8.3", | ||
"vscode-azureextensionui": "^0.24.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of date - merge with master
azure-arm-xyz to @azure/arm-xyz
Fixes #1069
Builds will fail till the corresponding PR in azuretools is merged and published: microsoft/vscode-azuretools#511