-
Notifications
You must be signed in to change notification settings - Fork 510
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 cache-key-prefix
option
#366
Conversation
339ae12
to
ecd541e
Compare
ecd541e
to
089c1d9
Compare
src/cache-restore.ts
Outdated
@@ -29,7 +29,8 @@ export const restoreCache = async ( | |||
); | |||
} | |||
|
|||
const primaryKey = `setup-go-${platform}-go-${versionSpec}-${fileHash}`; | |||
const cacheKeyPrefix = core.getInput('cache-key-prefix') || ''; |
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.
Another option is to use setup-go-
as the default
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.
const cacheKeyPrefix = core.getInput('cache-key-prefix') || 'setup-go'
const primaryKey = `${cacheKeyPrefix}-${platform}-go-${versionSpec}-${fileHash}`;
Not sure why the windows tests are failing, so any help will be appreciated |
I recently forked the repo, made the same changes, and got the same error for the linter workflow. Somehow, rerunning the job with debug logging made it pass. |
Hello lovely maintainers 👋 Anything we can do to move this forward (or close it so we'll know if we need to use a different solution)? |
|
Hello everyone, Sorry for the late response, first of all 🙂 |
Hello @erezrokah , can you please review and apply the changes i requested in order we can approve the PR merge? |
@dsame did you submit the review, though? I can't see any comments ATM |
Thanks for the review @dsame, but I'm not seeing it. Can you confirm you submitted it? |
@erezrokah please take a look at this one https://github.com/actions/setup-go/pull/366/files/acb2fd804097c1dffc2eb1075b3696da4b4bb27c#r1158511364 ? |
@dsame |
I'm not seeing anything there. Could it be that your review comments are still pending and you need to submit them like @candiduslynx mentioned? See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request#submitting-your-review |
@candiduslynx @erezrokah thanks for you note, i missed to click "submit" indeed. No the review is published |
Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com>
Thanks @dsame, I updated the PR and requested another review |
@erezrokah can you please run |
Woops, sorry about that. Fixed in ab4b1f7 |
Would be great to get a point release once this great PR is in to get cache keys plus tzst compression :) Thank you all for your work! |
Hi 👋 Anything blocking this from getting merged? |
Friendly ping- seems PR is stuck once more? |
PR is done and has three approvals :) |
@erezrokah @andig while the PR is perfect but there's a hesitation about should it be merged because it repeats functionality of https://github.com/actions/cache action. Please let us a bit more time for discussing. |
@dsame Thank you for taking the time to carefully review! |
I'm happy to close this PR and open a new one to add relevant docs per #358 (comment). Please let me know if you're open to such a contribution |
Hello @erezrokah. Sorry for the late response. You can prepare a note to the documentation. |
Closing this PR. I'll open a new one for the docs |
I'm really sad this decision had to be made:
We're trading a 2-line zero risk diff against setup complexity for every single user with more than the basic use case. That is imho a bad, high-cost trade-off. |
Description:
Adds a cache key prefix option so it's possible to use different cache keys for different jobs
Related issue:
Fixes #358
Check list: