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

fix(l1, l2, levm): move loc_report.json to loc_report.json.old even when there a restore-keys is used. #1438

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

lima-limon-inc
Copy link
Contributor

Even when there is no cache-hit, the file is fetched. If it came from the restore-keys directive, it doesn't count as a cache-hit even if the file is fetched.

Motivation
Show LOC diffs in the different slack channels.

Description
This description was taken from this comment: #1433 (comment)

We are saving the log_report.json file on a cache entry that has as key loc-report-${{ github.ref_name }}.
github.ref_name is the name of a branch not a commit hash. That name is not "main" but rather the name of the PR it came from. In this case ci-diffs.

What's the problem with that?
If the branch names don't match EXACTLY, the cache fails. HOWEVER, we have a restore-keys as a backup (here: https://github.com/lambdaclass/ethrex/blob/main/.github/workflows/daily_reports.yaml#L124-L125).

restor-keys is a backup that acts like a sort of regex. It tells github-cache "Hey, seen as you failed fetching that EXACT key, give me, as a backup, a value that matches the following prefix". That prefix should match everytime, since every key we generate uses that exact same prefix.

So what's the problem? The problem is that we are only changing the file name IF there is a cache hit aka this if: https://github.com/lambdaclass/ethrex/blob/main/.github/workflows/daily_reports.yaml#L132. So, once again, what's the problem? We are getting a cache hit everytime, we either get it with the exact key or using the restore-keys as a fallback.

And there is the problem, if the value is fetched using restore-keys, it doesn't count as a cache hit.
This behavior is described here: https://github.com/actions/cache/blob/main/README.md?plain=1#L129.

That would also explain why the script worked while I was working on this PR: I was always on the same branch, so it was always a cache hit, it was never using restore-keys. However, when the cron job is executed, it probably fails every time and ends up using the backup.

That is also strange, one would think that it would end up using "main" branch as a key name. My hunch is that it uses commits coming from PR's, so it's maybe using those PR branch names for keys.

I'm gonna create a new PR for it to be merged on Tuesday, so we can see the difference in output and check that this indeed is the issue.

No associated issue number

Even when there is no cache-hit, the file is fetched.
If it came from the `restore-keys` directive, it doesn't count as a
cache-hit even if the file is fetched.
@lima-limon-inc
Copy link
Contributor Author

Ideally, this should be merged on Wednesday. So we can see the bot's output on monday and tuesday, and we let it store those values in the cache. Just in case.

@lima-limon-inc lima-limon-inc changed the title fix(l1, l2, levm) fix(l1,l2,levm) Dec 10, 2024
@lima-limon-inc lima-limon-inc changed the title fix(l1,l2,levm) fix(l1, l2, levm) Dec 10, 2024
@lima-limon-inc lima-limon-inc marked this pull request as ready for review December 10, 2024 17:10
@lima-limon-inc lima-limon-inc requested a review from a team as a code owner December 10, 2024 17:10
@lima-limon-inc lima-limon-inc changed the title fix(l1, l2, levm) fix (l1, l2, levm) Dec 10, 2024
@lima-limon-inc lima-limon-inc changed the title fix (l1, l2, levm) fix(l1, l2, levm): Move loc_report.json to loc_report.json.old even when there a restore-keys is used. Dec 10, 2024
@lima-limon-inc lima-limon-inc changed the title fix(l1, l2, levm): Move loc_report.json to loc_report.json.old even when there a restore-keys is used. fix(l1, l2, levm): move loc_report.json to loc_report.json.old even when there a restore-keys is used. Dec 10, 2024
@JereSalo JereSalo added this pull request to the merge queue Dec 12, 2024
Merged via the queue into main with commit cf69ecf Dec 12, 2024
13 checks passed
@JereSalo JereSalo deleted the ci-diffs-second-attempt branch December 12, 2024 20:23
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.

2 participants