Skip to content

Commit

Permalink
fix(l1, l2, levm): move loc_report.json to loc_report.json.old ev…
Browse files Browse the repository at this point in the history
…en when there a `restore-keys` is used. (#1438)

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.
<!-- Why does this pull request exist? What are its goals? -->

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

<!-- A clear and concise general description of the changes this PR
introduces -->
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.
<!-- Link to issues: Resolves #111, Resolves #222 -->

No associated issue number
  • Loading branch information
lima-limon-inc authored Dec 12, 2024
1 parent f63abf0 commit cf69ecf
Showing 1 changed file with 1 addition and 5 deletions.
6 changes: 1 addition & 5 deletions .github/workflows/daily_reports.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,8 @@ jobs:
restore-keys: |
loc-report-
- name: Check previous loc report
run: |
cat loc_report.json
- name: Rename cached loc_report.json to loc_report.json.old
if: steps.cache-loc-report.outputs.cache-hit == 'true'
if: steps.cache-loc-report.outputs.cache-hit != ''
run: mv loc_report.json loc_report.json.old

- name: Generate the loc report
Expand Down

0 comments on commit cf69ecf

Please sign in to comment.