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

Properly check for cache hits. #2172

Merged
merged 4 commits into from
May 24, 2024
Merged

Properly check for cache hits. #2172

merged 4 commits into from
May 24, 2024

Conversation

vrabaud
Copy link
Collaborator

@vrabaud vrabaud commented May 23, 2024

steps.cache-ext.outputs.cache-hit != 'true' is actually always true because steps.cache-ext.outputs.cache-hit does not exist anymore, as the step cache-hit is now hidden in the cache action. The cache-hit information has to be sent back from the cache action.

@vrabaud vrabaud requested a review from y-guyon May 23, 2024 14:34
Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised this change is needed, it was working as is at some point.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vincent: Searching for "cache-hit" in https://github.com/actions/cache, I found that we are testing cache-hit in the same way as the sample code.

Are you sure this pull request is necessary?

@vrabaud
Copy link
Collaborator Author

vrabaud commented May 23, 2024

Let's put this one on the back burner, maybe I thgought it was necessary because of a cache miss.

@vrabaud
Copy link
Collaborator Author

vrabaud commented May 24, 2024

The cache hit was not checked properly: the answer from the action has to be sent back to the main workflow. This bug was just introduced in #2174

@vrabaud vrabaud marked this pull request as ready for review May 24, 2024 15:09
@vrabaud vrabaud requested a review from y-guyon May 24, 2024 15:09
@vrabaud vrabaud merged commit 73936dd into AOMediaCodec:main May 24, 2024
19 checks passed
@vrabaud vrabaud deleted the fix_2 branch May 24, 2024 15:48
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.

3 participants