Skip to content

Commit

Permalink
fix: Sort deployments in descending order and fix APIM arm template (#…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
tbarlow12 committed Oct 3, 2019
1 parent 118eb91 commit 865cfd0
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 13 deletions.
2 changes: 1 addition & 1 deletion src/armTemplates/resources/apim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')]",
Expand Down
36 changes: 25 additions & 11 deletions src/services/resourceService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,22 @@ 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(),
} as any;

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;
});
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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`;
Expand All @@ -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();
Expand All @@ -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";
Expand All @@ -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);
});
});
2 changes: 1 addition & 1 deletion src/services/resourceService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
}

Expand Down

0 comments on commit 865cfd0

Please sign in to comment.