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

feat: Add cache-key-prefix option #366

Closed
wants to merge 6 commits into from

Conversation

erezrokah
Copy link

@erezrokah erezrokah commented Apr 5, 2023

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:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@erezrokah erezrokah requested a review from a team as a code owner April 5, 2023 13:18
@@ -29,7 +29,8 @@ export const restoreCache = async (
);
}

const primaryKey = `setup-go-${platform}-go-${versionSpec}-${fileHash}`;
const cacheKeyPrefix = core.getInput('cache-key-prefix') || '';
Copy link
Author

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

Copy link
Contributor

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}`;

@erezrokah
Copy link
Author

Not sure why the windows tests are failing, so any help will be appreciated

@rokerzfirst101
Copy link

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.

@erezrokah
Copy link
Author

erezrokah commented May 29, 2023

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)?

@candiduslynx
Copy link

Hello lovely maintainers 👋 Anything we can do to move this forward (or close it so we'll if know if we need to use a different solution)?

@dsame @MaksimZhukov @IvanZosimov @marko-zivic-93

@marko-zivic-93
Copy link
Contributor

Hello everyone,

Sorry for the late response, first of all 🙂
We have started working on this feature, but right now we have shifted our focus a bit from it. This should be handled in the upcoming quarter. Thank you for your patience 🙂

@dsame
Copy link
Contributor

dsame commented Jun 1, 2023

Hello @erezrokah , can you please review and apply the changes i requested in order we can approve the PR merge?

@candiduslynx
Copy link

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

@erezrokah
Copy link
Author

Hello @erezrokah , can you please review and apply the changes i requested in order we can approve the PR merge?

Thanks for the review @dsame, but I'm not seeing it. Can you confirm you submitted it?

@dsame
Copy link
Contributor

dsame commented Jun 2, 2023

@candiduslynx
Copy link

candiduslynx commented Jun 2, 2023

@erezrokah please take a look at this one https://github.com/actions/setup-go/pull/366/files/acb2fd804097c1dffc2eb1075b3696da4b4bb27c#r1158511364 ?

@dsame
You definitely didn't publish the review (kindly check that you push submit review in the review tab)

@erezrokah
Copy link
Author

@dsame
Copy link
Contributor

dsame commented Jun 2, 2023

@candiduslynx @erezrokah thanks for you note, i missed to click "submit" indeed. No the review is published

src/cache-restore.ts Outdated Show resolved Hide resolved
@erezrokah erezrokah requested a review from dsame June 2, 2023 12:57
@erezrokah
Copy link
Author

@candiduslynx @erezrokah thanks for you note, i missed to click "submit" indeed. No the review is published

Thanks @dsame, I updated the PR and requested another review

@dsame
Copy link
Contributor

dsame commented Jun 2, 2023

@erezrokah can you please run npm run format and commit the format fixes? It is necessary to make these checks to pass: https://github.com/actions/setup-go/actions/runs/5155716632/jobs/9286746499?pr=366

@erezrokah
Copy link
Author

@erezrokah can you please run npm run format and commit the format fixes? It is necessary to make these checks to pass: https://github.com/actions/setup-go/actions/runs/5155716632/jobs/9286746499?pr=366

Woops, sorry about that. Fixed in ab4b1f7

@andig
Copy link

andig commented Jun 2, 2023

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!

@erezrokah
Copy link
Author

Hi 👋 Anything blocking this from getting merged?

@andig
Copy link

andig commented Jun 15, 2023

Friendly ping- seems PR is stuck once more?

@candiduslynx
Copy link

@andig
Copy link

andig commented Jun 16, 2023

PR is done and has three approvals :)

@dsame
Copy link
Contributor

dsame commented Jun 21, 2023

@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.

@andig
Copy link

andig commented Jun 23, 2023

@dsame Thank you for taking the time to carefully review!
I feel it does not repeat actions/cache functionality, but rather exposes it. If easier/clearer, you might potentially consider exposing cache-key directly instead of prefix. In any case, I think having to configure an additional key or prefix is much simpler from user perspective than setting up cache with correct folders, even if fully documented.

@erezrokah
Copy link
Author

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

@dmitry-shibanov
Copy link
Contributor

Hello @erezrokah. Sorry for the late response. You can prepare a note to the documentation.

@erezrokah
Copy link
Author

Closing this PR. I'll open a new one for the docs

@erezrokah erezrokah closed this Jul 14, 2023
@erezrokah erezrokah deleted the feat/add_cache_prefix branch July 14, 2023 06:22
@erezrokah erezrokah restored the feat/add_cache_prefix branch July 14, 2023 06:22
@andig
Copy link

andig commented Jul 14, 2023

I'm really sad this decision had to be made:

In any case, I think having to configure an additional key or prefix is much simpler from user perspective than setting up cache with correct folders, even if fully documented.

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.

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.

feat: add cache prefix
7 participants