Fixing #1015539 #4628
Merged
Fixing #1015539 #4628
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…users/aydey/AzureRG/CustomScriptExtensionBugFixes
…licy while executing the powershell script on vm
sachinma
reviewed
Jun 27, 2017
| @@ -397,6 +397,11 @@ export class WinRMExtensionHelper { | |||
| reject(tl.loc("ARG_SetExtensionFailedForVm", this.resourceGroupName, vmName, result)); | |||
| return; | |||
| } | |||
| if (!this.ValidateExtensionExecutionStatus(vmName, dnsName, extensionName, location, _fileUris)) { | |||
| @@ -345,7 +345,7 @@ export class WinRMExtensionHelper { | |||
| if (result["properties"]["instanceView"] && result["properties"]["instanceView"]["extensions"]) { | |||
| var extensions = result["properties"]["instanceView"]["extensions"]; | |||
| for (var extension of extensions) { | |||
| if (result["name"] === extensionName) { | |||
| if (extension["name"] === extensionName) { | |||
| for (var substatus of extension["substatuses"]) { | |||
| if (substatus["code"] && substatus["code"].indexOf("ComponentStatus/StdErr") >= 0 && !!substatus["message"] && substatus["message"] != "") { | |||
| invalidExecutionStatus = true; | |||
There was a problem hiding this comment.
Consider returning success only if you find the matching extension
| return; | ||
| } | ||
| var validStatus = await this.ValidateExtensionExecutionStatus(vmName, dnsName, extensionName, location, _fileUris); | ||
| if (!validStatus) { | ||
| tl.debug("WinRMCustomScriptExtension is not valid on vm " + vmName); | ||
| reject(tl.loc("ARG_SetExtensionFailedForVm", this.resourceGroupName, vmName, result)); | ||
| reject(tl.loc("ARG_SetExtensionFailedForVm", this.resourceGroupName, vmName, JSON.stringify(result))); |
There was a problem hiding this comment.
Will it make sense here. I think we should attempt to print the stderr result message
…users/aydey/AzureRG/CustomScriptExtensionBugFixes
…users/aydey/AzureRG/CustomScriptExtensionBugFixes
…stomScript Extension fails
sachinma
requested changes
Jul 3, 2017
| } | ||
| } | ||
| try { | ||
| await this.ValidateExtensionExecutionStatus(vmName, dnsName, extensionName, location, _fileUris); |
There was a problem hiding this comment.
This might return success in some cases.
| if (result["properties"]["instanceView"] && result["properties"]["instanceView"]["extensions"]) { | ||
| var extensions = result["properties"]["instanceView"]["extensions"]; | ||
| for (var extension of extensions) { | ||
| if (result["name"] === extensionName) { | ||
| if (extension["name"] === extensionName) { | ||
| for (var substatus of extension["substatuses"]) { |
There was a problem hiding this comment.
if the extension is not found, you should fail with the message.
sachinma
requested changes
Jul 3, 2017
| @@ -345,7 +345,7 @@ | |||
| "SecurityRuleNameCannotBeNull": "securityRuleName cannot be null or undefined and it must be of type string.", | |||
| "SecurityRuleParametersCannotBeNull": "securityRuleParameters cannot be null or undefined.", | |||
| "OutputVariableShouldNotBeEmpty": "Output variable should not be empty.", | |||
| "ARG_SetExtensionFailedForVm": "Failed to set extension on Virtual Machine %s of Resource Group %s with result: %s", | |||
| "ARG_SetExtensionFailedForVm": "StdErr Message: Failed to set extension on Virtual Machine %s of Resource Group %s with error: %s", | |||
| @@ -321,49 +328,63 @@ export class WinRMExtensionHelper { | |||
|
|
|||
| private GetExtension(vmName: string, extensionName: string): Promise<any> { | |||
There was a problem hiding this comment.
Rename to getCustomScriptExtension and remove the extensionName param
| break; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if(!extensionPresent){ | ||
| reject("Extension not found on the vm: " + vmName); |
There was a problem hiding this comment.
If this can show without debug; then it needs to be in task.json
| @@ -388,13 +409,21 @@ export class WinRMExtensionHelper { | |||
| return new Promise<any>((resolve, reject) => { | |||
| this.computeClient.virtualMachineExtensions.createOrUpdate(this.resourceGroupName, vmName, az.ComputeResourceType.VirtualMachine, extensionName, parameters, async (error, result, request, response) => { | |||
| if (error) { | |||
| reject(tl.loc("CreationOfExtensionFailed", utils.getError(error))); | |||
There was a problem hiding this comment.
Check if you should add the VM name to the message.
sachinma
approved these changes
Jul 3, 2017
bryanmacfarlane
deleted the
users/aydey/AzureRG/CustomScriptExtensionBugFixes
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.