From f9082773141cd5755aa408d3ce081c5c2261a723 Mon Sep 17 00:00:00 2001 From: Danny Gleckler Date: Thu, 8 Feb 2024 20:07:45 -0500 Subject: [PATCH 1/5] Fix save-always/post-if with an output --- __tests__/restore.test.ts | 24 +++++++++++++++++++---- __tests__/restoreImpl.test.ts | 36 +++++++++++++++++++++++++++++------ __tests__/restoreOnly.test.ts | 9 ++++++--- action.yml | 4 +++- src/constants.ts | 6 ++++-- src/restoreImpl.ts | 2 ++ 6 files changed, 65 insertions(+), 16 deletions(-) diff --git a/__tests__/restore.test.ts b/__tests__/restore.test.ts index 250f7ef..ff09552 100644 --- a/__tests__/restore.test.ts +++ b/__tests__/restore.test.ts @@ -173,8 +173,12 @@ test("restore with cache found for key", async () => { expect(stateMock).toHaveBeenCalledWith("CACHE_RESULT", key); expect(stateMock).toHaveBeenCalledTimes(2); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); expect(failedMock).toHaveBeenCalledTimes(0); @@ -218,8 +222,12 @@ test("restore with cache found for restore key", async () => { expect(stateMock).toHaveBeenCalledWith("CACHE_RESULT", restoreKey); expect(stateMock).toHaveBeenCalledTimes(2); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` ); @@ -260,7 +268,11 @@ test("Fail restore when fail on cache miss is enabled and primary + restore keys ); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(0); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(failedMock).toHaveBeenCalledWith( `Failed to restore cache entry. Exiting as fail-on-cache-miss is set. Input key: ${key}` @@ -306,8 +318,12 @@ test("restore when fail on cache miss is enabled and primary key doesn't match r expect(stateMock).toHaveBeenCalledWith("CACHE_RESULT", restoreKey); expect(stateMock).toHaveBeenCalledTimes(2); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` diff --git a/__tests__/restoreImpl.test.ts b/__tests__/restoreImpl.test.ts index 8bab894..5992473 100644 --- a/__tests__/restoreImpl.test.ts +++ b/__tests__/restoreImpl.test.ts @@ -79,8 +79,12 @@ test("restore without AC available should no-op", async () => { await restoreImpl(new StateProvider()); expect(restoreCacheMock).toHaveBeenCalledTimes(0); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); }); test("restore on GHES without AC available should no-op", async () => { @@ -95,8 +99,12 @@ test("restore on GHES without AC available should no-op", async () => { await restoreImpl(new StateProvider()); expect(restoreCacheMock).toHaveBeenCalledTimes(0); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); }); test("restore on GHES with AC available ", async () => { @@ -133,8 +141,12 @@ test("restore on GHES with AC available ", async () => { ); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); expect(failedMock).toHaveBeenCalledTimes(0); @@ -355,8 +367,12 @@ test("restore with cache found for key", async () => { ); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); expect(failedMock).toHaveBeenCalledTimes(0); @@ -397,8 +413,12 @@ test("restore with cache found for restore key", async () => { ); expect(stateMock).toHaveBeenCalledWith("CACHE_KEY", key); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` ); @@ -441,8 +461,12 @@ test("restore with lookup-only set", async () => { expect(stateMock).toHaveBeenCalledWith("CACHE_RESULT", key); expect(stateMock).toHaveBeenCalledTimes(2); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(2); expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "" + ); expect(infoMock).toHaveBeenCalledWith( `Cache found and can be restored from key: ${key}` diff --git a/__tests__/restoreOnly.test.ts b/__tests__/restoreOnly.test.ts index 81e5bca..40875ec 100644 --- a/__tests__/restoreOnly.test.ts +++ b/__tests__/restoreOnly.test.ts @@ -86,7 +86,8 @@ test("restore with no cache found", async () => { ); expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); - expect(outputMock).toHaveBeenCalledTimes(1); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); + expect(outputMock).toHaveBeenCalledTimes(2); expect(failedMock).toHaveBeenCalledTimes(0); expect(infoMock).toHaveBeenCalledWith( @@ -169,8 +170,9 @@ test("restore with cache found for key", async () => { expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); expect(outputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(outputMock).toHaveBeenCalledWith("cache-matched-key", key); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); - expect(outputMock).toHaveBeenCalledTimes(3); + expect(outputMock).toHaveBeenCalledTimes(4); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); expect(failedMock).toHaveBeenCalledTimes(0); @@ -212,8 +214,9 @@ test("restore with cache found for restore key", async () => { expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); expect(outputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(outputMock).toHaveBeenCalledWith("cache-matched-key", restoreKey); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); - expect(outputMock).toHaveBeenCalledTimes(3); + expect(outputMock).toHaveBeenCalledTimes(4); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` diff --git a/action.yml b/action.yml index 0125281..2b51484 100644 --- a/action.yml +++ b/action.yml @@ -33,11 +33,13 @@ inputs: outputs: cache-hit: description: 'A boolean value to indicate an exact match was found for the primary key' + save-always-d18d746b9: + description: "Run the post step to save the cache even if another step before fails" runs: using: 'node20' main: 'dist/restore/index.js' post: 'dist/save/index.js' - post-if: "success() || github.event.inputs.save-always" + post-if: "success() || (contains(steps.*.outputs.save-always-d18d746b9, 'true') && !contains(steps.*.outputs.save-always-d18d746b9, 'false'))" branding: icon: 'archive' color: 'gray-dark' diff --git a/src/constants.ts b/src/constants.ts index 0158ae0..d61d0cd 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -5,13 +5,15 @@ export enum Inputs { UploadChunkSize = "upload-chunk-size", // Input for cache, save action EnableCrossOsArchive = "enableCrossOsArchive", // Input for cache, restore, save action FailOnCacheMiss = "fail-on-cache-miss", // Input for cache, restore action - LookupOnly = "lookup-only" // Input for cache, restore action + LookupOnly = "lookup-only", // Input for cache, restore action + SaveAlways = "save-always" // Input for cache action } export enum Outputs { CacheHit = "cache-hit", // Output from cache, restore action CachePrimaryKey = "cache-primary-key", // Output from restore action - CacheMatchedKey = "cache-matched-key" // Output from restore action + CacheMatchedKey = "cache-matched-key", // Output from restore action + SaveAlways = "save-always-d18d746b9" // Output from cache action, with unique suffix for detection in post-if } export enum State { diff --git a/src/restoreImpl.ts b/src/restoreImpl.ts index 0aff57a..98fbbde 100644 --- a/src/restoreImpl.ts +++ b/src/restoreImpl.ts @@ -12,6 +12,8 @@ import * as utils from "./utils/actionUtils"; export async function restoreImpl( stateProvider: IStateProvider ): Promise { + core.setOutput(Outputs.SaveAlways, core.getInput(Inputs.SaveAlways)); + try { if (!utils.isCacheFeatureAvailable()) { core.setOutput(Outputs.CacheHit, "false"); From 727a4d213743f2cb4da3d5cd287dc2ba4bfbc985 Mon Sep 17 00:00:00 2001 From: Danny Gleckler Date: Thu, 8 Feb 2024 20:20:05 -0500 Subject: [PATCH 2/5] Always output explicit "false" to prevent hijacking --- __tests__/restore.test.ts | 8 ++++---- __tests__/restoreImpl.test.ts | 12 ++++++------ __tests__/restoreOnly.test.ts | 6 +++--- src/restoreImpl.ts | 5 ++++- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/__tests__/restore.test.ts b/__tests__/restore.test.ts index ff09552..1cfb246 100644 --- a/__tests__/restore.test.ts +++ b/__tests__/restore.test.ts @@ -177,7 +177,7 @@ test("restore with cache found for key", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); @@ -226,7 +226,7 @@ test("restore with cache found for restore key", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` @@ -271,7 +271,7 @@ test("Fail restore when fail on cache miss is enabled and primary + restore keys expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(failedMock).toHaveBeenCalledWith( @@ -322,7 +322,7 @@ test("restore when fail on cache miss is enabled and primary key doesn't match r expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith( diff --git a/__tests__/restoreImpl.test.ts b/__tests__/restoreImpl.test.ts index 5992473..88662a3 100644 --- a/__tests__/restoreImpl.test.ts +++ b/__tests__/restoreImpl.test.ts @@ -83,7 +83,7 @@ test("restore without AC available should no-op", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); }); @@ -103,7 +103,7 @@ test("restore on GHES without AC available should no-op", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); }); @@ -145,7 +145,7 @@ test("restore on GHES with AC available ", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); @@ -371,7 +371,7 @@ test("restore with cache found for key", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith(`Cache restored from key: ${key}`); @@ -417,7 +417,7 @@ test("restore with cache found for restore key", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith( `Cache restored from key: ${restoreKey}` @@ -465,7 +465,7 @@ test("restore with lookup-only set", async () => { expect(setCacheHitOutputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(setCacheHitOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", - "" + "false" ); expect(infoMock).toHaveBeenCalledWith( diff --git a/__tests__/restoreOnly.test.ts b/__tests__/restoreOnly.test.ts index 40875ec..8a0eb41 100644 --- a/__tests__/restoreOnly.test.ts +++ b/__tests__/restoreOnly.test.ts @@ -86,7 +86,7 @@ test("restore with no cache found", async () => { ); expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); - expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", "false"); expect(outputMock).toHaveBeenCalledTimes(2); expect(failedMock).toHaveBeenCalledTimes(0); @@ -170,7 +170,7 @@ test("restore with cache found for key", async () => { expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); expect(outputMock).toHaveBeenCalledWith("cache-hit", "true"); expect(outputMock).toHaveBeenCalledWith("cache-matched-key", key); - expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", "false"); expect(outputMock).toHaveBeenCalledTimes(4); @@ -214,7 +214,7 @@ test("restore with cache found for restore key", async () => { expect(outputMock).toHaveBeenCalledWith("cache-primary-key", key); expect(outputMock).toHaveBeenCalledWith("cache-hit", "false"); expect(outputMock).toHaveBeenCalledWith("cache-matched-key", restoreKey); - expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", ""); + expect(outputMock).toHaveBeenCalledWith("save-always-d18d746b9", "false"); expect(outputMock).toHaveBeenCalledTimes(4); diff --git a/src/restoreImpl.ts b/src/restoreImpl.ts index 98fbbde..2582f38 100644 --- a/src/restoreImpl.ts +++ b/src/restoreImpl.ts @@ -12,7 +12,10 @@ import * as utils from "./utils/actionUtils"; export async function restoreImpl( stateProvider: IStateProvider ): Promise { - core.setOutput(Outputs.SaveAlways, core.getInput(Inputs.SaveAlways)); + core.setOutput( + Outputs.SaveAlways, + core.getInput(Inputs.SaveAlways) || "false" + ); try { if (!utils.isCacheFeatureAvailable()) { From 2d00cd51fdafe961015856ee0011dd83995813a8 Mon Sep 17 00:00:00 2001 From: Danny Gleckler Date: Thu, 8 Feb 2024 20:52:30 -0500 Subject: [PATCH 3/5] Add save-always inpute/output test --- __tests__/restoreImpl.test.ts | 37 +++++++++++++++++++++++++++++++++++ src/utils/testUtils.ts | 4 ++++ 2 files changed, 41 insertions(+) diff --git a/__tests__/restoreImpl.test.ts b/__tests__/restoreImpl.test.ts index 88662a3..2fee7cf 100644 --- a/__tests__/restoreImpl.test.ts +++ b/__tests__/restoreImpl.test.ts @@ -473,3 +473,40 @@ test("restore with lookup-only set", async () => { ); expect(failedMock).toHaveBeenCalledTimes(0); }); + +test("restore with save-always set", async () => { + jest.spyOn(actionUtils, "isGhes").mockImplementation(() => true); + const path = "node_modules"; + const key = "node-test"; + testUtils.setInputs({ + path: path, + key, + saveAlways: true + }); + + const setCacheHitOutputMock = jest.spyOn(core, "setOutput"); + const restoreCacheMock = jest + .spyOn(cache, "restoreCache") + .mockImplementationOnce(() => { + return Promise.resolve(undefined); + }); + + await restoreImpl(new StateProvider()); + + expect(restoreCacheMock).toHaveBeenCalledTimes(1); + expect(restoreCacheMock).toHaveBeenCalledWith( + [path], + key, + [], + { + lookupOnly: false + }, + false + ); + + expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); + expect(setCacheHitOutputMock).toHaveBeenCalledWith( + "save-always-d18d746b9", + "true" + ); +}); diff --git a/src/utils/testUtils.ts b/src/utils/testUtils.ts index ba0670b..84aeae9 100644 --- a/src/utils/testUtils.ts +++ b/src/utils/testUtils.ts @@ -16,6 +16,7 @@ interface CacheInput { enableCrossOsArchive?: boolean; failOnCacheMiss?: boolean; lookupOnly?: boolean; + saveAlways?: boolean; } export function setInputs(input: CacheInput): void { @@ -32,6 +33,8 @@ export function setInputs(input: CacheInput): void { setInput(Inputs.FailOnCacheMiss, input.failOnCacheMiss.toString()); input.lookupOnly !== undefined && setInput(Inputs.LookupOnly, input.lookupOnly.toString()); + input.saveAlways !== undefined && + setInput(Inputs.SaveAlways, input.saveAlways.toString()); } export function clearInputs(): void { @@ -42,4 +45,5 @@ export function clearInputs(): void { delete process.env[getInputName(Inputs.EnableCrossOsArchive)]; delete process.env[getInputName(Inputs.FailOnCacheMiss)]; delete process.env[getInputName(Inputs.LookupOnly)]; + delete process.env[getInputName(Inputs.SaveAlways)]; } From 3e29e63694c646c5dd59e4739a6ef5e1ad35d084 Mon Sep 17 00:00:00 2001 From: Danny Gleckler Date: Fri, 9 Feb 2024 02:32:10 -0500 Subject: [PATCH 4/5] Fix mock name --- __tests__/restoreImpl.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/__tests__/restoreImpl.test.ts b/__tests__/restoreImpl.test.ts index 2fee7cf..2f6d3a6 100644 --- a/__tests__/restoreImpl.test.ts +++ b/__tests__/restoreImpl.test.ts @@ -484,7 +484,7 @@ test("restore with save-always set", async () => { saveAlways: true }); - const setCacheHitOutputMock = jest.spyOn(core, "setOutput"); + const setSaveAlwaysOutputMock = jest.spyOn(core, "setOutput"); const restoreCacheMock = jest .spyOn(cache, "restoreCache") .mockImplementationOnce(() => { @@ -504,8 +504,8 @@ test("restore with save-always set", async () => { false ); - expect(setCacheHitOutputMock).toHaveBeenCalledTimes(1); - expect(setCacheHitOutputMock).toHaveBeenCalledWith( + expect(setSaveAlwaysOutputMock).toHaveBeenCalledTimes(1); + expect(setSaveAlwaysOutputMock).toHaveBeenCalledWith( "save-always-d18d746b9", "true" ); From 3e72a6ebe141e2d806ccb28d2696f2680fd84c32 Mon Sep 17 00:00:00 2001 From: Danny Gleckler Date: Fri, 9 Feb 2024 11:17:15 -0500 Subject: [PATCH 5/5] Add save-always to README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index d1d2aef..f5ae8ee 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,7 @@ If you are using a `self-hosted` Windows runner, `GNU tar` and `zstd` are requir * `enableCrossOsArchive` - An optional boolean when enabled, allows Windows runners to save or restore caches that can be restored or saved respectively on other platforms. Default: `false` * `fail-on-cache-miss` - Fail the workflow if cache entry is not found. Default: `false` * `lookup-only` - If true, only checks if cache entry exists and skips download. Does not change save cache behavior. Default: `false` +* `save-always` - If true, always saves the cache, even if the job fails. Requires a [step `id`](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsid). Default: `false` #### Environment Variables