Dandua98/cosmos arm #116
Dandua98/cosmos arm #116
Conversation
dandua98
requested review from
dgomezc,
KirillShchetinin,
markAtMicrosoft,
mohannad-abwah,
mvegaca,
sibille,
smmatte and
tejbirsingh
as code owners
dandua98
requested review from
amrrsharaff,
ngkelly3,
Kai-Bailey,
trevorNgo and
SahilTara
| userSubscriptionItem.subscriptionId === undefined | ||
| ) { | ||
| throw new SubscriptionError( | ||
| "SubscriptionItem cannot have undefined values" |
There was a problem hiding this comment.
Fix to move this to constants is in next PR
| // See http://go.microsoft.com/fwlink/?LinkId=827846 |
There was a problem hiding this comment.
These are by VSCode, not us
| ): ResourceManagementClient { | ||
| let userCredentials: ServiceClientCredentials = | ||
| userSubscriptionItem.session.credentials; | ||
| if ( |
There was a problem hiding this comment.
That is private method - you don't need to validate input. Instead you need to make sure that this method is called with correct parameters that are not null.
please remove line 27. And possibly entire "if" depends on if you explicitly want containment of the userSubscriptionItem.subscription(Id) to be validated in this method.
There was a problem hiding this comment.
Fix for this is in next PR (#117) which uses this module too. I extended some functionality there so would rather address it there
|
|
||
| public getResourceManagementClient( | ||
| userSubscriptionItem: SubscriptionItem | ||
| ): ResourceManagementClient { |
There was a problem hiding this comment.
And this one is public - check for null in the input parameters. If userSubscriptionItem is null -> don't call this.SetClientState. Instead either throw or whatever is expected result.
There was a problem hiding this comment.
Okey for now, but please keep in mind for the future that not everything can be carried to the future PR's. "Missing null-check" is buggy enough to be fixed within the same PR. State of your main branch can't be broken between PR's because you decided to fix your previous PR in the next PR.
There was a problem hiding this comment.
It's not a missing null-check. It can't be null anyway (can only be undefined) and we're checking for that instead
| { | ||
| "apiVersion": "2015-04-08", | ||
| "kind": "[parameters('kind')]", | ||
| "type": "Microsoft.DocumentDb/databaseAccounts", |
There was a problem hiding this comment.
Shouldn't this be Microsoft.DocumentDB/databaseAccounts?
There was a problem hiding this comment.
Nope, this is not decided by me. It's the parameters Azure accepts
There was a problem hiding this comment.
https://docs.microsoft.com/en-us/azure/templates/Microsoft.DocumentDB/2015-04-08/databaseAccounts
Please fix this in the next PR since I realize you have conflicts.
| }; | ||
|
|
||
| try { | ||
| if (this.SubscriptionItemCosmosClient === undefined) { | ||
| throw new AuthorizationError("Cosmos Client cannot be undefined."); | ||
| } | ||
|
|
||
| let azureResourceClient: ResourceManagementClient = new ResourceManager().getResourceManagementClient( | ||
| userSubscriptionItem |
There was a problem hiding this comment.
I mean "userSubscriptionItem"
There was a problem hiding this comment.
It's a parameter to the, I just modified what gets done in this function (modifying how cosmos is deployed internally) and not function and class itself
WARNING: Don't review this PR until #115 is closed. This will reduce the number of changes shown
This PR introduces ARM templates for Cosmos and closes AB#24106
Testing Notes:
./buildfollowed by F5 in extension dir to run the extension) and deploy a cosmos resource through the wizardNotes: