runs-on / cache

Shockingly faster GitHub Action cache with S3 backend
https://runs-on.com
MIT License
42 stars 7 forks source link

Fix save-always/post-if with an output #6

Closed ruffsl closed 3 weeks ago

ruffsl commented 1 month ago

Fixes:

Cherry-picked from:

ruffsl commented 1 month ago

Shoot, cherry-picking over the orginal PR didn't work (as compared to when applied to github/cache):

image

ruffsl commented 1 month ago

Just noticed the main branch is 10 commits behind v4?! Guess I don't understand the branching strategy here.

https://github.com/runs-on/cache/compare/main...runs-on:cache:v4

However is also looks like the v4 branch itself is 13 commits behind v4.0.2 tag from upstream:

https://github.com/runs-on/cache/compare/v4...actions:cache:v4.0.2

@crohr , any interest in updating v4 branch to track the prerequisite patches from upstream, E.g:

crohr commented 1 month ago

V4 branch has more commits (mine) for handling s3 backend. I will pick the changes from upstream, most likely this week or next!

crohr commented 1 month ago

@ruffsl I'd like the changes to merged upstream before proceeding, so that I do not diverge from upstream.

Is your issue not solvable through the use of the dedicated save / restore actions?

ruffsl commented 1 month ago

Is your issue not solvable through the use of the dedicated save / restore actions?

Eh, sort of. We have a lot of separate cache calls, so this would effectively double all those steps.

I'd like the changes to merged upstream before proceeding, so that I do not diverge from upstream.

No prob. But could we still at least pull in the current head of main branch from upstream? At least then the final patch would be simpler to apply and track from our own fork while we wait for upstream to merge in the fix. Upstream maintainers for actions/cache are missing in action at the moment (pun intended):

crohr commented 1 month ago

@ruffsl v4 has been updated with the latest fixes from upstream.

ruffsl commented 1 month ago

Dang. I rebased from the head of v4 on this repo, then rebuilt the patch, but can't get the same fix working.

It's odd in that the diff of the npm build look the same, but perhaps this fork has affected the control flow?

ruffsl commented 1 month ago

Once such local change I can readily spot between the two diffs is this:

--- https://raw.githubusercontent.com/bearfriend/cache/3a22487daddcfa2ebf7238869de052504cc6dc4b/dist/restore/index.js
+++ https://raw.githubusercontent.com/botsandus/cache/98f0c46be05ee2b67a1c04787e1448b4537ac39b/dist/restore/index.js
@@ -1,4 +1,4 @@
-function restoreImpl(stateProvider) {
+function restoreImpl(stateProvider, earlyExit) {
     return __awaiter(this, void 0, void 0, function* () {
         core.setOutput(constants_1.Outputs.SaveAlways, core.getInput(constants_1.Inputs.SaveAlways) || "false");
         try {
@@ -20,7 +20,14 @@
             const enableCrossOsArchive = utils.getInputAsBool(constants_1.Inputs.EnableCrossOsArchive);
             const failOnCacheMiss = utils.getInputAsBool(constants_1.Inputs.FailOnCacheMiss);
             const lookupOnly = utils.getInputAsBool(constants_1.Inputs.LookupOnly);
-            const cacheKey = yield cache.restoreCache(cachePaths, primaryKey, restoreKeys, { lookupOnly: lookupOnly }, enableCrossOsArchive);
+            let cacheKey;
+            if (canSaveToS3) {
+                core.info("The cache action detected a local S3 bucket cache. Using it.");
+                cacheKey = yield custom.restoreCache(cachePaths, primaryKey, restoreKeys, { lookupOnly: lookupOnly });
+            }
+            else {
+                cacheKey = yield cache.restoreCache(cachePaths, primaryKey, restoreKeys, { lookupOnly: lookupOnly }, enableCrossOsArchive);
+            }
             if (!cacheKey) {
                 if (failOnCacheMiss) {
                     throw new Error(`Failed to restore cache entry. Exiting as fail-on-cache-miss is set. Input key: ${primaryKey}`);
@@ -45,6 +52,9 @@
         }
         catch (error) {
             core.setFailed(error.message);
+            if (earlyExit) {
+                process.exit(1);
+            }
         }
     });
 }

@crohr , any suggestions?

ruffsl commented 3 weeks ago

Looks like this'll be a wontfix from upstream:

solvable through the use of the dedicated save / restore actions?

@crohr , I've now indeed gone back to that approach using if: always() via an explicit save step. The only bummer being that it nurfs the utility of caching in a composite action with ordered post steps: