From af980963d65cd9a7f15cba8a8d51850023e34df7 Mon Sep 17 00:00:00 2001 From: Nick Cipollo Date: Sun, 21 Mar 2021 19:05:15 -0400 Subject: [PATCH] Fixes #33 Add artifactErrorsFailBuild flag --- README.md | 1 + __tests__/Action.test.ts | 2 ++ __tests__/ArtifactGlobber.test.ts | 30 ++++++++++++--------- __tests__/ArtifactUploader.test.ts | 43 +++++++++++++++++++++++------- __tests__/Inputs.test.ts | 35 ++++++++++++++++++++---- __tests__/Integration.test.ts | 11 +++++--- action.yml | 4 +++ lib/Action.js | 3 +-- lib/ArtifactGlobber.js | 16 ++++++++--- lib/ArtifactUploader.js | 10 +++++-- lib/Inputs.js | 6 ++++- src/Action.ts | 4 +-- src/ArtifactGlobber.ts | 18 +++++++++---- src/ArtifactUploader.ts | 7 ++++- src/Inputs.ts | 8 +++++- 15 files changed, 149 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 25fd017..a570d9b 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,7 @@ This action will create a github release and optionally upload an artifact to it ## Action Inputs - **allowUpdates**: An optional flag which indicates if we should update a release if it already exists. Defaults to false. +- **artifactErrorsFailBuild**: An optional flag which indicates if artifact read or upload errors should fail the build. - **artifact**: An optional set of paths representing artifacts to upload to the release. This may be a single path or a comma delimited list of paths (or globs). - **artifacts**: An optional set of paths representing artifacts to upload to the release. This may be a single path or a comma delimited list of paths (or globs). - **artifactContentType**: The content type of the artifact. Defaults to raw. diff --git a/__tests__/Action.test.ts b/__tests__/Action.test.ts index 4b47a48..6351c07 100644 --- a/__tests__/Action.test.ts +++ b/__tests__/Action.test.ts @@ -17,6 +17,7 @@ const artifacts = [ new Artifact('b/art2') ] +const artifactErrorsFailBuild = false const createBody = 'createBody' const createName = 'createName' const commit = 'commit' @@ -239,6 +240,7 @@ describe("Action", () => { const MockInputs = jest.fn(() => { return { allowUpdates: allowUpdates, + artifactErrorsFailBuild: true, artifacts: inputArtifact, createdReleaseBody: createBody, createdReleaseName: createName, diff --git a/__tests__/ArtifactGlobber.test.ts b/__tests__/ArtifactGlobber.test.ts index 81530e9..7e510da 100644 --- a/__tests__/ArtifactGlobber.test.ts +++ b/__tests__/ArtifactGlobber.test.ts @@ -1,8 +1,8 @@ const warnMock = jest.fn() -import { FileArtifactGlobber } from "../src/ArtifactGlobber" -import { Globber } from "../src/Globber"; -import { Artifact } from "../src/Artifact"; +import {FileArtifactGlobber} from "../src/ArtifactGlobber" +import {Globber} from "../src/Globber"; +import {Artifact} from "../src/Artifact"; import untildify = require("untildify"); const contentType = "raw" @@ -24,19 +24,19 @@ describe("ArtifactGlobber", () => { const expectedArtifacts = globResults.map((path) => new Artifact(path, contentType)) - expect(globber.globArtifactString('~/path', 'raw')) + expect(globber.globArtifactString('~/path', 'raw', false)) .toEqual(expectedArtifacts) expect(globMock).toBeCalledWith(untildify('~/path')) expect(warnMock).not.toBeCalled() }) - + it("globs simple path", () => { const globber = createArtifactGlobber() const expectedArtifacts = globResults.map((path) => new Artifact(path, contentType)) - - expect(globber.globArtifactString('path', 'raw')) + + expect(globber.globArtifactString('path', 'raw', false)) .toEqual(expectedArtifacts) expect(globMock).toBeCalledWith('path') expect(warnMock).not.toBeCalled() @@ -50,24 +50,28 @@ describe("ArtifactGlobber", () => { .concat(globResults) .map((path) => new Artifact(path, contentType)) - expect(globber.globArtifactString('path1,path2', 'raw')) + expect(globber.globArtifactString('path1,path2', 'raw', false)) .toEqual(expectedArtifacts) expect(globMock).toBeCalledWith('path1') expect(globMock).toBeCalledWith('path2') expect(warnMock).not.toBeCalled() }) - it("warns when no glob results are produced", () => { + it("warns when no glob results are produced and empty results shouldn't throw", () => { const globber = createArtifactGlobber([]) - const expectedArtifacts = - globResults.map((path) => new Artifact(path, contentType)) - - expect(globber.globArtifactString('path', 'raw')) + expect(globber.globArtifactString('path', 'raw', false)) .toEqual([]) expect(warnMock).toBeCalled() }) + it("throws when no glob results are produced and empty results shouild throw", () => { + const globber = createArtifactGlobber([]) + expect(() => { + globber.globArtifactString('path', 'raw', true) + }).toThrow() + }) + function createArtifactGlobber(results: string[] = globResults): FileArtifactGlobber { const MockGlobber = jest.fn(() => { return { diff --git a/__tests__/ArtifactUploader.test.ts b/__tests__/ArtifactUploader.test.ts index 7f59ba7..b6d47a1 100644 --- a/__tests__/ArtifactUploader.test.ts +++ b/__tests__/ArtifactUploader.test.ts @@ -1,7 +1,7 @@ -import { Artifact } from "../src/Artifact" -import { GithubArtifactUploader } from "../src/ArtifactUploader" -import { Releases } from "../src/Releases"; -import { RequestError } from '@octokit/request-error' +import {Artifact} from "../src/Artifact" +import {GithubArtifactUploader} from "../src/ArtifactUploader" +import {Releases} from "../src/Releases"; +import {RequestError} from '@octokit/request-error' const artifacts = [ new Artifact('a/art1'), @@ -19,7 +19,9 @@ const uploadMock = jest.fn() jest.mock('fs', () => { return { readFileSync: () => fileContents, - statSync: () => { return { size: contentLength } } + statSync: () => { + return {size: contentLength} + } }; }) @@ -124,6 +126,19 @@ describe('ArtifactUploader', () => { expect(deleteMock).toBeCalledTimes(0) }) + it('throws upload error when replacesExistingArtifacts is true', async () => { + mockListWithoutAssets() + mockUploadError() + const uploader = createUploader(true, true) + + expect.hasAssertions() + try { + await uploader.uploadArtifacts(artifacts, releaseId, url) + } catch (error) { + expect(error).toEqual(Error("Failed to upload artifact art1. error.")) + } + }) + it('throws error from replace', async () => { mockDeleteError() mockListWithAssets() @@ -155,7 +170,7 @@ describe('ArtifactUploader', () => { expect(deleteMock).toBeCalledTimes(0) }) - function createUploader(replaces: boolean): GithubArtifactUploader { + function createUploader(replaces: boolean, throws: boolean = false): GithubArtifactUploader { const MockReleases = jest.fn(() => { return { create: jest.fn(), @@ -167,7 +182,7 @@ describe('ArtifactUploader', () => { uploadArtifact: uploadMock } }) - return new GithubArtifactUploader(new MockReleases(), replaces) + return new GithubArtifactUploader(new MockReleases(), replaces, throws) } function mockDeleteError(): any { @@ -194,14 +209,24 @@ describe('ArtifactUploader', () => { } function mockListWithoutAssets() { - listArtifactsMock.mockResolvedValue({ data: [] }) + listArtifactsMock.mockResolvedValue({data: []}) } function mockUploadArtifact(status: number = 200, failures: number = 0) { - const error = new RequestError(`HTTP ${status}`, status, { headers: {}, request: { method: 'GET', url: '', headers: {} } }) + const error = new RequestError(`HTTP ${status}`, status, { + headers: {}, + request: {method: 'GET', url: '', headers: {}} + }) for (let index = 0; index < failures; index++) { uploadMock.mockRejectedValueOnce(error) } uploadMock.mockResolvedValue({}) } + + function mockUploadError() { + uploadMock.mockRejectedValue({ + message: "error", + status: 502 + }) + } }); diff --git a/__tests__/Inputs.test.ts b/__tests__/Inputs.test.ts index 7b915f4..eb350be 100644 --- a/__tests__/Inputs.test.ts +++ b/__tests__/Inputs.test.ts @@ -59,7 +59,28 @@ describe('Inputs', () => { }) }) + describe('artifactErrorsFailBuild', () => { + it('returns false', () => { + expect(inputs.artifactErrorsFailBuild).toBe(false) + }) + + it('returns true', () => { + mockGetInput.mockReturnValue('true') + expect(inputs.artifactErrorsFailBuild).toBe(true) + }) + }) + describe('artifacts', () => { + it('globber told to throw errors', () => { + mockGetInput.mockReturnValueOnce('art1') + .mockReturnValueOnce('contentType') + .mockReturnValueOnce('true') + + expect(inputs.artifacts).toEqual(artifacts) + expect(mockGlob).toBeCalledTimes(1) + expect(mockGlob).toBeCalledWith('art1', 'contentType', true) + }) + it('returns empty artifacts', () => { mockGetInput.mockReturnValueOnce('') .mockReturnValueOnce('') @@ -71,28 +92,32 @@ describe('Inputs', () => { it('returns input.artifacts', () => { mockGetInput.mockReturnValueOnce('art1') .mockReturnValueOnce('contentType') + .mockReturnValueOnce('false') expect(inputs.artifacts).toEqual(artifacts) expect(mockGlob).toBeCalledTimes(1) - expect(mockGlob).toBeCalledWith('art1', 'contentType') + expect(mockGlob).toBeCalledWith('art1', 'contentType', false) }) it('returns input.artifacts with default contentType', () => { mockGetInput.mockReturnValueOnce('art1') + .mockReturnValueOnce('raw') + .mockReturnValueOnce('false') expect(inputs.artifacts).toEqual(artifacts) expect(mockGlob).toBeCalledTimes(1) - expect(mockGlob).toBeCalledWith('art1', 'raw') + expect(mockGlob).toBeCalledWith('art1', 'raw', false) }) it('returns input.artifact', () => { mockGetInput.mockReturnValueOnce('') .mockReturnValueOnce('art2') .mockReturnValueOnce('contentType') + .mockReturnValueOnce('false') expect(inputs.artifacts).toEqual(artifacts) expect(mockGlob).toBeCalledTimes(1) - expect(mockGlob).toBeCalledWith('art2', 'contentType') + expect(mockGlob).toBeCalledWith('art2', 'contentType', false) }) }) @@ -164,7 +189,7 @@ describe('Inputs', () => { expect(inputs.draft).toBe(true) }) }) - + describe('owner', () => { it('returns owner from context', function () { process.env.GITHUB_REPOSITORY = "owner/repo" @@ -175,7 +200,7 @@ describe('Inputs', () => { mockGetInput.mockReturnValue("owner") expect(inputs.owner).toBe("owner") }); - }) + }) describe('prerelase', () => { it('returns false', () => { diff --git a/__tests__/Integration.test.ts b/__tests__/Integration.test.ts index 63c0a4c..16c2063 100644 --- a/__tests__/Integration.test.ts +++ b/__tests__/Integration.test.ts @@ -18,7 +18,11 @@ describe.skip('Integration Test', () => { const inputs = getInputs() const releases = new GithubReleases(inputs, git) - const uploader = new GithubArtifactUploader(releases, inputs.replacesArtifacts) + const uploader = new GithubArtifactUploader( + releases, + inputs.artifactErrorsFailBuild, + inputs.replacesArtifacts + ) action = new Action(inputs, releases, uploader) }) @@ -30,6 +34,7 @@ describe.skip('Integration Test', () => { const MockInputs = jest.fn(() => { return { allowUpdates: true, + artifactErrorsFailBuild: false, artifacts: artifacts(), createdReleaseBody: "This release was generated by release-action's integration test", createdReleaseName: "Releases Action Integration Test", @@ -47,12 +52,12 @@ describe.skip('Integration Test', () => { }) return new MockInputs(); } - + function artifacts() { const globber = new FileArtifactGlobber() const artifactPath = path.join(__dirname, 'Integration.test.ts') const artifactString = `~/Desktop/test.txt,blarg.tx, ${artifactPath}` - return globber.globArtifactString(artifactString, "raw") + return globber.globArtifactString(artifactString, "raw", false) } function getToken(): string { diff --git a/action.yml b/action.yml index 2e251ea..5c86176 100644 --- a/action.yml +++ b/action.yml @@ -6,6 +6,10 @@ inputs: description: 'An optional flag which indicates if we should update a release if it already exists. Defaults to false.' required: false default: '' + artifactErrorsFailBuild: + description: 'An optional flag which indicates if artifact read or upload errors should fail the build.' + required: false + default: '' artifact: deprecationMessage: Use 'artifacts' instead. description: 'An optional set of paths representing artifacts to upload to the release. This may be a single path or a comma delimited list of paths (or globs)' diff --git a/lib/Action.js b/lib/Action.js index ad938a7..00d4b2f 100644 --- a/lib/Action.js +++ b/lib/Action.js @@ -80,8 +80,7 @@ class Action { } createRelease() { return __awaiter(this, void 0, void 0, function* () { - const response = yield this.releases.create(this.inputs.tag, this.inputs.createdReleaseBody, this.inputs.commit, this.inputs.draft, this.inputs.createdReleaseName, this.inputs.prerelease); - return response; + return yield this.releases.create(this.inputs.tag, this.inputs.createdReleaseBody, this.inputs.commit, this.inputs.draft, this.inputs.createdReleaseName, this.inputs.prerelease); }); } } diff --git a/lib/ArtifactGlobber.js b/lib/ArtifactGlobber.js index e83e91a..5eda18e 100644 --- a/lib/ArtifactGlobber.js +++ b/lib/ArtifactGlobber.js @@ -31,23 +31,31 @@ class FileArtifactGlobber { constructor(globber = new Globber_1.FileGlobber()) { this.globber = globber; } - globArtifactString(artifact, contentType) { + globArtifactString(artifact, contentType, throwsWhenNoFiles) { return artifact.split(',') .map(path => FileArtifactGlobber.expandPath(path)) - .map(pattern => this.globPattern(pattern)) + .map(pattern => this.globPattern(pattern, throwsWhenNoFiles)) .reduce((accumulated, current) => accumulated.concat(current)) .map(path => new Artifact_1.Artifact(path, contentType)); } - globPattern(pattern) { + globPattern(pattern, throwsWhenNoFiles) { const paths = this.globber.glob(pattern); if (paths.length == 0) { - FileArtifactGlobber.reportGlobWarning(pattern); + if (throwsWhenNoFiles) { + FileArtifactGlobber.throwGlobError(pattern); + } + else { + FileArtifactGlobber.reportGlobWarning(pattern); + } } return paths; } static reportGlobWarning(pattern) { core.warning(`Artifact pattern :${pattern} did not match any files`); } + static throwGlobError(pattern) { + throw Error(`Artifact pattern :${pattern} did not match any files`); + } static expandPath(path) { return untildify_1.default(path); } diff --git a/lib/ArtifactUploader.js b/lib/ArtifactUploader.js index 91c2415..fa44c2f 100644 --- a/lib/ArtifactUploader.js +++ b/lib/ArtifactUploader.js @@ -31,9 +31,10 @@ Object.defineProperty(exports, "__esModule", { value: true }); exports.GithubArtifactUploader = void 0; const core = __importStar(require("@actions/core")); class GithubArtifactUploader { - constructor(releases, replacesExistingArtifacts = true) { + constructor(releases, replacesExistingArtifacts = true, throwsUploadErrors = false) { this.releases = releases; this.replacesExistingArtifacts = replacesExistingArtifacts; + this.throwsUploadErrors = throwsUploadErrors; } uploadArtifacts(artifacts, releaseId, uploadUrl) { return __awaiter(this, void 0, void 0, function* () { @@ -57,7 +58,12 @@ class GithubArtifactUploader { yield this.uploadArtifact(artifact, releaseId, uploadUrl, retry - 1); } else { - core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`); + if (this.throwsUploadErrors) { + throw Error(`Failed to upload artifact ${artifact.name}. ${error.message}.`); + } + else { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`); + } } } }); diff --git a/lib/Inputs.js b/lib/Inputs.js index a822502..0f7e8c2 100644 --- a/lib/Inputs.js +++ b/lib/Inputs.js @@ -42,10 +42,14 @@ class CoreInputs { contentType = 'raw'; } return this.artifactGlobber - .globArtifactString(artifacts, contentType); + .globArtifactString(artifacts, contentType, this.artifactErrorsFailBuild); } return []; } + get artifactErrorsFailBuild() { + const allow = core.getInput('artifactErrorsFailBuild'); + return allow == 'true'; + } get createdReleaseBody() { if (CoreInputs.omitBody) return undefined; diff --git a/src/Action.ts b/src/Action.ts index 89eb853..a8ba2b2 100644 --- a/src/Action.ts +++ b/src/Action.ts @@ -78,7 +78,7 @@ export class Action { } private async createRelease(): Promise { - const response = await this.releases.create( + return await this.releases.create( this.inputs.tag, this.inputs.createdReleaseBody, this.inputs.commit, @@ -86,7 +86,5 @@ export class Action { this.inputs.createdReleaseName, this.inputs.prerelease ) - - return response } } diff --git a/src/ArtifactGlobber.ts b/src/ArtifactGlobber.ts index 365e91d..19b7a41 100644 --- a/src/ArtifactGlobber.ts +++ b/src/ArtifactGlobber.ts @@ -4,7 +4,7 @@ import {Artifact} from "./Artifact"; import untildify from "untildify"; export interface ArtifactGlobber { - globArtifactString(artifact: string, contentType: string): Artifact[] + globArtifactString(artifact: string, contentType: string, throwsWhenNoFiles: boolean): Artifact[] } export class FileArtifactGlobber implements ArtifactGlobber { @@ -14,18 +14,22 @@ export class FileArtifactGlobber implements ArtifactGlobber { this.globber = globber } - globArtifactString(artifact: string, contentType: string): Artifact[] { + globArtifactString(artifact: string, contentType: string, throwsWhenNoFiles: boolean): Artifact[] { return artifact.split(',') .map(path => FileArtifactGlobber.expandPath(path)) - .map(pattern => this.globPattern(pattern)) + .map(pattern => this.globPattern(pattern, throwsWhenNoFiles)) .reduce((accumulated, current) => accumulated.concat(current)) .map(path => new Artifact(path, contentType)) } - private globPattern(pattern: string): string[] { + private globPattern(pattern: string, throwsWhenNoFiles: boolean): string[] { const paths = this.globber.glob(pattern) if (paths.length == 0) { - FileArtifactGlobber.reportGlobWarning(pattern) + if (throwsWhenNoFiles) { + FileArtifactGlobber.throwGlobError(pattern) + } else { + FileArtifactGlobber.reportGlobWarning(pattern) + } } return paths } @@ -34,6 +38,10 @@ export class FileArtifactGlobber implements ArtifactGlobber { core.warning(`Artifact pattern :${pattern} did not match any files`) } + private static throwGlobError(pattern: string) { + throw Error(`Artifact pattern :${pattern} did not match any files`) + } + private static expandPath(path: string): string { return untildify(path) } diff --git a/src/ArtifactUploader.ts b/src/ArtifactUploader.ts index 33420e6..a214815 100644 --- a/src/ArtifactUploader.ts +++ b/src/ArtifactUploader.ts @@ -10,6 +10,7 @@ export class GithubArtifactUploader implements ArtifactUploader { constructor( private releases: Releases, private replacesExistingArtifacts: boolean = true, + private throwsUploadErrors: boolean = false, ) { } @@ -41,7 +42,11 @@ export class GithubArtifactUploader implements ArtifactUploader { core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}. Retrying...`) await this.uploadArtifact(artifact, releaseId, uploadUrl, retry - 1) } else { - core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`) + if (this.throwsUploadErrors) { + throw Error(`Failed to upload artifact ${artifact.name}. ${error.message}.`) + } else { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`) + } } } } diff --git a/src/Inputs.ts b/src/Inputs.ts index 98a0983..1bc1614 100644 --- a/src/Inputs.ts +++ b/src/Inputs.ts @@ -6,6 +6,7 @@ import {Artifact} from './Artifact'; export interface Inputs { readonly allowUpdates: boolean + readonly artifactErrorsFailBuild: boolean readonly artifacts: Artifact[] readonly commit: string readonly createdReleaseBody?: string @@ -46,11 +47,16 @@ export class CoreInputs implements Inputs { contentType = 'raw' } return this.artifactGlobber - .globArtifactString(artifacts, contentType) + .globArtifactString(artifacts, contentType, this.artifactErrorsFailBuild) } return [] } + get artifactErrorsFailBuild(): boolean { + const allow = core.getInput('artifactErrorsFailBuild') + return allow == 'true' + } + get createdReleaseBody(): string | undefined { if (CoreInputs.omitBody) return undefined return this.body