From e051715283f5d794706fe5e175fb0849ee00ed86 Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 04:22:20 +0000 Subject: [PATCH 1/8] Initial changes --- packages/cache/__tests__/restoreCache.test.ts | 8 ++-- packages/cache/__tests__/saveCache.test.ts | 46 +++++++++++------- packages/cache/src/cache.ts | 47 +++++++++++-------- 3 files changed, 61 insertions(+), 40 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index cb3a06c1dc..eb7b44af2f 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -73,14 +73,16 @@ test('restore with no cache found', async () => { test('restore with server error should fail', async () => { const paths = ['node_modules'] const key = 'node-test' + const logWarningMock = jest.spyOn(core, "warning"); jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(() => { throw new Error('HTTP Error Occurred') }) - await expect(restoreCache(paths, key)).rejects.toThrowError( - 'HTTP Error Occurred' - ) + const cacheKey = await restoreCache(paths, key) + expect(cacheKey).toBe(undefined) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to restore: Error: HTTP Error Occurred') }) test('restore with restore keys and no cache found', async () => { diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index 92628f7151..ed370720bd 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -48,6 +48,7 @@ test('save with large cache outputs should fail', async () => { const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, "warning"); const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -57,10 +58,11 @@ test('save with large cache outputs should fail', async () => { const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' - ) + + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.') const archiveFolder = '/foo/bar' @@ -79,6 +81,7 @@ test('save with large cache outputs should fail in GHES with error message', asy const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, "warning"); const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -105,10 +108,11 @@ test('save with large cache outputs should fail in GHES with error message', asy } return response }) - - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'The cache filesize must be between 0 and 1073741824 bytes' - ) + + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: The cache filesize must be between 0 and 1073741824 bytes') const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -127,6 +131,7 @@ test('save with large cache outputs should fail in GHES without error message', const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') + const logWarningMock = jest.spyOn(core, "warning"); const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -150,9 +155,10 @@ test('save with large cache outputs should fail in GHES without error message', return response }) - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' - ) + const cacheId = await saveCache([filePath], primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.') const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -168,6 +174,7 @@ test('save with large cache outputs should fail in GHES without error message', test('save with reserve cache failure should fail', async () => { const paths = ['node_modules'] const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const logWarningMock = jest.spyOn(core, "warning"); const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -187,9 +194,11 @@ test('save with reserve cache failure should fail', async () => { .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - await expect(saveCache(paths, primaryKey)).rejects.toThrowError( - `Unable to reserve cache with key ${primaryKey}, another job may be creating this cache.` - ) + const cacheId = await saveCache(paths, primaryKey) + expect(cacheId).toBe(-1) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: ReserveCacheError: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache.') + expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, paths, { compressionMethod: compression @@ -203,7 +212,7 @@ test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' const cachePaths = [path.resolve(filePath)] - + const logWarningMock = jest.spyOn(core, "warning"); const cacheId = 4 const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -227,10 +236,11 @@ test('save with server error should fail', async () => { const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) + + await saveCache([filePath], primaryKey) + expect(logWarningMock).toHaveBeenCalledTimes(1) + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: HTTP Error Occurred') - await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( - 'HTTP Error Occurred' - ) expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { compressionMethod: compression diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index b11d6310b2..b446bbc449 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -4,6 +4,7 @@ import * as utils from './internal/cacheUtils' import * as cacheHttpClient from './internal/cacheHttpClient' import {createTar, extractTar, listTar} from './internal/tar' import {DownloadOptions, UploadOptions} from './options' +import { ArtifactCacheEntry } from './internal/contracts' export class ValidationError extends Error { constructor(message: string) { @@ -86,23 +87,24 @@ export async function restoreCache( } const compressionMethod = await utils.getCompressionMethod() - - // path are needed to compute version - const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { - compressionMethod - }) - if (!cacheEntry?.archiveLocation) { - // Cache not found - return undefined - } - - const archivePath = path.join( - await utils.createTempDirectory(), - utils.getCacheFileName(compressionMethod) - ) - core.debug(`Archive Path: ${archivePath}`) - + let archivePath = "" try { + // path are needed to compute version + const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod + }) + + if (!cacheEntry?.archiveLocation) { + // Cache not found + return undefined + } + + archivePath = path.join( + await utils.createTempDirectory(), + utils.getCacheFileName(compressionMethod) + ) + core.debug(`Archive Path: ${archivePath}`) + // Download the cache from the cache entry await cacheHttpClient.downloadCache( cacheEntry.archiveLocation, @@ -123,7 +125,12 @@ export async function restoreCache( await extractTar(archivePath, compressionMethod) core.info('Cache restored successfully') - } finally { + + return cacheEntry.cacheKey + } catch(error) { + // Supress all cache related errors because caching should be optional + core.warning(`Fail to restore: ${error}`); + }finally { // Try to delete the archive to save space try { await utils.unlinkFile(archivePath) @@ -132,7 +139,7 @@ export async function restoreCache( } } - return cacheEntry.cacheKey + return undefined } /** @@ -152,7 +159,7 @@ export async function saveCache( checkKey(key) const compressionMethod = await utils.getCompressionMethod() - let cacheId = null + let cacheId = -1 const cachePaths = await utils.resolvePaths(paths) core.debug('Cache Paths:') @@ -217,6 +224,8 @@ export async function saveCache( core.debug(`Saving Cache (ID: ${cacheId})`) await cacheHttpClient.saveCache(cacheId, archivePath, options) + } catch(error) { + core.warning(`Fail to save: ${error}`) } finally { // Try to delete the archive to save space try { From 639c79e4a46964a08887f9e81ef14b06b9a0d977 Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 04:47:59 +0000 Subject: [PATCH 2/8] added info error as well --- packages/cache/__tests__/saveCache.test.ts | 14 +++++++------- packages/cache/src/cache.ts | 7 ++++++- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index ed370720bd..aa03f59789 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -62,7 +62,7 @@ test('save with large cache outputs should fail', async () => { const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.') + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.') const archiveFolder = '/foo/bar' @@ -112,7 +112,7 @@ test('save with large cache outputs should fail in GHES with error message', asy const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: The cache filesize must be between 0 and 1073741824 bytes') + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: The cache filesize must be between 0 and 1073741824 bytes') const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -158,7 +158,7 @@ test('save with large cache outputs should fail in GHES without error message', const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.') + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.') const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -174,7 +174,7 @@ test('save with large cache outputs should fail in GHES without error message', test('save with reserve cache failure should fail', async () => { const paths = ['node_modules'] const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' - const logWarningMock = jest.spyOn(core, "warning"); + const logInfoMock = jest.spyOn(core, "info"); const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -196,8 +196,8 @@ test('save with reserve cache failure should fail', async () => { const cacheId = await saveCache(paths, primaryKey) expect(cacheId).toBe(-1) - expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: ReserveCacheError: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache.') + expect(logInfoMock).toHaveBeenCalledTimes(1) + expect(logInfoMock).toHaveBeenCalledWith(`Fail to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined`) expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, paths, { @@ -239,7 +239,7 @@ test('save with server error should fail', async () => { await saveCache([filePath], primaryKey) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Error: HTTP Error Occurred') + expect(logWarningMock).toHaveBeenCalledWith('Fail to save: HTTP Error Occurred') expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index b446bbc449..be4485389a 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -225,7 +225,12 @@ export async function saveCache( core.debug(`Saving Cache (ID: ${cacheId})`) await cacheHttpClient.saveCache(cacheId, archivePath, options) } catch(error) { - core.warning(`Fail to save: ${error}`) + const typedError = error as Error; + if (typedError.name === ReserveCacheError.name) { + core.info(`Fail to save: ${typedError.message}`) + } else { + core.warning(`Fail to save: ${typedError.message}`) + } } finally { // Try to delete the archive to save space try { From 8c47ec23fed979575437d6b70ebfc9297934e0cc Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 04:54:54 +0000 Subject: [PATCH 3/8] Format --- packages/cache/__tests__/restoreCache.test.ts | 6 ++-- packages/cache/__tests__/saveCache.test.ts | 36 ++++++++++++------- packages/cache/src/cache.ts | 28 +++++++-------- 3 files changed, 41 insertions(+), 29 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index eb7b44af2f..c40d58a33f 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -73,7 +73,7 @@ test('restore with no cache found', async () => { test('restore with server error should fail', async () => { const paths = ['node_modules'] const key = 'node-test' - const logWarningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(core, 'warning') jest.spyOn(cacheHttpClient, 'getCacheEntry').mockImplementation(() => { throw new Error('HTTP Error Occurred') @@ -82,7 +82,9 @@ test('restore with server error should fail', async () => { const cacheKey = await restoreCache(paths, key) expect(cacheKey).toBe(undefined) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to restore: Error: HTTP Error Occurred') + expect(logWarningMock).toHaveBeenCalledWith( + 'Fail to restore: Error: HTTP Error Occurred' + ) }) test('restore with restore keys and no cache found', async () => { diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index aa03f59789..22cfa10d70 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -48,7 +48,7 @@ test('save with large cache outputs should fail', async () => { const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') - const logWarningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -58,11 +58,13 @@ test('save with large cache outputs should fail', async () => { const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - + const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.') + expect(logWarningMock).toHaveBeenCalledWith( + 'Fail to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' + ) const archiveFolder = '/foo/bar' @@ -81,7 +83,7 @@ test('save with large cache outputs should fail in GHES with error message', asy const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') - const logWarningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -108,11 +110,13 @@ test('save with large cache outputs should fail in GHES with error message', asy } return response }) - + const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: The cache filesize must be between 0 and 1073741824 bytes') + expect(logWarningMock).toHaveBeenCalledWith( + 'Fail to save: The cache filesize must be between 0 and 1073741824 bytes' + ) const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -131,7 +135,7 @@ test('save with large cache outputs should fail in GHES without error message', const cachePaths = [path.resolve(filePath)] const createTarMock = jest.spyOn(tar, 'createTar') - const logWarningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(core, 'warning') const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit jest @@ -158,7 +162,9 @@ test('save with large cache outputs should fail in GHES without error message', const cacheId = await saveCache([filePath], primaryKey) expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.') + expect(logWarningMock).toHaveBeenCalledWith( + 'Fail to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' + ) const archiveFolder = '/foo/bar' expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -174,7 +180,7 @@ test('save with large cache outputs should fail in GHES without error message', test('save with reserve cache failure should fail', async () => { const paths = ['node_modules'] const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' - const logInfoMock = jest.spyOn(core, "info"); + const logInfoMock = jest.spyOn(core, 'info') const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -197,7 +203,9 @@ test('save with reserve cache failure should fail', async () => { const cacheId = await saveCache(paths, primaryKey) expect(cacheId).toBe(-1) expect(logInfoMock).toHaveBeenCalledTimes(1) - expect(logInfoMock).toHaveBeenCalledWith(`Fail to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined`) + expect(logInfoMock).toHaveBeenCalledWith( + `Fail to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined` + ) expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, paths, { @@ -212,7 +220,7 @@ test('save with server error should fail', async () => { const filePath = 'node_modules' const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' const cachePaths = [path.resolve(filePath)] - const logWarningMock = jest.spyOn(core, "warning"); + const logWarningMock = jest.spyOn(core, 'warning') const cacheId = 4 const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') @@ -236,10 +244,12 @@ test('save with server error should fail', async () => { const getCompressionMock = jest .spyOn(cacheUtils, 'getCompressionMethod') .mockReturnValueOnce(Promise.resolve(compression)) - + await saveCache([filePath], primaryKey) expect(logWarningMock).toHaveBeenCalledTimes(1) - expect(logWarningMock).toHaveBeenCalledWith('Fail to save: HTTP Error Occurred') + expect(logWarningMock).toHaveBeenCalledWith( + 'Fail to save: HTTP Error Occurred' + ) expect(reserveCacheMock).toHaveBeenCalledTimes(1) expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index be4485389a..0ce62672dc 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -4,7 +4,7 @@ import * as utils from './internal/cacheUtils' import * as cacheHttpClient from './internal/cacheHttpClient' import {createTar, extractTar, listTar} from './internal/tar' import {DownloadOptions, UploadOptions} from './options' -import { ArtifactCacheEntry } from './internal/contracts' +import {ArtifactCacheEntry} from './internal/contracts' export class ValidationError extends Error { constructor(message: string) { @@ -87,10 +87,10 @@ export async function restoreCache( } const compressionMethod = await utils.getCompressionMethod() - let archivePath = "" + let archivePath = '' try { - // path are needed to compute version - const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + // path are needed to compute version + const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { compressionMethod }) @@ -98,13 +98,13 @@ export async function restoreCache( // Cache not found return undefined } - - archivePath = path.join( + + archivePath = path.join( await utils.createTempDirectory(), utils.getCacheFileName(compressionMethod) ) core.debug(`Archive Path: ${archivePath}`) - + // Download the cache from the cache entry await cacheHttpClient.downloadCache( cacheEntry.archiveLocation, @@ -125,12 +125,12 @@ export async function restoreCache( await extractTar(archivePath, compressionMethod) core.info('Cache restored successfully') - + return cacheEntry.cacheKey - } catch(error) { - // Supress all cache related errors because caching should be optional - core.warning(`Fail to restore: ${error}`); - }finally { + } catch (error) { + // Supress all cache related errors because caching should be optional + core.warning(`Fail to restore: ${error}`) + } finally { // Try to delete the archive to save space try { await utils.unlinkFile(archivePath) @@ -224,8 +224,8 @@ export async function saveCache( core.debug(`Saving Cache (ID: ${cacheId})`) await cacheHttpClient.saveCache(cacheId, archivePath, options) - } catch(error) { - const typedError = error as Error; + } catch (error) { + const typedError = error as Error if (typedError.name === ReserveCacheError.name) { core.info(`Fail to save: ${typedError.message}`) } else { From 5008429d2a977181a5ba4ed3152585ec2b0d610a Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 04:58:58 +0000 Subject: [PATCH 4/8] Unused package --- packages/cache/src/cache.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 0ce62672dc..a410cce318 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -4,7 +4,6 @@ import * as utils from './internal/cacheUtils' import * as cacheHttpClient from './internal/cacheHttpClient' import {createTar, extractTar, listTar} from './internal/tar' import {DownloadOptions, UploadOptions} from './options' -import {ArtifactCacheEntry} from './internal/contracts' export class ValidationError extends Error { constructor(message: string) { From 0bde6ed088757158031d68ece682d906979e77bf Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 08:00:59 +0000 Subject: [PATCH 5/8] adding message field --- packages/cache/__tests__/restoreCache.test.ts | 2 +- packages/cache/src/cache.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index c40d58a33f..16c25eb003 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -83,7 +83,7 @@ test('restore with server error should fail', async () => { expect(cacheKey).toBe(undefined) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to restore: Error: HTTP Error Occurred' + 'Fail to restore: HTTP Error Occurred' ) }) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index a410cce318..eafcf90027 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -5,6 +5,7 @@ import * as cacheHttpClient from './internal/cacheHttpClient' import {createTar, extractTar, listTar} from './internal/tar' import {DownloadOptions, UploadOptions} from './options' + export class ValidationError extends Error { constructor(message: string) { super(message) @@ -128,7 +129,7 @@ export async function restoreCache( return cacheEntry.cacheKey } catch (error) { // Supress all cache related errors because caching should be optional - core.warning(`Fail to restore: ${error}`) + core.warning(`Fail to restore: ${(error as Error).message}`) } finally { // Try to delete the archive to save space try { From babb9fb0bc995268919d6fe8f6788d695f05a94c Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 08:06:16 +0000 Subject: [PATCH 6/8] removed line --- packages/cache/src/cache.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index eafcf90027..9cf6a870ba 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -5,7 +5,6 @@ import * as cacheHttpClient from './internal/cacheHttpClient' import {createTar, extractTar, listTar} from './internal/tar' import {DownloadOptions, UploadOptions} from './options' - export class ValidationError extends Error { constructor(message: string) { super(message) From 7b1bc1f56a9ea269d2f9685078a9d7e3ba070c5f Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Mon, 20 Jun 2022 08:48:14 +0000 Subject: [PATCH 7/8] Review comments --- packages/cache/__tests__/restoreCache.test.ts | 2 +- packages/cache/__tests__/saveCache.test.ts | 10 +++++----- packages/cache/src/cache.ts | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 16c25eb003..36ec880129 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -83,7 +83,7 @@ test('restore with server error should fail', async () => { expect(cacheKey).toBe(undefined) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to restore: HTTP Error Occurred' + 'Failed to restore: HTTP Error Occurred' ) }) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index 22cfa10d70..945b254f0b 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -63,7 +63,7 @@ test('save with large cache outputs should fail', async () => { expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' + 'Failed to save: Cache size of ~11264 MB (11811160064 B) is over the 10GB limit, not saving cache.' ) const archiveFolder = '/foo/bar' @@ -115,7 +115,7 @@ test('save with large cache outputs should fail in GHES with error message', asy expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to save: The cache filesize must be between 0 and 1073741824 bytes' + 'Failed to save: The cache filesize must be between 0 and 1073741824 bytes' ) const archiveFolder = '/foo/bar' @@ -163,7 +163,7 @@ test('save with large cache outputs should fail in GHES without error message', expect(cacheId).toBe(-1) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' + 'Failed to save: Cache size of ~11264 MB (11811160064 B) is over the data cap limit, not saving cache.' ) const archiveFolder = '/foo/bar' @@ -204,7 +204,7 @@ test('save with reserve cache failure should fail', async () => { expect(cacheId).toBe(-1) expect(logInfoMock).toHaveBeenCalledTimes(1) expect(logInfoMock).toHaveBeenCalledWith( - `Fail to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined` + `Failed to save: Unable to reserve cache with key ${primaryKey}, another job may be creating this cache. More details: undefined` ) expect(reserveCacheMock).toHaveBeenCalledTimes(1) @@ -248,7 +248,7 @@ test('save with server error should fail', async () => { await saveCache([filePath], primaryKey) expect(logWarningMock).toHaveBeenCalledTimes(1) expect(logWarningMock).toHaveBeenCalledWith( - 'Fail to save: HTTP Error Occurred' + 'Failed to save: HTTP Error Occurred' ) expect(reserveCacheMock).toHaveBeenCalledTimes(1) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 9cf6a870ba..00c9e84e09 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -128,7 +128,7 @@ export async function restoreCache( return cacheEntry.cacheKey } catch (error) { // Supress all cache related errors because caching should be optional - core.warning(`Fail to restore: ${(error as Error).message}`) + core.warning(`Failed to restore: ${(error as Error).message}`) } finally { // Try to delete the archive to save space try { @@ -226,9 +226,9 @@ export async function saveCache( } catch (error) { const typedError = error as Error if (typedError.name === ReserveCacheError.name) { - core.info(`Fail to save: ${typedError.message}`) + core.info(`Failed to save: ${typedError.message}`) } else { - core.warning(`Fail to save: ${typedError.message}`) + core.warning(`Failed to save: ${typedError.message}`) } } finally { // Try to delete the archive to save space From 017007559c76d08d674a5584582403231f802cba Mon Sep 17 00:00:00 2001 From: Shubham Tiwari <64764738+tiwarishub@users.noreply.github.com> Date: Tue, 21 Jun 2022 07:22:33 +0000 Subject: [PATCH 8/8] review comment to add validation as errors handling --- packages/cache/src/cache.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 00c9e84e09..609c7f94cb 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -127,8 +127,13 @@ export async function restoreCache( return cacheEntry.cacheKey } catch (error) { - // Supress all cache related errors because caching should be optional - core.warning(`Failed to restore: ${(error as Error).message}`) + const typedError = error as Error + if (typedError.name === ValidationError.name) { + throw error + } else { + // Supress all non-validation cache related errors because caching should be optional + core.warning(`Failed to restore: ${(error as Error).message}`) + } } finally { // Try to delete the archive to save space try { @@ -225,7 +230,9 @@ export async function saveCache( await cacheHttpClient.saveCache(cacheId, archivePath, options) } catch (error) { const typedError = error as Error - if (typedError.name === ReserveCacheError.name) { + if (typedError.name === ValidationError.name) { + throw error + } else if (typedError.name === ReserveCacheError.name) { core.info(`Failed to save: ${typedError.message}`) } else { core.warning(`Failed to save: ${typedError.message}`)