-
Notifications
You must be signed in to change notification settings - Fork 15
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
github: fix caching the docker images #176
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
- Coverage 76.26% 76.25% -0.01%
==========================================
Files 130 130
Lines 33970 33970
==========================================
- Hits 25906 25904 -2
- Misses 8064 8066 +2 ☔ View full report in Codecov by Sentry. |
1c44e67
to
e8326b5
Compare
This commit updates the way docker cache is created by including the current date into the cache key. Now when "save-always" option in actions/cache is deprecated [1] this seems to be the conventional way [2] to create the cache. [1] actions/cache#1452 [2] https://github.com/actions/cache?tab=readme-ov-file#creating-a-cache-key Signed-off-by: Mikhail Koviazin <mikhail.koviazin@aiven.io>
e8326b5
to
7683a03
Compare
Expanded the commit message a bit:
1: actions/cache#1452 Also removed the I am especially interested in seeing @smeso review on this PR because you created the original caching approach. |
@@ -98,7 +104,7 @@ jobs: | |||
with: | |||
path: ./custom-cache/ | |||
fail-on-cache-miss: true | |||
key: custom-cache | |||
key: custom-cache-${{ steps.date.outputs.current_date }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible the current_date
computed here is different from the current_date
computed in populate-cache
if the pipeline is run around midnight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this is possible, and the pipeline might fail because of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we can't possibly set a global variable of a date. So the only possible way to fix this is to repeat the same "look for cache -> if not found, create one" pattern here as well just because of this corner case and Github limitations :/ That's unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also other options, like:
- Create cache for each PR instead of tying it with the date
- Just accept the occasional PR github action failures and restart them manually. It shouldn't happen that often anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be possible to create a job to get the date and then just add a dependency to it, at least according to the docs. So something like:
get-date:
outputs:
current_date: ${{ steps.date.outputs.current_date }}
steps:
- name: Set current date
id: date
run: echo "current_date=$(date +'%Y%m%d')" >> $GITHUB_OUTPUT
and then use it like:
run-tests:
runs-on: ubuntu-latest
needs: [populate-cache, get-date]
....
with:
path: ./custom-cache/
fail-on-cache-miss: true
key: custom-cache-${{ needs.get-date.outputs.current_date }}
I haven't tested this, but if it works, then it would be nice because it abstracts the duplicate "Set current date" step.
If it doesn't work, then I'm happy with option 2. That race condition would happen super rarely anyway and it would work on the next try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, thanks, I'll try!
This PR supercedes #170 and fixes #140
Pull Request check-list
Description of change