Repository: serverless/serverless-azure-functions
Pull request #301 (2019-09-03 22:56:06)

fix: Move service initialization outside of plugins
Fixes error we found in testing serverless optional variables (`${opt:stage, 'dev'}`), which allow you to specify a param at deploy time, as well as a default value. Also fixes potentially more serious problems with setting names before the configuration is initialized.

mydiemho: > Fixes error we found in testing serverless optional variables (`${opt:stage, 'dev'}`), which allow you to specify a param at deploy time, as well as a default value. Also fixes potentially more serious problems with setting names before the configuration is initialized.

1. what is the error you see when testing ` (`${opt:stage, 'dev'}`),`?
1. What are the serious problems with setting names before configuration is initialized?


Patch:
diff --git a/package-lock.json b/package-lock.json
index 1c07b97..c7ed0d6 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1,6 +1,6 @@
 {
   "name": "serverless-azure-functions",
-  "version": "1.0.0-34",
+  "version": "1.0.0-35",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
diff --git a/package.json b/package.json
index 5e71fb5..7188059 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "serverless-azure-functions",
-  "version": "1.0.0-34",
+  "version": "1.0.0-35",
   "description": "Provider plugin for the Serverless Framework v1.x which adds support for Azure Functions.",
   "license": "MIT",
   "main": "./lib/index.js",
diff --git a/src/models/azureProvider.ts b/src/models/azureProvider.ts
index 08483f8..33c0e64 100644
--- a/src/models/azureProvider.ts
+++ b/src/models/azureProvider.ts
@@ -18,3 +18,9 @@ export interface FunctionMetadata {
   handler: string;
   events: FunctionEvent[];
 }
+
+export interface DeploymentExtendedError {
+  code: string;
+  message: string;
+  details?: DeploymentExtendedError[];
+}
diff --git a/src/plugins/func/azureFuncPlugin.ts b/src/plugins/func/azureFuncPlugin.ts
index 3a7c48c..7c3d79a 100644
--- a/src/plugins/func/azureFuncPlugin.ts
+++ b/src/plugins/func/azureFuncPlugin.ts
@@ -3,7 +3,6 @@ import { FuncService } from "../../services/funcService";
 import { AzureBasePlugin } from "../azureBasePlugin";
 
 export class AzureFuncPlugin extends AzureBasePlugin {
-  private service: FuncService;
 
   public constructor(serverless: Serverless, options: Serverless.Options) {
     super(serverless, options);
@@ -48,8 +47,6 @@ export class AzureFuncPlugin extends AzureBasePlugin {
         }
       }
     }
-
-    this.service = new FuncService(serverless, options);
   }
 
   private async func() {
@@ -57,10 +54,12 @@ export class AzureFuncPlugin extends AzureBasePlugin {
   }
 
   private async add() {
-    this.service.add();
+    const service = new FuncService(this.serverless, this.options);
+    service.add();
   }
 
   private async remove() {
-    this.service.remove();
+    const service = new FuncService(this.serverless, this.options);
+    service.remove();
   }
 }
