Fix cache-hit output when cache missed #1404

fchimpan · 2024-05-22T03:03:04Z

Description

According to the README, if the cache was not found, the output should be 'false'. However, it actually returned an empty string. This seems to be due to the behavior of GitHub Actions to return an empty string if the output is not defined. Please see these conversations.

I would like to change the code to return 'false' when the cache is not found, per the README.

Motivation and Context

In PR #1263, a README correction is suggested. This is a great initiative, thank you. However, to avoid confusion, how about changing the implementation instead?

How Has This Been Tested?

The issue was caused by not defining an output for cache-hit when the cache was not found, and then returning. Therefore, I will set the output before returning.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (add or update README or docs)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

fchimpan · 2024-05-27T05:40:15Z

Hi, @bethanyj28
I would like to request a reviewer for this pull request. If you are not the reviewer, could you please recommend a suitable reviewer?

Thank you very much!

fchimpan · 2024-06-18T05:56:48Z

@joshmgross
I would like to request a reviewer for this pull request. If you are not the reviewer, could you please recommend a suitable reviewer?

Thank you very much!

joshmgross · 2024-06-18T19:59:01Z

src/restoreImpl.ts

@@ -62,7 +62,7 @@ export async function restoreImpl(
                    ...restoreKeys
                ].join(", ")}`
            );
-
+            core.setOutput(Outputs.CacheHit, false.toString());


Should we set this output before the if (failOnCacheMiss) case above?

That would ensure it's set even if fail-on-cache-miss is true.

Hi, @joshmgross

Thanks for your response.

Indeed, since it applies even in the case of exceptions, it seems good to output before failOnCacheMiss. What do you think?
6f5b5ee

Thanks @fchimpan!

Looks like the dist/ is out of date - could you re-run npm run build?

https://github.com/actions/cache/actions/runs/9599939755/job/26475812160?pr=1404

Thanks @joshmgross !
8addae5
I have pushed the dist.

fchimpan · 2024-06-21T00:58:58Z

I tested the implementation of this PR by actually using it. As expected, it was confirmed that ‘false’ is returned when the cache is not found.

v4 test : https://github.com/fchimpan/cache-pr-1404-test/actions/runs/9606621627
This PR test : https://github.com/fchimpan/cache-pr-1404-test/actions/runs/9606621624
Please see cache-miss job's Check outputs step.

fchimpan · 2024-06-26T11:57:42Z

Hi @joshmgross ,
I’ve made the necessary changes. Could you please review the code?

Thank you!

* fix: cache-hit output * fix: Output chache hit timing * fix: Output chache hit timing --------- Co-authored-by: Josh Gross <joshmgross@github.com>

v4hn · 2024-10-07T19:11:01Z

Comment: The ternary logic which this patch removes was a useful (but -granted- undocumented and broken) API to detect whether restore-keys was used.
E.g.,

steps:
- name: Restore Cache
  id: restore
  uses: actions/cache/restore@v4
  with:
    path: ...
    key: abc-${{ github.sha }}-${{ github.run_id }}-${{ github.run_attempt }}
    restore-keys: |
      abc-${{ github.sha }}-${{ github.run_id }}
- name: Run if any cache was restored
  if: steps.restore.outputs.cache-hit != ''
  ...

The correct way to get the same check seems to be
if: steps.restore.outputs.cache-matched-key != ''.
Maybe it makes sense to add that use-case in the README?

Jason3S · 2024-10-08T06:36:13Z

I think it would have been better to change the documentation. This was a major breaking change.

Jason3S · 2024-10-08T07:03:52Z

Before landing this type of change, it would be useful to check how it is used:
https://github.com/search?q=.cache-hit+language%3AYaml+&type=code

Ari-Kishon · 2024-10-08T07:21:35Z

This was a major change and should have been released as such.

The change in return value cause several critical steps in our workflows to be skipped and caused them all to fail.

I understand that this behavior has technically a bug, but given how long it returned empty values this should have been considered valid behavior and taken into account.

This change could have potentially disrupted development for my whole team had I not caught it before going on vacation.

We rely heavily on actions from this organization because we perceive them be stable and professionally maintained.

stof · 2024-10-08T09:05:18Z

The readme says that chache-hit is a boolean value: https://github.com/actions/cache?tab=readme-ov-file#outputs

The string 'false' is not a boolean value (and it is a truthy value). Please revert this broken change

v4hn · 2024-10-08T12:08:35Z

The readme says that chache-hit is a boolean value: https://github.com/actions/cache?tab=readme-ov-file#outputs

The string 'false' is not a boolean value (and it is a truthy value). Please revert this broken change

In that sense the API was inconsistent with the docs already before.
When a restore-keys entry was found instead of the primary key, 'false' was already returned before and a falsy value was only returned in case neither key was found.

stof · 2024-10-08T12:12:22Z

Then the readme correction proposed in #1263 is still relevant, even though the description of this PR says that it is meant to fix the implementation instead of having to clarify the readme (the PR of course needs to be adapted to account for the change done here)

nati-levi · 2024-10-08T15:15:53Z

This broke our CI.
Should have been major release.
Its a breaking change.

AnimMouse · 2024-10-08T15:58:01Z

This basically broke all of my composite actions.
It's sad that it's 2024, and GitHub Actions still can't make their type system right!
true and false are booleans, not strings!

joshmgross · 2024-10-08T16:03:51Z

👋 I will work on reverting this change

@v4

* update documentation to use <action>@v4 * Update README.md and use v4 of checkout action (actions#1437) Update examples to use latest available checkout action v4. * Explicit use bash for Windows (actions#1377) Co-authored-by: Josh Gross <joshmgross@github.com> * Fix cache-hit output when cache missed (actions#1404) * fix: cache-hit output * fix: Output chache hit timing * fix: Output chache hit timing --------- Co-authored-by: Josh Gross <joshmgross@github.com> * Clarify that the `restore-keys` input is a string in the docs (actions#1434) * Fix Description for restore-keys at Readme As previously the restore-keys were defined as an ordered lists which is wrong as per the issue description where the actual format is a multi-line string with one key per line. * Added a space between the sentence of restore-keys description While at the PR review it's been identified there's a need for a space between the sentence ``` An ordered multiline string listing the prefix-matched keys,that are used for restoring stale cache if no cache hit occurred for key. ``` where it's written as "prefix-matched keys,that are" this commit will address the review comment and introduce a space between "prefix-matched keys, that are" and change the sentence to ``` An ordered multiline string listing the prefix-matched keys, that are used for restoring stale cache if no cache hit occurred for key. ``` * Change restore-keys description at cache/restore/action.yml and cache/action.yml * Add workflow file for publishing releases to immutable action package This workflow file publishes new action releases to the immutable action package of the same name as this repo. This is part of the Immutable Actions project which is not yet fully released to the public. First party actions like this one are part of our initial testing of this feature. * Deprecate `save-always` input (actions#1452) The `save-always` input added in v4 is not working as intended due to `post-if` expressions not supporting the input context. To avoid breaking users who have already added this input to their workflows, it is being deprecated now and will be removed in the next major version (v5). See actions#1315 for more details. * Fix typo: depening -> depending (actions#1462) Co-authored-by: Josh Gross <joshmgross@github.com> * restore action's README now references v4 instead of v3 (actions#1445) Co-authored-by: Josh Gross <joshmgross@github.com> * Prepare `4.1.0` release (actions#1464) * Restore original behavior of `cache-hit` output (actions#1467) * Restore original behavior of `cache-hit` output * Bump version to 4.1.1 * Add Bun example (actions#1456) * Add Bun example * Fix Bun Windows example * Revise `isGhes` logic * ran `npm run build` * appease the linter * added unit tests * Bump braces from 3.0.2 to 3.0.3 Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3. - [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md) - [Commits](micromatch/braces@3.0.2...3.0.3) --- updated-dependencies: - dependency-name: braces dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * Create dependabot.yml * Prepare release 4.1.2 * Bump actions/checkout from 3 to 4 Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Bump actions/stale from 3 to 9 Bumps [actions/stale](https://github.com/actions/stale) from 3 to 9. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v3...v9) --- updated-dependencies: - dependency-name: actions/stale dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Bump actions/setup-node from 3 to 4 Bumps [actions/setup-node](https://github.com/actions/setup-node) from 3 to 4. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v3...v4) --- updated-dependencies: - dependency-name: actions/setup-node dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Bump github/codeql-action from 2 to 3 Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v2...v3) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * Upgrade @actions/cache to 4.0.0 * Update RELEASES.md * Upgrade @vercel/ncc to 0.38.3 * Upgrade @actions/core to 1.11.1 and other deps * Add licensed output * Add reviewed licensed packages * Add lodash to list of reviewed licenses * Add licensed output * Rerun CI * Add 3.4.0 release notes * Correct GitHub Spelling in caching-strategies.md (actions#1526) GitHub was spelled incorrectly 3 lines under the Understanding how to choose path section * docs: Make the "always save prime numbers" example more clear (actions#1525) * Update force deletion docs due a recent deprecation (actions#1500) * fix: update force deletion docs due a recent deprecation * fix: applied josh's suggestions --------- Co-authored-by: Josh Gross <joshmgross@github.com> * bump @actions/cache to v4.0.1 * Update publish-immutable-actions.yml * bump @actions/cache to v4.0.2, prep for v4.2.2 release * add changes * changed * mask whole url * debugging * type * artifact changes * update cache package to mask whole sas to the end of the line * mask * update * latest test before pr * updated cache with latest changes * updates * new package * update cache with main * Update to use the latest version of the cache package to obfuscate the SAS * Update releases.md * Update README.md * fix: fix --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: todgru <todgru@gmail.com> Co-authored-by: P. Ottlinger <ottlinger@users.noreply.github.com> Co-authored-by: Oleg A. <t0rr@mail.ru> Co-authored-by: Josh Gross <joshmgross@github.com> Co-authored-by: r4mimu <52129983+fchimpan@users.noreply.github.com> Co-authored-by: Soubhik Kumar Mitra <59209034+x612skm@users.noreply.github.com> Co-authored-by: Bassem Dghaidi <568794+Link-@users.noreply.github.com> Co-authored-by: Joel Ambass <Jcambass@users.noreply.github.com> Co-authored-by: mackey0225 <masaki.asano0225@gmail.com> Co-authored-by: Eman Resu <78693624+quatquatt@users.noreply.github.com> Co-authored-by: Jan T. Sott <git@idleberg.com> Co-authored-by: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: janco-absa <janco.bester@absa.africa> Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com> Co-authored-by: Alessandro Sebastiani <sebbalex@users.noreply.github.com> Co-authored-by: Rob Herley <robherley@github.com> Co-authored-by: Salman Chishti <salmanmkc@GitHub.com> Co-authored-by: Ben De St Paer-Gotch <nebuk89@github.com>

fix: cache-hit output

688d5d4

fchimpan requested a review from a team as a code owner May 22, 2024 03:03

joshmgross reviewed Jun 18, 2024

View reviewed changes

fchimpan added 2 commits June 21, 2024 00:09

fix: Output chache hit timing

6f5b5ee

fix: Output chache hit timing

8addae5

navalboyblue approved these changes Jul 17, 2024

View reviewed changes

Merge branch 'main' into fix-cache-miss-output

222d593

joshmgross self-assigned this Jul 31, 2024

joshmgross approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'main' into fix-cache-miss-output

589c269

joshmgross enabled auto-merge (squash) July 31, 2024 16:55

joshmgross merged commit 40c3b67 into actions:main Jul 31, 2024
14 checks passed

joshmgross mentioned this pull request Aug 14, 2024

Cache-Hit output is null #1448

Closed

joshmgross mentioned this pull request Oct 3, 2024

Prepare 4.1.0 release #1464

Merged

prolic mentioned this pull request Oct 7, 2024

Bump actions/cache from 4.0.2 to 4.1.0 futrnostr/futr#29

Merged

Jason3S mentioned this pull request Oct 8, 2024

Cache-Hit set to string, not boolean, as of v4.1.0 #1466

Closed

AnimMouse mentioned this pull request Oct 8, 2024

Update to actions/cache outputs cache-hit as string instead of boolean AnimMouse/setup-rclone#10

Closed

joshmgross mentioned this pull request Oct 8, 2024

Restore original behavior of cache-hit output #1467

Merged

renovate bot mentioned this pull request Oct 8, 2024

fix(deps): update all non-major dependencies daku10/blog.daku10.dev#107

Merged

1 task

Boss-1s mentioned this pull request Oct 30, 2024

Bump actions/cache from 4.0.2 to 4.1.2 in /.github/workflows Boss-1s/scratchattach#13

Closed

Fix cache-hit output when cache missed #1404

Fix cache-hit output when cache missed #1404

Conversation

fchimpan commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

fchimpan commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fchimpan commented Jun 18, 2024

Uh oh!

joshmgross Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

fchimpan Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

joshmgross Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

fchimpan Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

fchimpan commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fchimpan commented Jun 26, 2024

Uh oh!

Uh oh!

v4hn commented Oct 7, 2024

Uh oh!

Jason3S commented Oct 8, 2024

Uh oh!

Jason3S commented Oct 8, 2024

Uh oh!

Ari-Kishon commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stof commented Oct 8, 2024

Uh oh!

v4hn commented Oct 8, 2024

Uh oh!

stof commented Oct 8, 2024

Uh oh!

nati-levi commented Oct 8, 2024

Uh oh!

AnimMouse commented Oct 8, 2024

Uh oh!

joshmgross commented Oct 8, 2024

Uh oh!

Uh oh!

fchimpan commented May 22, 2024 •

edited

Loading

fchimpan commented May 27, 2024 •

edited

Loading

fchimpan commented Jun 21, 2024 •

edited

Loading

Ari-Kishon commented Oct 8, 2024 •

edited

Loading