From 1dae85574671bf3939d2b9c524f1a5cfc3ca8e58 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 6 Dec 2022 11:38:17 +0000 Subject: [PATCH 1/5] Add fallback to gzip compression if cache not found --- packages/cache/src/cache.ts | 39 +++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 609c7f94..d29eeda2 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -4,6 +4,8 @@ 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 {CompressionMethod} from './internal/constants' +import {ArtifactCacheEntry} from './internal/contracts' export class ValidationError extends Error { constructor(message: string) { @@ -85,17 +87,38 @@ export async function restoreCache( checkKey(key) } - const compressionMethod = await utils.getCompressionMethod() + let cacheEntry: ArtifactCacheEntry | null + let compressionMethod = await utils.getCompressionMethod() let archivePath = '' try { - // path are needed to compute version - const cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { - compressionMethod - }) + try { + // path are needed to compute version + cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod + }) - if (!cacheEntry?.archiveLocation) { - // Cache not found - return undefined + if (!cacheEntry?.archiveLocation) { + // Cache not found + return undefined + } + } catch (error) { + if ( + process.platform == 'win32' && + compressionMethod != CompressionMethod.Gzip + ) { + // On windows, we will try to download the cache entry with the same key + // but with different compression method. This is to support the old cache entry created + // by the old version of the cache action. + compressionMethod = CompressionMethod.Gzip + cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod + }) + if (!cacheEntry?.archiveLocation) { + throw error + } + } else { + throw error + } } archivePath = path.join( From c0085d773988fba7addf6c7f87f97b0a27d47b7f Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Tue, 6 Dec 2022 11:52:46 +0000 Subject: [PATCH 2/5] Fix test --- packages/cache/src/cache.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index d29eeda2..0efaecfe 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -103,8 +103,8 @@ export async function restoreCache( } } catch (error) { if ( - process.platform == 'win32' && - compressionMethod != CompressionMethod.Gzip + process.platform === 'win32' && + compressionMethod !== CompressionMethod.Gzip ) { // On windows, we will try to download the cache entry with the same key // but with different compression method. This is to support the old cache entry created From d1094e15235370f00f7bf3ad8250970003935107 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Wed, 7 Dec 2022 07:39:41 +0000 Subject: [PATCH 3/5] Add test --- packages/cache/__tests__/restoreCache.test.ts | 75 +++++++++++++++++++ packages/cache/src/cache.ts | 5 +- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 36ec8801..2c9b4525 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -161,6 +161,81 @@ test('restore with gzip compressed cache found', async () => { expect(getCompressionMock).toHaveBeenCalledTimes(1) }) +test('restore with zstd as default but gzip compressed cache found on windows', async () => { + if (process.platform === 'win32') { + const paths = ['node_modules'] + const key = 'node-test' + + const cacheEntry: ArtifactCacheEntry = { + cacheKey: key, + scope: 'refs/heads/main', + archiveLocation: 'www.actionscache.test/download' + } + const getCacheMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') + getCacheMock + .mockImplementationOnce(async () => { + return Promise.resolve({}) + }) + .mockImplementationOnce(async () => { + return Promise.resolve(cacheEntry) + }) + + const tempPath = '/foo/bar' + + const createTempDirectoryMock = jest.spyOn( + cacheUtils, + 'createTempDirectory' + ) + createTempDirectoryMock.mockImplementation(async () => { + return Promise.resolve(tempPath) + }) + + const archivePath = path.join(tempPath, CacheFilename.Gzip) + const downloadCacheMock = jest.spyOn(cacheHttpClient, 'downloadCache') + + const fileSize = 142 + const getArchiveFileSizeInBytesMock = jest + .spyOn(cacheUtils, 'getArchiveFileSizeInBytes') + .mockReturnValue(fileSize) + + const extractTarMock = jest.spyOn(tar, 'extractTar') + const unlinkFileMock = jest.spyOn(cacheUtils, 'unlinkFile') + + const compression = CompressionMethod.Zstd + const getCompressionMock = jest + .spyOn(cacheUtils, 'getCompressionMethod') + .mockReturnValue(Promise.resolve(compression)) + + const cacheKey = await restoreCache(paths, key) + + expect(cacheKey).toBe(key) + expect(getCacheMock).toHaveBeenNthCalledWith(1, [key], paths, { + compressionMethod: compression + }) + expect(getCacheMock).toHaveBeenNthCalledWith(2, [key], paths, { + compressionMethod: CompressionMethod.Gzip + }) + expect(createTempDirectoryMock).toHaveBeenCalledTimes(1) + expect(downloadCacheMock).toHaveBeenCalledWith( + cacheEntry.archiveLocation, + archivePath, + undefined + ) + expect(getArchiveFileSizeInBytesMock).toHaveBeenCalledWith(archivePath) + + expect(extractTarMock).toHaveBeenCalledTimes(1) + expect(extractTarMock).toHaveBeenCalledWith( + archivePath, + CompressionMethod.Gzip + ) + + expect(unlinkFileMock).toHaveBeenCalledTimes(1) + expect(unlinkFileMock).toHaveBeenCalledWith(archivePath) + + expect(getCompressionMock).toHaveBeenCalledTimes(1) + } +}) + test('restore with zstd compressed cache found', async () => { const paths = ['node_modules'] const key = 'node-test' diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index 0efaecfe..c0d0f85a 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -106,9 +106,8 @@ export async function restoreCache( process.platform === 'win32' && compressionMethod !== CompressionMethod.Gzip ) { - // On windows, we will try to download the cache entry with the same key - // but with different compression method. This is to support the old cache entry created - // by the old version of the cache action. + // This is to support the old cache entry created + // by the old version of the cache action on windows. compressionMethod = CompressionMethod.Gzip cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { compressionMethod From d79a09bc0e4f1dde4d6ebd0ad455f01cad5f1321 Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Wed, 7 Dec 2022 08:27:08 +0000 Subject: [PATCH 4/5] Address review comments --- packages/cache/__tests__/restoreCache.test.ts | 2 +- packages/cache/src/cache.ts | 31 ++++++------------- .../cache/src/internal/cacheHttpClient.ts | 26 ++++++++++++++++ 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/packages/cache/__tests__/restoreCache.test.ts b/packages/cache/__tests__/restoreCache.test.ts index 2c9b4525..82b259b9 100644 --- a/packages/cache/__tests__/restoreCache.test.ts +++ b/packages/cache/__tests__/restoreCache.test.ts @@ -174,7 +174,7 @@ test('restore with zstd as default but gzip compressed cache found on windows', const getCacheMock = jest.spyOn(cacheHttpClient, 'getCacheEntry') getCacheMock .mockImplementationOnce(async () => { - return Promise.resolve({}) + throw new Error('Cache not found.') }) .mockImplementationOnce(async () => { return Promise.resolve(cacheEntry) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index c0d0f85a..b2185e5e 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -96,30 +96,19 @@ export async function restoreCache( cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { compressionMethod }) - - if (!cacheEntry?.archiveLocation) { - // Cache not found - return undefined - } } catch (error) { - if ( - process.platform === 'win32' && - compressionMethod !== CompressionMethod.Gzip - ) { - // This is to support the old cache entry created - // by the old version of the cache action on windows. - compressionMethod = CompressionMethod.Gzip - cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { - compressionMethod - }) - if (!cacheEntry?.archiveLocation) { - throw error - } - } else { - throw error - } + cacheEntry = await cacheHttpClient.getCacheEntryForGzipFallbackOnWindows( + keys, + paths, + compressionMethod, + error + ) } + if (!cacheEntry?.archiveLocation) { + // Cache not found + return undefined + } archivePath = path.join( await utils.createTempDirectory(), utils.getCacheFileName(compressionMethod) diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index c66d1a73..04882612 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -122,6 +122,32 @@ export async function getCacheEntry( return cacheResult } +// This is to support the old cache entry created +// by the old version of the cache action on windows. +export async function getCacheEntryForGzipFallbackOnWindows( + keys: string[], + paths: string[], + compressionMethod: CompressionMethod, + error: unknown +): Promise { + let cacheEntry: ArtifactCacheEntry | null + if ( + process.platform === 'win32' && + compressionMethod !== CompressionMethod.Gzip + ) { + compressionMethod = CompressionMethod.Gzip + cacheEntry = await getCacheEntry(keys, paths, { + compressionMethod + }) + if (!cacheEntry?.archiveLocation) { + throw error + } + } else { + throw error + } + return cacheEntry +} + export async function downloadCache( archiveLocation: string, archivePath: string, From 27f9a7d4619bde4df165846e435d9e2795ab3bea Mon Sep 17 00:00:00 2001 From: Sampark Sharma Date: Wed, 7 Dec 2022 08:41:56 +0000 Subject: [PATCH 5/5] Revert Address review comments --- packages/cache/src/cache.ts | 22 +++++++++++----- .../cache/src/internal/cacheHttpClient.ts | 26 ------------------- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/packages/cache/src/cache.ts b/packages/cache/src/cache.ts index b2185e5e..3840e439 100644 --- a/packages/cache/src/cache.ts +++ b/packages/cache/src/cache.ts @@ -97,12 +97,22 @@ export async function restoreCache( compressionMethod }) } catch (error) { - cacheEntry = await cacheHttpClient.getCacheEntryForGzipFallbackOnWindows( - keys, - paths, - compressionMethod, - error - ) + // This is to support the old cache entry created + // by the old version of the cache action on windows. + if ( + process.platform === 'win32' && + compressionMethod !== CompressionMethod.Gzip + ) { + compressionMethod = CompressionMethod.Gzip + cacheEntry = await cacheHttpClient.getCacheEntry(keys, paths, { + compressionMethod + }) + if (!cacheEntry?.archiveLocation) { + throw error + } + } else { + throw error + } } if (!cacheEntry?.archiveLocation) { diff --git a/packages/cache/src/internal/cacheHttpClient.ts b/packages/cache/src/internal/cacheHttpClient.ts index 04882612..c66d1a73 100644 --- a/packages/cache/src/internal/cacheHttpClient.ts +++ b/packages/cache/src/internal/cacheHttpClient.ts @@ -122,32 +122,6 @@ export async function getCacheEntry( return cacheResult } -// This is to support the old cache entry created -// by the old version of the cache action on windows. -export async function getCacheEntryForGzipFallbackOnWindows( - keys: string[], - paths: string[], - compressionMethod: CompressionMethod, - error: unknown -): Promise { - let cacheEntry: ArtifactCacheEntry | null - if ( - process.platform === 'win32' && - compressionMethod !== CompressionMethod.Gzip - ) { - compressionMethod = CompressionMethod.Gzip - cacheEntry = await getCacheEntry(keys, paths, { - compressionMethod - }) - if (!cacheEntry?.archiveLocation) { - throw error - } - } else { - throw error - } - return cacheEntry -} - export async function downloadCache( archiveLocation: string, archivePath: string,