diff --git a/src/plugins/invoke/azureInvokePlugin.ts b/src/plugins/invoke/azureInvokePlugin.ts
index 17a0f9b..ae2f117 100644
--- a/src/plugins/invoke/azureInvokePlugin.ts
+++ b/src/plugins/invoke/azureInvokePlugin.ts
@@ -6,8 +6,6 @@ import { AzureBasePlugin } from "../azureBasePlugin";
 
 export class AzureInvokePlugin extends AzureBasePlugin {
 
-  private invokeService: InvokeService;
-
   public constructor(serverless: Serverless, options: Serverless.Options) {
     super(serverless, options);
     const path = this.options["path"];
@@ -117,8 +115,8 @@ export class AzureInvokePlugin extends AzureBasePlugin {
       return;
     }
 
-    this.invokeService = new InvokeService(this.serverless, this.options, local);
-    const response = await this.invokeService.invoke(method, functionName, data);
+    const invokeService = new InvokeService(this.serverless, this.options, local);
+    const response = await invokeService.invoke(method, functionName, data);
     if (response) {
       this.log(JSON.stringify(response.data));
     }
diff --git a/src/plugins/offline/azureOfflinePlugin.ts b/src/plugins/offline/azureOfflinePlugin.ts
index 699419b..129d2ee 100644
--- a/src/plugins/offline/azureOfflinePlugin.ts
+++ b/src/plugins/offline/azureOfflinePlugin.ts
@@ -3,11 +3,9 @@ import { OfflineService } from "../../services/offlineService";
 import { AzureBasePlugin } from "../azureBasePlugin";
 
 export class AzureOfflinePlugin extends AzureBasePlugin {
-  private offlineService: OfflineService;
 
   public constructor(serverless: Serverless, options: Serverless.Options) {
     super(serverless, options);
-    this.offlineService = new OfflineService(this.serverless, this.options);
 
     this.hooks = {
       "before:offline:offline": this.azureOfflineBuild.bind(this),
@@ -60,14 +58,17 @@ export class AzureOfflinePlugin extends AzureBasePlugin {
   }
 
   private async azureOfflineBuild(){
-    await this.offlineService.build();
+    const offlineService = new OfflineService(this.serverless, this.options);
+    await offlineService.build();
   }
 
   private async azureOfflineStart(){
-    await this.offlineService.start();
+    const offlineService = new OfflineService(this.serverless, this.options);
+    await offlineService.start();
   }
 
   private async azureOfflineCleanup(){
-    await this.offlineService.cleanup();
+    const offlineService = new OfflineService(this.serverless, this.options);
+    await offlineService.cleanup();
   }
 }
diff --git a/src/plugins/package/azurePackagePlugin.ts b/src/plugins/package/azurePackagePlugin.ts
index 2c30567..91c7de6 100644
--- a/src/plugins/package/azurePackagePlugin.ts
+++ b/src/plugins/package/azurePackagePlugin.ts
@@ -7,7 +7,6 @@ import { ServerlessCliCommand } from "../../models/serverless";
 
 export class AzurePackagePlugin extends AzureBasePlugin {
   private bindingsCreated: boolean = false;
-  private packageService: PackageService;
   public provider: AzureProvider;
 
   public constructor(serverless: Serverless, options: Serverless.Options) {
@@ -17,11 +16,11 @@ export class AzurePackagePlugin extends AzureBasePlugin {
       "before:webpack:package:packageModules": this.webpack.bind(this),
       "after:package:finalize": this.finalize.bind(this),
     };
-
-    this.packageService = new PackageService(this.serverless, this.options);
   }
 
   private async setupProviderConfiguration(): Promise<void> {
+    const packageService = new PackageService(this.serverless, this.options);
+
     if (this.processedCommands[0] === ServerlessCliCommand.DEPLOY && this.getOption("package")) {
       this.log("Deploying pre-built package. No need to create bindings");
       return;
@@ -30,14 +29,16 @@ export class AzurePackagePlugin extends AzureBasePlugin {
       throw new Error("Cannot package Azure Functions individually. " +
         "Remove `individually` attribute from the `package` section of the serverless config");
     }
-    this.packageService.cleanUpServerlessDir();
-    await this.packageService.createBindings();
+    packageService.cleanUpServerlessDir();
+    await packageService.createBindings();
     this.bindingsCreated = true;
 
     return Promise.resolve();
   }
 
   private async webpack(): Promise<void> {
+    const packageService = new PackageService(this.serverless, this.options);
+
     if (this.getOption("package")) {
       this.log("No need to perform webpack. Using pre-existing package");
       return Promise.resolve();
@@ -46,17 +47,19 @@ export class AzurePackagePlugin extends AzureBasePlugin {
       await this.setupProviderConfiguration();
     }
 
-    await this.packageService.prepareWebpack();
+    await packageService.prepareWebpack();
   }
 
   /**
    * Cleans up generated folders & files after packaging is complete
    */
   private async finalize(): Promise<void> {
+    const packageService = new PackageService(this.serverless, this.options);
+
     if (this.getOption("package")) {
       this.log("No need to clean up generated folders & files. Using pre-existing package");
       return Promise.resolve();
     }
-    await this.packageService.cleanUp();
+    await packageService.cleanUp();
   }
 }
diff --git a/src/services/armService.ts b/src/services/armService.ts
index d2dfd75..09d2b93 100644
--- a/src/services/armService.ts
+++ b/src/services/armService.ts
@@ -10,6 +10,7 @@ import { Guard } from "../shared/guard";
 import { BaseService } from "./baseService";
 import { ResourceService } from "./resourceService"
 import deepEqual from "deep-equal";
+import { DeploymentExtendedError } from "../models/azureProvider";
 
 export class ArmService extends BaseService {
   private resourceClient: ResourceManagementClient;
@@ -130,14 +131,39 @@ export class ArmService extends BaseService {
     this.log(`---> Resource Group: ${this.resourceGroup}`)
     this.log(`---> Deployment Name: ${this.deploymentName}`)
 
-    const result = await this.resourceClient.deployments.createOrUpdate(
-      this.resourceGroup,
-      this.deploymentName,
-      armDeployment
-    );
-    this.log("-> ARM deployment complete");
+    try {
+      const result = await this.resourceClient.deployments.createOrUpdate(
+        this.resourceGroup,
+        this.deploymentName,
+        armDeployment
+      );
+      this.log("-> ARM deployment complete");
+      return result;
+    } catch (err) {
+      const lastDeployment = await resourceService.getLastDeployment();
+      const errorDetails: DeploymentExtendedError = lastDeployment.properties["error"];
+      if (errorDetails) {
+        throw new Error(this.deploymentErrorToString(errorDetails));
+      }
+    }
+  }
 
-    return result;
+  private deploymentErrorToString(deploymentError: DeploymentExtendedError) {
+    if (!deploymentError.code || !deploymentError.message) {
+      return JSON.stringify(deploymentError);
+    }
+    let errorString = `${deploymentError.code} - ${deploymentError.message}`;
+    if (deploymentError.details) {
+
+      errorString += `
+      ------------------------
+      DEPLOYMENT ERROR DETAILS
+      ------------------------\n`
+      deploymentError.details.forEach((childError) => {
+        errorString += `\n${this.deploymentErrorToString(childError)}`
+      })
+    }
+    return errorString;
   }
 
   /**
diff --git a/src/services/baseService.ts b/src/services/baseService.ts
index e2da323..e1a513a 100644
--- a/src/services/baseService.ts
+++ b/src/services/baseService.ts
@@ -36,7 +36,7 @@ export abstract class BaseService {
     Guard.null(serverless);
     this.setDefaultValues();
     this.config = serverless.service as any;
-    this.setConfigFromCli();
+    this.setupConfig();
 
     this.baseUrl = "https://management.azure.com";
     this.serviceName = this.getServiceName();
@@ -227,7 +227,7 @@ export abstract class BaseService {
 
     if (!providerRegion || providerRegion === awsDefault) {
       // no region specified in serverless.yml
-      this.serverless.service.provider.region = "westus";
+      this.serverless.service.provider.region = this.serverless.service.provider["location"] || "westus";
     }
 
     if (!this.serverless.service.provider.stage) {
@@ -256,8 +256,9 @@ export abstract class BaseService {
    * Overwrite values for resourceGroup, prefix, region and stage
    * in config if passed through CLI
    */
-  private setConfigFromCli() {
+  private setupConfig() {
     const { prefix, region, stage, subscriptionId } = this.config.provider;
+
     this.config.provider = {
       ...this.config.provider,
       prefix: this.getOption("prefix") || prefix,
@@ -268,9 +269,6 @@ export abstract class BaseService {
         || subscriptionId
         || this.serverless.variables["subscriptionId"]
     }
-    if (!this.config.provider.region && this.config.provider["location"]) {
-      this.config.provider.region = this.config.provider["location"];
-    }
     this.config.provider.resourceGroup = (
       this.getOption("resourceGroup", this.config.provider.resourceGroup)
     ) || AzureNamingService.getResourceName(this.config, null, `${this.getServiceName()}-rg`);


Test patch:
diff --git a/src/index.test.ts b/src/index.test.ts
index 1eb39ab..76e999c 100644
--- a/src/index.test.ts
+++ b/src/index.test.ts
@@ -8,6 +8,13 @@ import { AzureLoginPlugin } from "./plugins/login/azureLoginPlugin";
 import { AzureApimServicePlugin } from "./plugins/apim/azureApimServicePlugin";
 import { AzureApimFunctionPlugin } from "./plugins/apim/azureApimFunctionPlugin";
 import AzureProvider from "./provider/azureProvider";
+import { AzureFuncPlugin } from "./plugins/func/azureFuncPlugin";
+import { AzureOfflinePlugin } from "./plugins/offline/azureOfflinePlugin";
+import { AzureRollbackPlugin } from "./plugins/rollback/azureRollbackPlugin";
+
+jest.genMockFromModule("./services/baseService");
+jest.mock("./services/baseService")
+import { BaseService } from "./services/baseService";
 
 describe("Azure Index", () => {
   it("contains all registered plugins", () => {
@@ -25,5 +32,23 @@ describe("Azure Index", () => {
     expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureDeployPlugin);
     expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureApimServicePlugin);
     expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureApimFunctionPlugin);
+    expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureFuncPlugin);
+    expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureOfflinePlugin);
+    expect(sls.pluginManager.addPlugin).toBeCalledWith(AzureRollbackPlugin);
+  });
+
+  it("does not initialize BaseService in constructor of any plugin", () => {
+    const sls = MockFactory.createTestServerless();
+    const options = MockFactory.createTestServerlessOptions();
+    new AzureIndex(sls, options);
+    sls.setProvider = jest.fn();
+
+    const calls = (sls.pluginManager.addPlugin as any).mock.calls;
+
+    for (const call of calls) {
+      const pluginClass = call[0];
+      new pluginClass(MockFactory.createTestServerless(), MockFactory.createTestServerlessOptions());
+    }
+    expect(BaseService).not.toBeCalled();
   });
 });
diff --git a/src/services/armService.test.ts b/src/services/armService.test.ts
index d70cb46..a052906 100644
--- a/src/services/armService.test.ts
+++ b/src/services/armService.test.ts
@@ -8,6 +8,7 @@ import jsonpath from "jsonpath";
 import { Deployments } from "@azure/arm-resources";
 import { Deployment, DeploymentExtended } from "@azure/arm-resources/esm/models";
 import { ResourceService } from "./resourceService";
+import { DeploymentExtendedError } from "../models/azureProvider";
 
 describe("Arm Service", () => {
   let sls: Serverless
@@ -311,5 +312,47 @@ describe("Arm Service", () => {
       expect(call[1]).toMatch(expectedDeploymentNameRegex);
       expect(call[2]).toEqual(expectedDeployment);
     });
+
+    it("Throws more detailed error message upon failed ARM deployment", async () => {
+      Deployments.prototype.createOrUpdate = jest.fn(() => Promise.reject(null));
+      const lastDeploymentError: DeploymentExtendedError = {
+        code: "DeploymentFailed",
+        message: "At least one resource deployment operation failed. Please list deployment operations for details. Please see https://aka.ms/arm-debug for usage details.",
+        details: [
+          {
+            code: "ServiceAlreadyExists",
+            message: "Api service already exists: abc-123-apim"
+          },
+          {
+            code: "StorageAccountAlreadyTaken",
+            message: "The storage account named ABC123 is already taken."
+          }
+        ]
+      }
+      ResourceService.prototype.getLastDeployment = jest.fn(() => Promise.resolve({
+        properties: {
+          error: lastDeploymentError
+        }
+      })) as any;
+      const deployment: ArmDeployment = {
+        parameters: MockFactory.createTestParameters(false),
+        template: MockFactory.createTestArmTemplate()
+      };
+      deployment.parameters.param1 = "3"
+      const { code, message, details } = lastDeploymentError;
+      let errorPattern = [
+        code,
+        message,
+        details[0].code,
+        details[0].message,
+        details[1].code,
+        details[1].message
+      ].join(".*")
+      await expect(service.deployTemplate(deployment))
+        .rejects
+        .toThrowError(
+          new RegExp(`.*${errorPattern}.*`,"s")
+        );
+    });
   });
 });
diff --git a/src/services/baseService.test.ts b/src/services/baseService.test.ts
index af6c23f..402ff1e 100644
--- a/src/services/baseService.test.ts
+++ b/src/services/baseService.test.ts
@@ -275,4 +275,18 @@ describe("Base Service", () => {
     expect(service.getSubscriptionId()).toEqual(loginResultSubscriptionId);
     expect(serverless.service.provider["subscriptionId"]).toEqual(loginResultSubscriptionId);
   });
+
+  it("sets region to be value from location property if region not set", () => {
+    const slsService = MockFactory.createTestService();
+    delete slsService.provider.region;
+    const location = "East US";
+    slsService.provider["location"] = location;
+
+    const sls = MockFactory.createTestServerless({
+      service: slsService
+    });
+
+    service = new MockService(sls, {} as any);
+    expect(service.getRegion()).toEqual(location);
+  });
 });