From f8a69bc473af4a204d0c03de61d5c9d1300dfb17 Mon Sep 17 00:00:00 2001 From: Deepak Dahiya <59823596+t-dedah@users.noreply.github.com> Date: Mon, 4 Apr 2022 16:21:58 +0530 Subject: [PATCH] Added cacheSize in ReserveCache API request (#1044) * Added cacheSize in ReserveCache API request * minor * minor * minor * Cleanup * package-lock revert * Modified tests * New Response Type * cleanup * Linting * Lint fix * Resolved comments * Added tests * package-lock * Resolved package-lock mismatch * Liniting * Update packages/cache/src/cache.ts Co-authored-by: Bishal Prasad * Linting issue * Resolved few comments * version upgrade * Savecache tests * RequestUtil test * test * test * test * test * test * test * test * test * test * test Co-authored-by: Apple Co-authored-by: Bishal Prasad --- packages/cache/__tests__/requestUtils.test.ts | 34 ++++- packages/cache/__tests__/saveCache.test.ts | 130 ++++++++++++++++-- packages/cache/package-lock.json | 2 +- packages/cache/package.json | 2 +- packages/cache/src/cache.ts | 42 ++++-- .../cache/src/internal/cacheHttpClient.ts | 10 +- packages/cache/src/internal/cacheUtils.ts | 7 + packages/cache/src/internal/contracts.d.ts | 8 ++ packages/cache/src/internal/requestUtils.ts | 15 +- 9 files changed, 209 insertions(+), 41 deletions(-) diff --git a/packages/cache/__tests__/requestUtils.test.ts b/packages/cache/__tests__/requestUtils.test.ts index 7509d04d..05fc573b 100644 --- a/packages/cache/__tests__/requestUtils.test.ts +++ b/packages/cache/__tests__/requestUtils.test.ts @@ -1,5 +1,6 @@ -import {retry} from '../src/internal/requestUtils' +import {retry, retryTypedResponse} from '../src/internal/requestUtils' import {HttpClientError} from '@actions/http-client' +import * as requestUtils from '../src/internal/requestUtils' interface ITestResponse { statusCode: number @@ -145,3 +146,34 @@ test('retry converts errors to response object', async () => { null ) }) + +test('retryTypedResponse gives an error with error message', async () => { + const httpClientError = new HttpClientError( + 'The cache filesize must be between 0 and 10 * 1024 * 1024 bytes', + 400 + ) + jest.spyOn(requestUtils, 'retry').mockReturnValue( + new Promise(resolve => { + resolve(httpClientError) + }) + ) + try { + await retryTypedResponse( + 'reserveCache', + async () => + new Promise(resolve => { + resolve({ + statusCode: 400, + result: '', + headers: {}, + error: httpClientError + }) + }) + ) + } catch (error) { + expect(error).toHaveProperty( + 'message', + 'The cache filesize must be between 0 and 10 * 1024 * 1024 bytes' + ) + } +}) diff --git a/packages/cache/__tests__/saveCache.test.ts b/packages/cache/__tests__/saveCache.test.ts index d9dd5708..6949759b 100644 --- a/packages/cache/__tests__/saveCache.test.ts +++ b/packages/cache/__tests__/saveCache.test.ts @@ -5,6 +5,12 @@ import * as cacheHttpClient from '../src/internal/cacheHttpClient' import * as cacheUtils from '../src/internal/cacheUtils' import {CacheFilename, CompressionMethod} from '../src/internal/constants' import * as tar from '../src/internal/tar' +import {ITypedResponse} from '@actions/http-client/interfaces' +import { + ReserveCacheResponse, + ITypedResponseWithError +} from '../src/internal/contracts' +import {HttpClientError} from '@actions/http-client' jest.mock('../src/internal/cacheHttpClient') jest.mock('../src/internal/cacheUtils') @@ -16,16 +22,13 @@ beforeAll(() => { jest.spyOn(core, 'info').mockImplementation(() => {}) jest.spyOn(core, 'warning').mockImplementation(() => {}) jest.spyOn(core, 'error').mockImplementation(() => {}) - jest.spyOn(cacheUtils, 'getCacheFileName').mockImplementation(cm => { const actualUtils = jest.requireActual('../src/internal/cacheUtils') return actualUtils.getCacheFileName(cm) }) - jest.spyOn(cacheUtils, 'resolvePaths').mockImplementation(async filePaths => { return filePaths.map(x => path.resolve(x)) }) - jest.spyOn(cacheUtils, 'createTempDirectory').mockImplementation(async () => { return Promise.resolve('/foo/bar') }) @@ -70,6 +73,98 @@ test('save with large cache outputs should fail', async () => { expect(getCompressionMock).toHaveBeenCalledTimes(1) }) +test('save with large cache outputs should fail in GHES with error message', async () => { + const filePath = 'node_modules' + const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const cachePaths = [path.resolve(filePath)] + + const createTarMock = jest.spyOn(tar, 'createTar') + + const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit + jest + .spyOn(cacheUtils, 'getArchiveFileSizeInBytes') + .mockReturnValueOnce(cacheSize) + const compression = CompressionMethod.Gzip + const getCompressionMock = jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValueOnce(Promise.resolve(compression)) + + jest.spyOn(cacheUtils, 'isGhes').mockReturnValueOnce(true) + + const reserveCacheMock = jest + .spyOn(cacheHttpClient, 'reserveCache') + .mockImplementation(async () => { + const response: ITypedResponseWithError = { + statusCode: 400, + result: null, + headers: {}, + error: new HttpClientError( + 'The cache filesize must be between 0 and 1073741824 bytes', + 400 + ) + } + return response + }) + + await expect(saveCache([filePath], primaryKey)).rejects.toThrowError( + 'The cache filesize must be between 0 and 1073741824 bytes' + ) + + const archiveFolder = '/foo/bar' + expect(reserveCacheMock).toHaveBeenCalledTimes(1) + expect(createTarMock).toHaveBeenCalledTimes(1) + expect(createTarMock).toHaveBeenCalledWith( + archiveFolder, + cachePaths, + compression + ) + expect(getCompressionMock).toHaveBeenCalledTimes(1) +}) + +test('save with large cache outputs should fail in GHES without error message', async () => { + const filePath = 'node_modules' + const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' + const cachePaths = [path.resolve(filePath)] + + const createTarMock = jest.spyOn(tar, 'createTar') + + const cacheSize = 11 * 1024 * 1024 * 1024 //~11GB, over the 10GB limit + jest + .spyOn(cacheUtils, 'getArchiveFileSizeInBytes') + .mockReturnValueOnce(cacheSize) + const compression = CompressionMethod.Gzip + const getCompressionMock = jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValueOnce(Promise.resolve(compression)) + + jest.spyOn(cacheUtils, 'isGhes').mockReturnValueOnce(true) + + const reserveCacheMock = jest + .spyOn(cacheHttpClient, 'reserveCache') + .mockImplementation(async () => { + const response: ITypedResponseWithError = { + statusCode: 400, + result: null, + headers: {} + } + 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 archiveFolder = '/foo/bar' + expect(reserveCacheMock).toHaveBeenCalledTimes(1) + expect(createTarMock).toHaveBeenCalledTimes(1) + expect(createTarMock).toHaveBeenCalledWith( + archiveFolder, + cachePaths, + compression + ) + expect(getCompressionMock).toHaveBeenCalledTimes(1) +}) + test('save with reserve cache failure should fail', async () => { const paths = ['node_modules'] const primaryKey = 'Linux-node-bb828da54c148048dd17899ba9fda624811cfb43' @@ -77,7 +172,12 @@ test('save with reserve cache failure should fail', async () => { const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') .mockImplementation(async () => { - return -1 + const response: ITypedResponse = { + statusCode: 500, + result: null, + headers: {} + } + return response }) const createTarMock = jest.spyOn(tar, 'createTar') @@ -94,7 +194,7 @@ test('save with reserve cache failure should fail', async () => { expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, paths, { compressionMethod: compression }) - expect(createTarMock).toHaveBeenCalledTimes(0) + expect(createTarMock).toHaveBeenCalledTimes(1) expect(saveCacheMock).toHaveBeenCalledTimes(0) expect(getCompressionMock).toHaveBeenCalledTimes(1) }) @@ -108,7 +208,12 @@ test('save with server error should fail', async () => { const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') .mockImplementation(async () => { - return cacheId + const response: ITypedResponse = { + statusCode: 500, + result: {cacheId}, + headers: {} + } + return response }) const createTarMock = jest.spyOn(tar, 'createTar') @@ -130,17 +235,14 @@ test('save with server error should fail', async () => { expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { compressionMethod: compression }) - const archiveFolder = '/foo/bar' const archiveFile = path.join(archiveFolder, CacheFilename.Zstd) - expect(createTarMock).toHaveBeenCalledTimes(1) expect(createTarMock).toHaveBeenCalledWith( archiveFolder, cachePaths, compression ) - expect(saveCacheMock).toHaveBeenCalledTimes(1) expect(saveCacheMock).toHaveBeenCalledWith(cacheId, archiveFile, undefined) expect(getCompressionMock).toHaveBeenCalledTimes(1) @@ -155,7 +257,12 @@ test('save with valid inputs uploads a cache', async () => { const reserveCacheMock = jest .spyOn(cacheHttpClient, 'reserveCache') .mockImplementation(async () => { - return cacheId + const response: ITypedResponse = { + statusCode: 500, + result: {cacheId}, + headers: {} + } + return response }) const createTarMock = jest.spyOn(tar, 'createTar') @@ -171,17 +278,14 @@ test('save with valid inputs uploads a cache', async () => { expect(reserveCacheMock).toHaveBeenCalledWith(primaryKey, [filePath], { compressionMethod: compression }) - const archiveFolder = '/foo/bar' const archiveFile = path.join(archiveFolder, CacheFilename.Zstd) - expect(createTarMock).toHaveBeenCalledTimes(1) expect(createTarMock).toHaveBeenCalledWith( archiveFolder, cachePaths, compression ) - expect(saveCacheMock).toHaveBeenCalledTimes(1) expect(saveCacheMock).toHaveBeenCalledWith(cacheId, archiveFile, undefined) expect(getCompressionMock).toHaveBeenCalledTimes(1) diff --git a/packages/cache/package-lock.json b/packages/cache/package-lock.json index 48516b94..3017263a 100644 --- a/packages/cache/package-lock.json +++ b/packages/cache/package-lock.json @@ -1,6 +1,6 @@ { "name": "@actions/cache", - "version": "2.0.0", + "version": "2.0.1", "lockfileVersion": 2, "requires": true, "packages": { diff --git a/packages/cache/package.json b/packages/cache/package.json index 3cfbae1a..6ae7b318 100644 --- a/packages/cache/package.json +++ b/packages/cache/package.json @@ -1,6 +1,6 @@ { "name": "@actions/cache", - "version": "2.0.0", + "version": "2.0.1", "preview": true, "description": "Actions cache lib", "keywords": [ diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 9356e838..01d49f9f 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -152,17 +152,7 @@ export async function saveCache( checkKey(key) const compressionMethod = await utils.getCompressionMethod() - - core.debug('Reserving Cache') - const cacheId = await cacheHttpClient.reserveCache(key, paths, { - compressionMethod - }) - if (cacheId === -1) { - throw new ReserveCacheError( - `Unable to reserve cache with key ${key}, another job may be creating this cache.` - ) - } - core.debug(`Cache ID: ${cacheId}`) + let cacheId = null const cachePaths = await utils.resolvePaths(paths) core.debug('Cache Paths:') @@ -181,11 +171,12 @@ export async function saveCache( if (core.isDebug()) { await listTar(archivePath, compressionMethod) } - const fileSizeLimit = 10 * 1024 * 1024 * 1024 // 10GB per repo limit const archiveFileSize = utils.getArchiveFileSizeInBytes(archivePath) core.debug(`File Size: ${archiveFileSize}`) - if (archiveFileSize > fileSizeLimit) { + + // For GHES, this check will take place in ReserveCache API with enterprise file size limit + if (archiveFileSize > fileSizeLimit && !utils.isGhes()) { throw new Error( `Cache size of ~${Math.round( archiveFileSize / (1024 * 1024) @@ -193,6 +184,31 @@ export async function saveCache( ) } + core.debug('Reserving Cache') + const reserveCacheResponse = await cacheHttpClient.reserveCache( + key, + paths, + { + compressionMethod, + cacheSize: archiveFileSize + } + ) + + if (reserveCacheResponse?.result?.cacheId) { + cacheId = reserveCacheResponse?.result?.cacheId + } else if (reserveCacheResponse?.statusCode === 400) { + throw new ReserveCacheError( + reserveCacheResponse?.error?.message ?? + `Cache size of ~${Math.round( + archiveFileSize / (1024 * 1024) + )} MB (${archiveFileSize} B) is over the data cap limit, not saving cache.` + ) + } else { + throw new ReserveCacheError( + `Unable to reserve cache with key ${key}, another job may be creating this cache. More details: ${reserveCacheResponse?.error?.message}` + ) + } + core.debug(`Saving Cache (ID: ${cacheId})`) await cacheHttpClient.saveCache(cacheId, archivePath, options) } finally { diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index 65d7bd08..21f69917 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -13,7 +13,8 @@ import { InternalCacheOptions, CommitCacheRequest, ReserveCacheRequest, - ReserveCacheResponse + ReserveCacheResponse, + ITypedResponseWithError } from './contracts' import {downloadCacheHttpClient, downloadCacheStorageSDK} from './downloadUtils' import { @@ -143,13 +144,14 @@ export async function reserveCache( key: string, paths: string[], options?: InternalCacheOptions -): Promise { +): Promise> { const httpClient = createHttpClient() const version = getCacheVersion(paths, options?.compressionMethod) const reserveCacheRequest: ReserveCacheRequest = { key, - version + version, + cacheSize: options?.cacheSize } const response = await retryTypedResponse('reserveCache', async () => httpClient.postJson( @@ -157,7 +159,7 @@ export async function reserveCache( reserveCacheRequest ) ) - return response?.result?.cacheId ?? -1 + return response } function getContentRange(start: number, end: number): string { diff --git a/packages/cache/src/internal/cacheUtils.ts b/packages/cache/src/internal/cacheUtils.ts index 41096102..9c1035a9 100644 --- a/packages/cache/src/internal/cacheUtils.ts +++ b/packages/cache/src/internal/cacheUtils.ts @@ -123,3 +123,10 @@ export function assertDefined(name: string, value?: T): T { return value } + +export function isGhes(): boolean { + const ghUrl = new URL( + process.env['GITHUB_SERVER_URL'] || 'https://github.com' + ) + return ghUrl.hostname.toUpperCase() !== 'GITHUB.COM' +} diff --git a/packages/cache/src/internal/contracts.d.ts b/packages/cache/src/internal/contracts.d.ts index 80484769..eb79fdae 100644 --- a/packages/cache/src/internal/contracts.d.ts +++ b/packages/cache/src/internal/contracts.d.ts @@ -1,4 +1,10 @@ import {CompressionMethod} from './constants' +import {ITypedResponse} from '@actions/http-client/interfaces' +import {HttpClientError} from '@actions/http-client' + +export interface ITypedResponseWithError extends ITypedResponse { + error?: HttpClientError +} export interface ArtifactCacheEntry { cacheKey?: string @@ -14,6 +20,7 @@ export interface CommitCacheRequest { export interface ReserveCacheRequest { key: string version?: string + cacheSize?: number } export interface ReserveCacheResponse { @@ -22,4 +29,5 @@ export interface ReserveCacheResponse { export interface InternalCacheOptions { compressionMethod?: CompressionMethod + cacheSize?: number } diff --git a/packages/cache/src/internal/requestUtils.ts b/packages/cache/src/internal/requestUtils.ts index 2a8ba93c..be254b93 100644 --- a/packages/cache/src/internal/requestUtils.ts +++ b/packages/cache/src/internal/requestUtils.ts @@ -1,10 +1,8 @@ import * as core from '@actions/core' import {HttpCodes, HttpClientError} from '@actions/http-client' -import { - IHttpClientResponse, - ITypedResponse -} from '@actions/http-client/interfaces' +import {IHttpClientResponse} from '@actions/http-client/interfaces' import {DefaultRetryDelay, DefaultRetryAttempts} from './constants' +import {ITypedResponseWithError} from './contracts' export function isSuccessStatusCode(statusCode?: number): boolean { if (!statusCode) { @@ -94,14 +92,14 @@ export async function retry( export async function retryTypedResponse( name: string, - method: () => Promise>, + method: () => Promise>, maxAttempts = DefaultRetryAttempts, delay = DefaultRetryDelay -): Promise> { +): Promise> { return await retry( name, method, - (response: ITypedResponse) => response.statusCode, + (response: ITypedResponseWithError) => response.statusCode, maxAttempts, delay, // If the error object contains the statusCode property, extract it and return @@ -111,7 +109,8 @@ export async function retryTypedResponse( return { statusCode: error.statusCode, result: null, - headers: {} + headers: {}, + error } } else { return undefined