Repository: serverless/serverless-azure-functions
Pull request #354 (2019-09-30 20:34:39)

fix: Sort deployments in descending order and fix APIM arm template
<!--
1. Please check out and follow our Contributing Guidelines: https://github.com/serverless/serverless-azure-functions/CONTRIBUTING.md
2. Do not remove any section of the template. If something is not applicable leave it empty but leave it in the PR
3. Please follow the template, otherwise we'll have to ask you to update it and it will take longer until your PR is merged
-->

## What did you implement:

- Updated parameter name in APIM arm template
- Fixed bug of sorting deployments in ascending order, when it should have been descending. This would have pretty serious consequences, because it means that the comparison of ARM templates would always be targeting the first ever deployment, not the most recent.
- Because the `sort()` function sorts the array in place, this bug was not being detected by the tests. Updated `resourceService` tests to create copies of the array rather than using the original reference when testing the validity of the result.

<!--
Briefly describe the feature if no issue exists for this PR
-->

## How did you implement it:

- Flipped the timestamp comparison operator
- Updated the string in `apim.ts`

## How can we verify it:

- Deploy a service
- Make changes to an environment variable
- Deploy it again (should deploy ARM template)
- Change back to the original configuration
- Deploy it again (should deploy ARM template)

Without this fix, the last step would not have deployed the ARM template since it would be identical to the first deployment ever.

<!--
Add any applicable config, commands, screenshots or other resources
to make it easy for us to verify this works. The easier you make it for us
to review a PR, the faster we can review and merge it.

Examples:
* serverless.yml - Fully functioning to easily deploy changes
* Screenshots - Showing the difference between your output and the master
* Cloud Configuration - List cloud resources and show that the correct configuration is in place (e.g. AWS CLI commands)
* Other - Anything else that comes to mind to help us evaluate
-->

## Todos:

_**Note: Run `npm run test:ci` to run all validation checks on proposed changes**_

- [x] Write tests and confirm existing functionality is not broken.  
       **Validate via `npm test`**
- [x] Write documentation
- [x] Ensure there are no lint errors.  
       **Validate via `npm run lint`**  
       _Note: Some reported issues can be automatically fixed by running `npm run lint:fix`_
- [x] Make sure code coverage hasn't dropped
- [x] Provide verification config / commands / resources
- [x] Enable "Allow edits from maintainers" for this PR
- [x] Update the messages below

**_Is this ready for review?:_** YES  
**_Is it a breaking change?:_** NO



