Azure App Service Manage Task - Refactor & Bug fixes #6001

vincent1173 · 2017-12-10T18:23:29Z

Issues Fixed:

Get Kudu credentials from Publishing credentials instead of publishing profile.
Invoke web-request for slots for Start/Stop/Restart/Swap to avoid Cold-start.

sachinma · 2017-12-11T05:22:24Z

Tasks/AzureAppServiceManage/make.json

-                "node_modules/azurestack-common/node_modules/vsts-task-lib",
-                "node_modules/azurestack-common/node_modules/vso-node-api"
+                "node_modules/azure-arm-rest/node_modules/vsts-task-lib",
+                "node_modules/azure-arm-rest/node_modules/vso-node-api"
            ],


I think now there is no vso-node-api.

sachinma · 2017-12-11T05:25:08Z

Tasks/Common/azure-arm-rest/azure-arm-resource-filter.ts

+
+export class AzureResourceFilterManagementClient extends azureServiceClient.ServiceClient {
+    public _azureResourceFilter: AzureResourceFilter;
+


Should public members not start with _

sachinma · 2017-12-11T05:44:30Z

Tasks/Common/azure-arm-rest/azure-arm-resource-filter.ts

+        this._client = client;
+    }
+
+    public getResources(resourceType: string, resourceName: string, options: any, callback: azureServiceClient.ApiCallback) {


while the callback based approach is what is used most in this library - for newer methods we are keeping it all in promises for cleaner code. If you want to use Promises for the public methods - that might keep the code simpler. This method would be changed to
public getResources(resourceType: string, resourceName: string) : Promise

Created a wrapper class that resolves this callback as a promise.
https://github.com/Microsoft/vsts-tasks/pull/6001/files#diff-6bc02ddedf82bfe34fa4693469e1736eR20

Don't have the callback version at all. That will help you avoid creating the wrapper

Removed callback as discussed offline.

sachinma · 2017-12-11T05:49:46Z

Tasks/Common/azure-arm-rest/AzureResourceFilter.ts

+import { AzureResourceFilterManagementClient } from './azure-arm-resource-filter';
+import { AzureEndpoint } from './azureModels';
+
+export class AzureResourceFilterController {


why do you need this wrapper class. If it is for callback to promise conversion - you can directly use promises in the client class itself

@sachinma , this wrapper class is created as the logic required to find resources is common for Manage and deploy task. Resolving the promise here.

sachinma · 2017-12-11T05:50:49Z

Tasks/Common/azure-arm-rest/AzureResourceFilter.ts

+    private _azureResourceFilterManagementClient: AzureResourceFilterManagementClient;
+    private _endpoint: AzureEndpoint;
+
+    constructor(endpoint) {


consider providing an overload or a converter method directly in the base client class

sachinma · 2017-12-11T05:52:07Z

Tasks/Common/azure-arm-rest/azure-arm-common.ts

@@ -101,3 +102,118 @@ export class ApplicationTokenCredentials {
        return deferred.promise;
    }
 }
+
+export class AzureRMEndpoint {


Consider moving to a separate file

asranja · 2017-12-12T07:30:37Z

Tasks/AzureAppServiceManage/azureappservicemanage.ts

-
+import { AzureRMEndpoint } from 'azure-arm-rest/azure-arm-common';
+import { AzureEndpoint } from 'azure-arm-rest/azureModels';
+import {AzureAppService  } from 'azure-arm-rest/azure-arm-app-service';


missing space 👅

asranja · 2017-12-12T07:36:08Z

Tasks/AzureAppServiceManage/azureappservicemanage.ts

        switch(action) {
            case "Start Azure App Service": {
-                console.log(await azureRmUtil.startAppService(endPoint, resourceGroupName, webAppName, specifySlotFlag, slotName));
-                await waitForAppServiceToStart(endPoint, resourceGroupName, webAppName, specifySlotFlag, slotName);
+                var appService: AzureAppService = new AzureAppService(azureEndpoint, resourceGroupName, webAppName, slotName);


can this be moved outside switch and reused in all cases ?

This is not required for Swap slots, so made it duplicate.
Will check for other efficient ways,

asranja · 2017-12-12T07:40:35Z

Tasks/Common/azure-arm-rest/webClient.ts

@@ -86,7 +86,7 @@ async function toWebResponse(response: httpClient.HttpClientResponse): Promise<W
    return res;
 }

-function sleepFor(sleepDurationInSeconds): Promise<any> {
+export function sleepFor(sleepDurationInSeconds): Promise<any> {
    return new Promise((resolve, reeject) => {


"reeject" the promise 😝

asranja · 2017-12-12T07:44:24Z

Tasks/Common/azure-arm-rest/azure-arm-resource.ts

+
+            result = result.concat(response.body.value);
+            if(response.body.nextLink) {
+                var nextResult = await this._client.accumulateResultFromPagedResult(response.body.nextLink);


the second set of results can also have nextLink, right ?

accumulateResultFromPagedResult has a loop that queries for the result for next result API and so on.

asranja · 2017-12-12T07:49:46Z

Tasks/Common/azure-arm-rest/azure-arm-appinsights.ts

+    private _endpoint: AzureEndpoint;
+    private _client: ServiceClient;
+
+    constructor(endpoint: AzureEndpoint, resourceGroupName: string, resourceName: string) {


consider renaming resourceName to applicationInsightName.

asranja · 2017-12-12T07:52:50Z

Tasks/Common/azure-arm-rest/azure-arm-appinsights-webtests.ts

+
+    }
+
+    private  webTestData = {


private variables should start with _

Ajay-MS · 2017-12-15T06:04:13Z

Tasks/Common/azure-arm-rest/azure-arm-endpoint.ts

+import webClient = require("./webClient");
+import { AzureEndpoint } from "./azureModels";
+export class AzureRMEndpoint {
+    public _endpoint: AzureEndpoint;


I assume _variablename format you have choosen for private variables only. At few places public variables are defined with underscore and some places private variables are without underscore. Can we keep pattern same.

sure. thanks

Ajay-MS · 2017-12-15T06:07:45Z

Tasks/Common/azure-arm-rest/azure-arm-endpoint.ts

+                    break;
+                }
+                default: {
+                    this._endpoint.environmentAuthorityUrl = (!!this._endpoint.environmentAuthorityUrl) ? this._endpoint.environmentAuthorityUrl : "https://login.windows.net/";


Move "https://login.windows.net/" to constants. Should we define a constants file and add all constants there.

Ajay-MS · 2017-12-15T06:12:42Z

Tasks/Common/azure-arm-rest/azure-arm-endpoint.ts

+
+    }
+
+    private _getAzureStackData(endpoint: AzureEndpoint) {


By defining this method I am assuming you are removing dependency from azurestack-common. Please make sure all modules that are dependent on azurestack-common will get updated. Let me know if you need help regarding azure stack validations.

the dependency is removed only for Azue App Service Manage task, and will be removing for Azure App Service Deploy task too.
As discussed, it may be a code redundancy, will inform the other task owners as well. (Azure Key vault & Azure RG task)

Ajay-MS · 2017-12-15T06:15:22Z

Tasks/Common/azure-arm-rest/azure-arm-common.ts

@@ -2,7 +2,7 @@ import tl = require('vsts-task-lib/task');
 import Q = require('q');
 import querystring = require('querystring');
 import webClient = require("./webClient");
-var util = require('util');
+import { AzureEndpoint } from "./azureModels";


Where it's being used ?

Ajay-MS · 2017-12-15T06:16:40Z

Tasks/Common/azure-arm-rest/azure-arm-appinsights.ts

+        {
+            '{resourceGroupName}': this._resourceGroupName,
+            '{resourceName}': this._resourceName,
+        }, null, '2015-05-01');


There should be constant for API Version

Ajay-MS · 2017-12-15T06:19:57Z

Tasks/Common/azure-arm-rest/azure-arm-appinsights-webtests.ts

+        httpRequest.uri = this._client.getRequestUri(`//subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/microsoft.insights/webtests`,
+        {
+            '{resourceGroupName}': this._resourceGroupName
+        }, null, '2015-05-01');


Constant for API Version

will do it with deploy task.

Ajay-MS · 2017-12-15T06:23:59Z

Tasks/Common/azure-arm-rest/azure-arm-appinsights-webtests.ts

+
+    public async create(appInsightsResource: any, applicationUrl: string) {
+        var webTestName = "vsts-web-test-" + Date.now();
+        var webTestData =  JSON.parse(JSON.stringify(this.webTestData));


should we create a new private method populateWebTestData

Ajay-MS · 2017-12-15T10:30:54Z

Tasks/Common/azure-arm-rest/azure-arm-endpoint.ts

+            // Initialize Azure Endpoint for specific environment
+            switch(this._endpoint.environment.toLowerCase()) {
+                case this._environments.AzureStack: {
+                    this._getAzureStackData(this._endpoint).then((endpoint) => {


We need to initialize this value if Azure Stack data is not being initialized from server.

dikhakha · 2017-12-20T06:24:19Z

Tasks/AzureAppServiceManage/azureappservicemanage.ts

+            }
+            case "Install Extensions": {
+                if(appServiceKuduService) {
+                    await updateDeploymentStatusInKudu(appServiceKuduService, taskResult, DeploymentID, {"type": action});


where is the DeploymentID initialized ?

Ajay-MS · 2017-12-20T10:45:13Z

Tasks/Common/azure-arm-rest/azure-arm-endpoint.ts

+                activeDirectoryResourceID: tl.getEndpointDataParameter(this._connectedServiceName, 'activeDirectoryServiceEndpointResourceId', true)
+            } as AzureEndpoint;
+
+            if(this.endpoint.environment != null && ( !this.endpoint.environmentAuthorityUrl || !this.endpoint.activeDirectoryResourceID)) {


Check for Azure Stack can be moved above instead of nested check.

asranja · 2017-12-20T17:04:33Z

Tasks/AzureAppServiceManage/azureappservicemanage.ts

+import { AzureRMEndpoint } from 'azure-arm-rest/azure-arm-endpoint';
+import { AzureEndpoint } from 'azure-arm-rest/azureModels';
+import {AzureAppService  } from 'azure-arm-rest/azure-arm-app-service';
+// import { AzureAppService } from 'azure-arm-rest/AzureAppService';


asranja · 2017-12-20T17:14:05Z

Tasks/Common/azure-arm-rest/azure-arm-app-service-kudu.ts

+            retryIntervalInSeconds: reqOptions && reqOptions.retryIntervalInSeconds ? reqOptions.retryIntervalInSeconds :  10,
+            retryCount: reqOptions && reqOptions.retryCount ? reqOptions.retryCount : 6,
+            retriableErrorCodes: reqOptions && reqOptions.retriableErrorCodes ? reqOptions.retriableErrorCodes : ["ETIMEDOUT"],
+            retriableStatusCodes: reqOptions && reqOptions.retriableStatusCodes ? reqOptions.retriableStatusCodes :  [409, 500, 502, 503]


please add 504 Gateway timeout as well.

vincent1173 added 2 commits December 10, 2017 23:49

Refactored Manage Task

d3c53a8

Fix WebTest Issue

04be72d

vincent1173 requested review from bishal-pdMSFT, kmkumaran, Ajay-MS, sachinma, arjgupta, asranja, dikhakha, aydey and thesattiraju December 10, 2017 18:23

kmkumaran self-assigned this Dec 11, 2017

kmkumaran added the Area: Release label Dec 11, 2017

vincent1173 added the Area: AzureAppService Label to monitor Azure App Service issues label Dec 11, 2017

vincent1173 requested a review from pavanadepu2 December 11, 2017 05:17

sachinma reviewed Dec 11, 2017

View reviewed changes

vincent1173 added 5 commits December 11, 2017 11:43

Bug fixes #2

b4dbdaf

Addressed sachinm comments #1

c89bef6

add iterator

0d9287b

added xml2js dependency

65523f0

Addressed review comments #2

d7937d9

vincent1173 requested a review from rajatagrawal-dev December 12, 2017 05:34

bug Fix #3

45f6968

asranja reviewed Dec 12, 2017

View reviewed changes

Added single L0 Test to check PR Build

0cd53e7

Ajay-MS reviewed Dec 15, 2017

View reviewed changes

vincent1173 added 2 commits December 15, 2017 12:39

addressed few review comments

2c68d04

Fix L0 Tests

ddd2e42

Ajay-MS reviewed Dec 15, 2017

View reviewed changes

vincent1173 mentioned this pull request Dec 17, 2017

Swap Slots Error: Bad Request #5566

Closed

vincent1173 added 10 commits December 18, 2017 11:41

addressed review comments

509f72e

site extension issue fix

62cce8f

addressed review comments

5fea981

L0 Tests added

ed9d19e

change BuildID to releaseID

d5501c3

removed irrelevant log statements

f066868

fixed error name

14f86b9

Fix L0 Test

b18c310

Fix L0 Test

0454af4

Format L0 Tests

e7816b7

dikhakha reviewed Dec 20, 2017

View reviewed changes

remove deployment ID

7167c63

Ajay-MS reviewed Dec 20, 2017

View reviewed changes

Addressed review comments

82b211f

Ajay-MS approved these changes Dec 20, 2017

View reviewed changes

asranja reviewed Dec 20, 2017

View reviewed changes

asranja approved these changes Dec 20, 2017

View reviewed changes

addressed review comments

a75bd76

vincent1173 merged commit 6b63550 into master Dec 21, 2017


		export class AzureResourceFilterManagementClient extends azureServiceClient.ServiceClient {
		public _azureResourceFilter: AzureResourceFilter;

Azure App Service Manage Task - Refactor & Bug fixes #6001

Azure App Service Manage Task - Refactor & Bug fixes #6001

Conversation

vincent1173 commented Dec 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment