Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore original behavior of cache-hit output #1467

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

joshmgross
Copy link
Member

As described in #1466, #1404 broke workflows that relied on the previous behavior of cache-hit being an empty string when there was a cache miss:

if: ${{ steps.cache-restore.outputs.cache-hit }}

If cache-hit is an empty string, this would work as expected. When cache-hit is false, this is a non-empty string and treated as "true".

Actions outputs do not support proper boolean values, and we can't guarantee how users are interpreting this string output.

This PR reverts #1404 and updates the README to clarify all possible values of cache-hit:

  1. 'true' if there's an exact match
  2. 'false' if there's a cache hit with a restore key
  3. '' if there's a cache miss

This is likely confusing to users, but we should maintain the existing behavior to avoid breaking existing workflows.

@joshmgross joshmgross requested a review from a team as a code owner October 8, 2024 17:02
@joshmgross joshmgross merged commit 3624ceb into main Oct 8, 2024
16 checks passed
@joshmgross joshmgross deleted the joshmgross/restore-cache-hit-behavior branch October 8, 2024 17:08
@Jason3S
Copy link

Jason3S commented Oct 8, 2024

@joshmgross
Thank you 🙏

@AnimMouse
Copy link

@joshmgross
Thank you!

@ulgens
Copy link

ulgens commented Oct 10, 2024

This is likely confusing to users, but we should maintain the existing behavior to avoid breaking existing workflows.

I can see the reasoning here but this shouldn't mean a weird behaviour should be kept just because fixing it is a breaking change. IMO having it in the release notes and making the necessary change in version number should be enough, which was already done. Having this PR merged feels like a step in the wrong direction and there should be a plan to properly handle that breaking change.

@joshmgross
Copy link
Member Author

IMO having it in the release notes and making the necessary change in version number should be enough, which was already done.

Per https://semver.org/, a breaking change needs to be a major version bump (i.e. v5 of this action).

I agreed that we should fix this confusing behavior, but we're not ready to create a new major version of this action.
#1466 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants