From 865cfd0b67813c2af99356b93ddaf51e2d6fe9a3 Mon Sep 17 00:00:00 2001 From: Tanner Barlow Date: Thu, 3 Oct 2019 10:53:07 -0700 Subject: [PATCH] fix: Sort deployments in descending order and fix APIM arm template (#354) - 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. --- src/armTemplates/resources/apim.ts | 2 +- src/services/resourceService.test.ts | 36 +++++++++++++++++++--------- src/services/resourceService.ts | 2 +- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/armTemplates/resources/apim.ts b/src/armTemplates/resources/apim.ts index adb1743c..6869c3a5 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.test.ts b/src/services/resourceService.test.ts index dffaa4e9..9482e832 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); + }); }); diff --git a/src/services/resourceService.ts b/src/services/resourceService.ts index b76a1531..264aa8e2 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 }); }