Patch:
diff --git a/src/armTemplates/resources/apim.ts b/src/armTemplates/resources/apim.ts
index adb1743..6869c3a 100644
--- a/src/armTemplates/resources/apim.ts
+++ b/src/armTemplates/resources/apim.ts
@@ -61,7 +61,7 @@ export class ApimResource implements ArmResourceTemplateGenerator {
           "location": "[parameters('location')]",
           "sku": {
             "name": "[parameters('apimSkuName')]",
-            "capacity": "[parameters('apimCapacity')]"
+            "capacity": "[parameters('apimSkuCapacity')]"
           },
           "properties": {
             "publisherEmail": "[parameters('apimPublisherEmail')]",
diff --git a/src/services/resourceService.ts b/src/services/resourceService.ts
index b76a153..264aa8e 100644
--- a/src/services/resourceService.ts
+++ b/src/services/resourceService.ts
@@ -22,7 +22,7 @@ export class ResourceService extends BaseService {
     this.log(`Listing deployments for resource group '${this.resourceGroup}':`);
     const deployments = await this.resourceClient.deployments.listByResourceGroup(this.resourceGroup);
     return deployments.sort((a: DeploymentExtended, b: DeploymentExtended) => {
-      return (a.properties.timestamp > b.properties.timestamp) ? 1 : -1
+      return (a.properties.timestamp < b.properties.timestamp) ? 1 : -1
     });
   }
 


Test patch:
diff --git a/src/services/resourceService.test.ts b/src/services/resourceService.test.ts
index dffaa4e..9482e83 100644
--- a/src/services/resourceService.test.ts
+++ b/src/services/resourceService.test.ts
@@ -7,11 +7,14 @@ import { ResourceService } from "./resourceService";
 jest.mock("@azure/arm-resources")
 
 describe("Resource Service", () => {
-  let deployments: DeploymentsListByResourceGroupResponse;
+  
+  /**
+   * List of deployments in ASCENDING order (oldest first)
+   */
+  const deployments: DeploymentsListByResourceGroupResponse = MockFactory.createTestDeployments(5, true);
   const template = "myTemplate";
 
   beforeEach(() => {
-    deployments = MockFactory.createTestDeployments(5, true);
     ResourceManagementClient.prototype.resourceGroups = {
       createOrUpdate: jest.fn(),
       deleteMethod: jest.fn(),
@@ -19,7 +22,7 @@ describe("Resource Service", () => {
 
     ResourceManagementClient.prototype.deployments = {
       deleteMethod: jest.fn(),
-      listByResourceGroup: jest.fn(() => Promise.resolve(deployments)),
+      listByResourceGroup: jest.fn(() => Promise.resolve([...deployments])),
       exportTemplate: jest.fn(() => Promise.resolve(template)),
     } as any;
   });
@@ -90,7 +93,7 @@ describe("Resource Service", () => {
     const service = new ResourceService(sls, options);
     const deps = await service.getDeployments();
     // Make sure deps are in correct order
-    expect(deps).toEqual(deployments);
+    expect(deps).toEqual([...deployments].reverse());
   });
 
   it("lists deployments as string with timestamps", async () => {
@@ -102,10 +105,10 @@ describe("Resource Service", () => {
     const service = new ResourceService(sls, options);
     const deploymentString = await service.listDeployments();
     let expectedDeploymentString = "\n\nDeployments";
-    const originalTimestamp = +MockFactory.createTestTimestamp();
-    let i = 0
-    for (const dep of deployments) {
-      const timestamp = originalTimestamp + i
+    const originalTimestamp = +MockFactory.createTestTimestamp() + 4;
+    let i = 0;
+    for (const dep of [...deployments].reverse()) {
+      const timestamp = originalTimestamp - i;
       expectedDeploymentString += "\n-----------\n"
       expectedDeploymentString += `Name: ${dep.name}\n`
       expectedDeploymentString += `Timestamp: ${timestamp}\n`;
@@ -117,9 +120,9 @@ describe("Resource Service", () => {
   });
 
   it("lists deployments as string without timestamps", async () => {
-    deployments = MockFactory.createTestDeployments();
+    const deployments = MockFactory.createTestDeployments();
     ResourceManagementClient.prototype.deployments = {
-      listByResourceGroup: jest.fn(() => Promise.resolve(deployments)),
+      listByResourceGroup: jest.fn(() => Promise.resolve([...deployments])),
     } as any;
 
     const sls = MockFactory.createTestServerless();
@@ -130,7 +133,7 @@ describe("Resource Service", () => {
     const service = new ResourceService(sls, options);
     const deploymentString = await service.listDeployments();
     let expectedDeploymentString = "\n\nDeployments";
-    for (const dep of deployments) {
+    for (const dep of [...deployments].reverse()) {
       expectedDeploymentString += "\n-----------\n"
       expectedDeploymentString += `Name: ${dep.name}\n`
       expectedDeploymentString += "Timestamp: None\n";
@@ -156,4 +159,15 @@ describe("Resource Service", () => {
       );
     expect(result).toEqual(template);
   });
+
+  it("gets previous deployment template", async () => {
+    ResourceManagementClient.prototype.deployments = {
+      listByResourceGroup: jest.fn(() => Promise.resolve([...deployments])),
+    } as any;
+    const sls = MockFactory.createTestServerless();
+    const service = new ResourceService(sls, {} as any);
+    const deployment = await service.getPreviousDeployment();
+    const expected = deployments[deployments.length - 1];
+    expect(deployment).toEqual(expected);
+  });
 });