Fix cache-hit output when cache missed

[fchimpan]

Description

fix https://github.com/actions/cache/issues/1334

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

• [x] 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:

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

[fchimpan]

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]

@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!

[fchimpan]

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]

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

Thank you!

[v4hn]

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]

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

[Jason3S]

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]

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]

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]

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]

Then the readme correction proposed in https://github.com/actions/cache/pull/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]

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

[AnimMouse]

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]

👋 I will work on reverting this change