From 2ea88b1f3208a9fc897a781aa5a242909ddd83bb Mon Sep 17 00:00:00 2001 From: Nick Cipollo Date: Thu, 5 Mar 2020 11:39:02 -0500 Subject: [PATCH] Add retry for uploading artifact (#13) --- __tests__/ArtifactUploader.test.ts | 74 +++++++++++++++++- action.yml | 1 + lib/Action.js | 107 ++++++++++--------------- lib/ArtifactUploader.js | 75 +++++++++--------- lib/Main.js | 29 +++---- lib/Releases.js | 121 ++++++++++++----------------- src/ArtifactUploader.ts | 51 ++++++------ tsconfig.json | 2 +- 8 files changed, 238 insertions(+), 222 deletions(-) diff --git a/__tests__/ArtifactUploader.test.ts b/__tests__/ArtifactUploader.test.ts index bd3c672..cb0780b 100644 --- a/__tests__/ArtifactUploader.test.ts +++ b/__tests__/ArtifactUploader.test.ts @@ -1,6 +1,7 @@ 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'), @@ -32,6 +33,7 @@ describe('ArtifactUploader', () => { it('replaces all artifacts', async () => { mockDeleteSuccess() mockListWithAssets() + mockUploadArtifact() const uploader = createUploader(true) await uploader.uploadArtifacts(artifacts, releaseId, url) @@ -46,10 +48,69 @@ describe('ArtifactUploader', () => { expect(deleteMock).toBeCalledWith(1) expect(deleteMock).toBeCalledWith(2) }) - + it('replaces no artifacts when previous asset list empty', async () => { mockDeleteSuccess() mockListWithoutAssets() + mockUploadArtifact() + const uploader = createUploader(true) + + await uploader.uploadArtifacts(artifacts, releaseId, url) + + expect(uploadMock).toBeCalledTimes(2) + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art2') + + expect(deleteMock).toBeCalledTimes(0) + }) + + it('retry when upload failed with 5xx response', async () => { + mockListWithoutAssets() + mockUploadArtifact(500, 2) + const uploader = createUploader(true) + + await uploader.uploadArtifacts(artifacts, releaseId, url) + + expect(uploadMock).toBeCalledTimes(4) + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art2') + + expect(deleteMock).toBeCalledTimes(0) + }) + + it('abort when upload failed with 5xx response after 3 attemps', async () => { + mockListWithoutAssets() + mockUploadArtifact(500, 4) + const uploader = createUploader(true) + + await uploader.uploadArtifacts(artifacts, releaseId, url) + + expect(uploadMock).toBeCalledTimes(5) + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art2') + expect(uploadMock) + .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art2') + + expect(deleteMock).toBeCalledTimes(0) + }) + + it('abort when upload failed with non-5xx response', async () => { + mockListWithoutAssets() + mockUploadArtifact(401, 2) const uploader = createUploader(true) await uploader.uploadArtifacts(artifacts, releaseId, url) @@ -66,6 +127,7 @@ describe('ArtifactUploader', () => { it('throws error from replace', async () => { mockDeleteError() mockListWithAssets() + mockUploadArtifact() const uploader = createUploader(true) expect.hasAssertions() @@ -79,6 +141,7 @@ describe('ArtifactUploader', () => { it('updates all artifacts, delete none', async () => { mockDeleteError() mockListWithAssets() + mockUploadArtifact() const uploader = createUploader(false) await uploader.uploadArtifacts(artifacts, releaseId, url) @@ -93,7 +156,6 @@ describe('ArtifactUploader', () => { }) function createUploader(replaces: boolean): GithubArtifactUploader { - uploadMock.mockResolvedValue({}) const MockReleases = jest.fn(() => { return { create: jest.fn(), @@ -134,4 +196,12 @@ describe('ArtifactUploader', () => { function mockListWithoutAssets() { listArtifactsMock.mockResolvedValue({ data: [] }) } + + function mockUploadArtifact(status: number = 200, failures: number = 0) { + 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({}) + } }); \ No newline at end of file diff --git a/action.yml b/action.yml index efd44f4..234a926 100644 --- a/action.yml +++ b/action.yml @@ -6,6 +6,7 @@ inputs: description: 'An optional flag which indicates if we should update a release if it already exists. Defaults to 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)' default: '' artifacts: diff --git a/lib/Action.js b/lib/Action.js index 10810a3..50fe4bd 100644 --- a/lib/Action.js +++ b/lib/Action.js @@ -1,13 +1,4 @@ "use strict"; -var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { - function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } - return new (P || (P = Promise))(function (resolve, reject) { - function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } - function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } - function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } - step((generator = generator.apply(thisArg, _arguments || [])).next()); - }); -}; Object.defineProperty(exports, "__esModule", { value: true }); const ErrorMessage_1 = require("./ErrorMessage"); class Action { @@ -16,74 +7,62 @@ class Action { this.releases = releases; this.uploader = uploader; } - perform() { - return __awaiter(this, void 0, void 0, function* () { - const releaseResponse = yield this.createOrUpdateRelease(); - const releaseId = releaseResponse.id; - const uploadUrl = releaseResponse.upload_url; - const artifacts = this.inputs.artifacts; - if (artifacts.length > 0) { - yield this.uploader.uploadArtifacts(artifacts, releaseId, uploadUrl); - } - }); + async perform() { + const releaseResponse = await this.createOrUpdateRelease(); + const releaseId = releaseResponse.id; + const uploadUrl = releaseResponse.upload_url; + const artifacts = this.inputs.artifacts; + if (artifacts.length > 0) { + await this.uploader.uploadArtifacts(artifacts, releaseId, uploadUrl); + } } - createOrUpdateRelease() { - return __awaiter(this, void 0, void 0, function* () { - if (this.inputs.allowUpdates) { - try { - const getResponse = yield this.releases.getByTag(this.inputs.tag); - return yield this.updateRelease(getResponse.data.id); + async createOrUpdateRelease() { + if (this.inputs.allowUpdates) { + try { + const getResponse = await this.releases.getByTag(this.inputs.tag); + return await this.updateRelease(getResponse.data.id); + } + catch (error) { + if (this.noPublishedRelease(error)) { + return await this.updateDraftOrCreateRelease(); } - catch (error) { - if (this.noPublishedRelease(error)) { - return yield this.updateDraftOrCreateRelease(); - } - else { - throw error; - } + else { + throw error; } } - else { - return yield this.createRelease(); - } - }); + } + else { + return await this.createRelease(); + } } - updateRelease(id) { - return __awaiter(this, void 0, void 0, function* () { - const response = yield this.releases.update(id, this.inputs.tag, this.inputs.body, this.inputs.commit, this.inputs.draft, this.inputs.name, this.inputs.prerelease); - return response.data; - }); + async updateRelease(id) { + const response = await this.releases.update(id, this.inputs.tag, this.inputs.body, this.inputs.commit, this.inputs.draft, this.inputs.name, this.inputs.prerelease); + return response.data; } noPublishedRelease(error) { const errorMessage = new ErrorMessage_1.ErrorMessage(error); return errorMessage.status == 404; } - updateDraftOrCreateRelease() { - return __awaiter(this, void 0, void 0, function* () { - const draftReleaseId = yield this.findMatchingDraftReleaseId(); - if (draftReleaseId) { - return yield this.updateRelease(draftReleaseId); - } - else { - return yield this.createRelease(); - } - }); + async updateDraftOrCreateRelease() { + const draftReleaseId = await this.findMatchingDraftReleaseId(); + if (draftReleaseId) { + return await this.updateRelease(draftReleaseId); + } + else { + return await this.createRelease(); + } } - findMatchingDraftReleaseId() { + async findMatchingDraftReleaseId() { var _a; - return __awaiter(this, void 0, void 0, function* () { - const tag = this.inputs.tag; - const response = yield this.releases.listReleases(); - const releases = response.data; - const draftRelease = releases.find(release => release.draft && release.tag_name == tag); - return (_a = draftRelease) === null || _a === void 0 ? void 0 : _a.id; - }); + const tag = this.inputs.tag; + const response = await this.releases.listReleases(); + const releases = response.data; + const draftRelease = releases.find(release => release.draft && release.tag_name == tag); + return (_a = draftRelease) === null || _a === void 0 ? void 0 : _a.id; } - createRelease() { - return __awaiter(this, void 0, void 0, function* () { - const response = yield this.releases.create(this.inputs.tag, this.inputs.body, this.inputs.commit, this.inputs.draft, this.inputs.name, this.inputs.prerelease); - return response.data; - }); + async createRelease() { + const response = await this.releases.create(this.inputs.tag, this.inputs.body, this.inputs.commit, this.inputs.draft, this.inputs.name, this.inputs.prerelease); + return response.data; } } exports.Action = Action; diff --git a/lib/ArtifactUploader.js b/lib/ArtifactUploader.js index 5ef5276..f8b94a2 100644 --- a/lib/ArtifactUploader.js +++ b/lib/ArtifactUploader.js @@ -1,13 +1,4 @@ "use strict"; -var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { - function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } - return new (P || (P = Promise))(function (resolve, reject) { - function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } - function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } - function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } - step((generator = generator.apply(thisArg, _arguments || [])).next()); - }); -}; var __importStar = (this && this.__importStar) || function (mod) { if (mod && mod.__esModule) return mod; var result = {}; @@ -18,43 +9,47 @@ var __importStar = (this && this.__importStar) || function (mod) { Object.defineProperty(exports, "__esModule", { value: true }); const core = __importStar(require("@actions/core")); class GithubArtifactUploader { - constructor(releases, replacesExistingArtifacts) { - this.replacesExistingArtifacts = true; + constructor(releases, replacesExistingArtifacts = true) { this.releases = releases; this.replacesExistingArtifacts = replacesExistingArtifacts; } - uploadArtifacts(artifacts, releaseId, uploadUrl) { - return __awaiter(this, void 0, void 0, function* () { - if (this.replacesExistingArtifacts) { - yield this.deleteUpdatedArtifacts(artifacts, releaseId); - } - for (const artifact of artifacts) { - try { - yield this.releases.uploadArtifact(uploadUrl, artifact.contentLength, artifact.contentType, artifact.readFile(), artifact.name); - } - catch (error) { - const message = `Failed to upload artifact ${artifact.name}. Does it already exist?`; - core.warning(message); - } - } - return Promise.resolve(); - }); + async uploadArtifacts(artifacts, releaseId, uploadUrl) { + if (this.replacesExistingArtifacts) { + await this.deleteUpdatedArtifacts(artifacts, releaseId); + } + for (const artifact of artifacts) { + await this.uploadArtifact(artifact, uploadUrl); + } } - deleteUpdatedArtifacts(artifacts, releaseId) { - return __awaiter(this, void 0, void 0, function* () { - const response = yield this.releases.listArtifactsForRelease(releaseId); - const releaseAssets = response.data; - const assetByName = new Map(); - releaseAssets.forEach(asset => { - assetByName[asset.name] = asset; - }); - for (const artifact of artifacts) { - const asset = assetByName[artifact.name]; - if (asset) { - yield this.releases.deleteArtifact(asset.id); - } + async uploadArtifact(artifact, uploadUrl, retry = 3) { + try { + core.debug(`Uploading artifact ${artifact.name}...`); + await this.releases.uploadArtifact(uploadUrl, artifact.contentLength, artifact.contentType, artifact.readFile(), artifact.name); + } + catch (error) { + if (error.status >= 500 && retry > 0) { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}. Retrying...`); + await this.uploadArtifact(artifact, uploadUrl, retry - 1); } + else { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`); + } + } + } + async deleteUpdatedArtifacts(artifacts, releaseId) { + const response = await this.releases.listArtifactsForRelease(releaseId); + const releaseAssets = response.data; + const assetByName = {}; + releaseAssets.forEach(asset => { + assetByName[asset.name] = asset; }); + for (const artifact of artifacts) { + const asset = assetByName[artifact.name]; + if (asset) { + core.debug(`Deleting exist artifact ${artifact.name}...`); + await this.releases.deleteArtifact(asset.id); + } + } } } exports.GithubArtifactUploader = GithubArtifactUploader; diff --git a/lib/Main.js b/lib/Main.js index 57903ff..52546f0 100644 --- a/lib/Main.js +++ b/lib/Main.js @@ -1,13 +1,4 @@ "use strict"; -var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { - function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } - return new (P || (P = Promise))(function (resolve, reject) { - function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } - function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } - function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } - step((generator = generator.apply(thisArg, _arguments || [])).next()); - }); -}; var __importStar = (this && this.__importStar) || function (mod) { if (mod && mod.__esModule) return mod; var result = {}; @@ -24,17 +15,15 @@ const Action_1 = require("./Action"); const ArtifactUploader_1 = require("./ArtifactUploader"); const ArtifactGlobber_1 = require("./ArtifactGlobber"); const ErrorMessage_1 = require("./ErrorMessage"); -function run() { - return __awaiter(this, void 0, void 0, function* () { - try { - const action = createAction(); - yield action.perform(); - } - catch (error) { - const errorMessage = new ErrorMessage_1.ErrorMessage(error); - core.setFailed(errorMessage.toString()); - } - }); +async function run() { + try { + const action = createAction(); + await action.perform(); + } + catch (error) { + const errorMessage = new ErrorMessage_1.ErrorMessage(error); + core.setFailed(errorMessage.toString()); + } } function createAction() { const token = core.getInput('token'); diff --git a/lib/Releases.js b/lib/Releases.js index 77b7097..dde6328 100644 --- a/lib/Releases.js +++ b/lib/Releases.js @@ -1,94 +1,71 @@ "use strict"; -var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { - function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } - return new (P || (P = Promise))(function (resolve, reject) { - function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } - function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } - function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } - step((generator = generator.apply(thisArg, _arguments || [])).next()); - }); -}; Object.defineProperty(exports, "__esModule", { value: true }); class GithubReleases { constructor(context, git) { this.context = context; this.git = git; } - create(tag, body, commitHash, draft, name, prerelease) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.createRelease({ - body: body, - name: name, - draft: draft, - owner: this.context.repo.owner, - prerelease: prerelease, - repo: this.context.repo.repo, - target_commitish: commitHash, - tag_name: tag - }); + async create(tag, body, commitHash, draft, name, prerelease) { + return this.git.repos.createRelease({ + body: body, + name: name, + draft: draft, + owner: this.context.repo.owner, + prerelease: prerelease, + repo: this.context.repo.repo, + target_commitish: commitHash, + tag_name: tag }); } - deleteArtifact(assetId) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.deleteReleaseAsset({ - asset_id: assetId, - owner: this.context.repo.owner, - repo: this.context.repo.repo - }); + async deleteArtifact(assetId) { + return this.git.repos.deleteReleaseAsset({ + asset_id: assetId, + owner: this.context.repo.owner, + repo: this.context.repo.repo }); } - listArtifactsForRelease(releaseId) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.listAssetsForRelease({ - owner: this.context.repo.owner, - release_id: releaseId, - repo: this.context.repo.repo - }); + async listArtifactsForRelease(releaseId) { + return this.git.repos.listAssetsForRelease({ + owner: this.context.repo.owner, + release_id: releaseId, + repo: this.context.repo.repo }); } - listReleases() { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.listReleases({ - owner: this.context.repo.owner, - repo: this.context.repo.repo - }); + async listReleases() { + return this.git.repos.listReleases({ + owner: this.context.repo.owner, + repo: this.context.repo.repo }); } - getByTag(tag) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.getReleaseByTag({ - owner: this.context.repo.owner, - repo: this.context.repo.repo, - tag: tag - }); + async getByTag(tag) { + return this.git.repos.getReleaseByTag({ + owner: this.context.repo.owner, + repo: this.context.repo.repo, + tag: tag }); } - update(id, tag, body, commitHash, draft, name, prerelease) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.updateRelease({ - release_id: id, - body: body, - name: name, - draft: draft, - owner: this.context.repo.owner, - prerelease: prerelease, - repo: this.context.repo.repo, - target_commitish: commitHash, - tag_name: tag - }); + async update(id, tag, body, commitHash, draft, name, prerelease) { + return this.git.repos.updateRelease({ + release_id: id, + body: body, + name: name, + draft: draft, + owner: this.context.repo.owner, + prerelease: prerelease, + repo: this.context.repo.repo, + target_commitish: commitHash, + tag_name: tag }); } - uploadArtifact(assetUrl, contentLength, contentType, file, name) { - return __awaiter(this, void 0, void 0, function* () { - return this.git.repos.uploadReleaseAsset({ - url: assetUrl, - headers: { - "content-length": contentLength, - "content-type": contentType - }, - file: file, - name: name - }); + async uploadArtifact(assetUrl, contentLength, contentType, file, name) { + return this.git.repos.uploadReleaseAsset({ + url: assetUrl, + headers: { + "content-length": contentLength, + "content-type": contentType + }, + file: file, + name: name }); } } diff --git a/src/ArtifactUploader.ts b/src/ArtifactUploader.ts index 9cd0b28..5273155 100644 --- a/src/ArtifactUploader.ts +++ b/src/ArtifactUploader.ts @@ -2,53 +2,58 @@ import * as core from '@actions/core'; import { Artifact } from "./Artifact"; import { Releases } from "./Releases"; import { ReposListAssetsForReleaseResponseItem } from "@octokit/rest"; -import { ErrorMessage } from './ErrorMessage'; export interface ArtifactUploader { uploadArtifacts(artifacts: Artifact[], releaseId: number, uploadUrl: string): Promise } export class GithubArtifactUploader implements ArtifactUploader { - private releases: Releases - private replacesExistingArtifacts: boolean = true - - constructor(releases: Releases, replacesExistingArtifacts: boolean) { - this.releases = releases - this.replacesExistingArtifacts = replacesExistingArtifacts + constructor( + private releases: Releases, + private replacesExistingArtifacts: boolean = true, + ) { } async uploadArtifacts(artifacts: Artifact[], releaseId: number, uploadUrl: string): Promise { - if(this.replacesExistingArtifacts) { + if (this.replacesExistingArtifacts) { await this.deleteUpdatedArtifacts(artifacts, releaseId) } - for (const artifact of artifacts) { - try { - await this.releases.uploadArtifact(uploadUrl, - artifact.contentLength, - artifact.contentType, - artifact.readFile(), - artifact.name) - } catch (error) { - const message = `Failed to upload artifact ${artifact.name}. Does it already exist?` - core.warning(message) - } + await this.uploadArtifact(artifact, uploadUrl) } - return Promise.resolve() } - async deleteUpdatedArtifacts(artifacts: Artifact[], releaseId: number) { - const response = await this.releases.listArtifactsForRelease(releaseId) + private async uploadArtifact(artifact: Artifact, uploadUrl: string, retry = 3) { + try { + core.debug(`Uploading artifact ${artifact.name}...`) + await this.releases.uploadArtifact(uploadUrl, + artifact.contentLength, + artifact.contentType, + artifact.readFile(), + artifact.name) + } catch (error) { + if (error.status >= 500 && retry > 0) { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}. Retrying...`) + await this.uploadArtifact(artifact, uploadUrl, retry - 1) + } else { + core.warning(`Failed to upload artifact ${artifact.name}. ${error.message}.`) + } + } + } + + private async deleteUpdatedArtifacts(artifacts: Artifact[], releaseId: number): Promise { + const response = await this.releases.listArtifactsForRelease(releaseId) const releaseAssets = response.data - const assetByName = new Map() + const assetByName: Record = {} releaseAssets.forEach(asset => { assetByName[asset.name] = asset }); for (const artifact of artifacts) { const asset = assetByName[artifact.name] if (asset) { + core.debug(`Deleting exist artifact ${artifact.name}...`) await this.releases.deleteArtifact(asset.id) } } diff --git a/tsconfig.json b/tsconfig.json index 960dc9f..2570c60 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -2,7 +2,7 @@ "compilerOptions": { /* Basic Options */ // "incremental": true, /* Enable incremental compilation */ - "target": "es6", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */ + "target": "es2019", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */ "module": "commonjs", /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ // "allowJs": true, /* Allow javascript files to be compiled. */ // "checkJs": true, /* Report errors in .js files. */