Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

fix: Clean up downloaded artifact after rollback #289

Merged
merged 1 commit into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/services/rollbackService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ArmDeployment } from "../models/armTemplates";
import { DeploymentConfig } from "../models/serverless";
import { MockFactory } from "../test/mockFactory";
import { RollbackService } from "./rollbackService";
import fs from "fs";

jest.mock("./azureBlobStorageService");
import { AzureBlobStorageService } from "./azureBlobStorageService";
Expand All @@ -31,9 +32,10 @@ describe("Rollback Service", () => {
const containerName = "deployment-artifacts";
const artifactName = MockFactory.createTestDeployment().name.replace(
configConstants.naming.suffix.deployment, configConstants.naming.suffix.artifact) + ".zip";
const artifactPath = `.serverless${path.sep}${artifactName}`
const artifactPath = path.join(".serverless", artifactName);
const armDeployment: ArmDeployment = { template, parameters };
const deploymentString = "deployments";
let unlinkSpy: jest.SpyInstance;

function createOptions(timestamp?: string): Serverless.Options {
return {
Expand Down Expand Up @@ -64,10 +66,12 @@ describe("Rollback Service", () => {
ResourceService.prototype.listDeployments = jest.fn(() => Promise.resolve(deploymentString))
AzureBlobStorageService.prototype.generateBlobSasTokenUrl = jest.fn(() => sasURL) as any;
FunctionAppService.prototype.get = jest.fn(() => appStub) as any;
unlinkSpy = jest.spyOn(fs, "unlinkSync");
});

afterEach(() => {
mockFs.restore();
unlinkSpy.mockRestore();
jest.resetAllMocks();
});

Expand All @@ -94,6 +98,11 @@ describe("Rollback Service", () => {
});

it("should deploy blob package directly to function app", async () => {
const fsConfig = {};
fsConfig[artifactPath] = "contents";
// Mocking the existence of the downloaded artifact because the downloadBinary
// method won't write to the mock file system
mockFs(fsConfig);
const service = createService();
await service.rollback();
expect(AzureBlobStorageService.prototype.initialize).toBeCalled();
Expand All @@ -107,7 +116,8 @@ describe("Rollback Service", () => {
expect(FunctionAppService.prototype.uploadZippedArfifactToFunctionApp).toBeCalledWith(
appStub,
artifactPath
)
);
expect(unlinkSpy).toBeCalledWith(artifactPath);
});

it("should deploy function app with SAS URL", async () => {
Expand All @@ -133,5 +143,6 @@ describe("Rollback Service", () => {
);
expect(FunctionAppService.prototype.get).not.toBeCalled();
expect(FunctionAppService.prototype.uploadZippedArfifactToFunctionApp).not.toBeCalled();
expect(unlinkSpy).not.toBeCalled();
});
});
4 changes: 4 additions & 0 deletions src/services/rollbackService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { BaseService } from "./baseService";
import { FunctionAppService } from "./functionAppService";
import { ResourceService } from "./resourceService";
import { ArmDeployment } from "../models/armTemplates";
import fs from "fs";

/**
* Services for the Rollback Plugin
Expand Down Expand Up @@ -98,6 +99,9 @@ export class RollbackService extends BaseService {
const functionAppService = new FunctionAppService(this.serverless, this.options);
const functionApp = await functionAppService.get();
await functionAppService.uploadZippedArfifactToFunctionApp(functionApp, artifactPath);
if (fs.existsSync(artifactPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we already cleaning up this folder before deployment? Seems like we should maybe keep this around post deployment so developers can open up the zip more easily to inspect its contents??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind - this is after a rollback - not deployment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think it's good to keep around even for rollback. In case a rollback does not behave as expected, user can verify the deployed content

fs.unlinkSync(artifactPath);
}
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/test/mockFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ArmDeployment, ArmResourceTemplate, ArmTemplateProvisioningState } from
import { ServicePrincipalEnvVariables } from "../models/azureProvider";
import { Logger } from "../models/generic";
import { ServerlessAzureConfig, ServerlessAzureProvider, ServerlessAzureFunctionConfig, ServerlessCliCommand } from "../models/serverless";
import configConstants from "../config";

function getAttribute(object: any, prop: string, defaultValue: any): any {
if (object && object[prop]) {
Expand Down Expand Up @@ -174,7 +175,7 @@ export class MockFactory {
const result = [];
const originalTimestamp = +MockFactory.createTestTimestamp();
for (let i = 0; i < count; i++) {
const name = (includeTimestamp) ? `deployment${i + 1}-t${originalTimestamp + i}` : `deployment${i + 1}`;
const name = (includeTimestamp) ? `${configConstants.naming.suffix.deployment}${i + 1}-t${originalTimestamp + i}` : `deployment${i + 1}`;
result.push(
MockFactory.createTestDeployment(name, i)
)
Expand All @@ -194,7 +195,7 @@ export class MockFactory {

public static createTestDeployment(name?: string, second: number = 0): DeploymentExtended {
return {
name: name || `deployment1-t${MockFactory.createTestTimestamp()}`,
name: name || `${configConstants.naming.suffix.deployment}1-t${MockFactory.createTestTimestamp()}`,
properties: {
timestamp: new Date(2019, 1, 1, 0, 0, second),
parameters: MockFactory.createTestParameters(),
Expand Down