-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Add force-overwrite for cache #1308
base: main
Are you sure you want to change the base?
Conversation
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.
want
@joshmgross Would you mind reviewing this, or could you point me to the right person for that? This would close #342 which has hundreds of upvotes, mentions and a lot of comments. |
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.
Many need this.
src/saveImpl.ts
Outdated
); | ||
return; | ||
} | ||
|
||
if (utils.isExactKeyMatch(primaryKey, restoredKey) && forceOverwrite) { | ||
await issueCommand('actions-cache delete', {}, primaryKey); |
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.
@PrinsFrank were you able to validate these changes in your own repository?
I'm not entirely sure if the backend supporting the cache action will allow overwriting an existing cache entry like this, and as far as I know this actions-cache delete
command is not a real command.
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.
Sorry, I got confused with the CI options that are available in the actions-cache plugin, but not in the toolkit yet. I use that here: https://github.com/PrinsFrank/CI-PHP/blob/main/.github/workflows/quality.yml#L134, so I can confirm this workflow works, the code just currently doesn't. I wasn't sure how to run this code, but I figured it out and can see this doesn't work in the current form.
If I understand correctly there's currently not any code for this at all in upstream packages, so I tried implementing it in actions/toolkit#1783. As I mentioned before, I'm not a typescript developer so any pointers would be very much appreciated.
e457006
to
5afc636
Compare
5afc636
to
0d93be3
Compare
Description
This PR adds a
force-overwrite
options to force overwrite an already existing cache.I could use some help with the implementation. I'm native to PHP and not familiar with where all the code lives in the NPM packages. There is a CLI available with this exact command, but I guess it would make sense to implement the request to delete a cache in typescript just as all the other request, but I don't know where to. Any pointer or help would be appreciated!
Motivation and Context
When working with very long workflows and incremental changes any tiny change in cache can be helpful. That's why currently I delete a cache when it exists with the following workflow:
For context: without any caching the job will take about 20 minutes. With a main cache and a 'branch cache' that is never updated again, the job will take a few minutes after the first commit, but every next change results in the cache being further out of date and the job taking longer and longer. That's why deleting the existing cache will speed things up significantly.
How Has This Been Tested?
N/A
Screenshots (if appropriate):
N/A
Types of changes
Checklist: