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(cache): change key to match actions/cache documentation #213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(cache): change key to match actions/cache documentation #213

wants to merge 1 commit into from

Conversation

mmorel-35
Copy link

Description:
This PR change the cache key construction to fit those examples. It also add the use of restoreKeys and a more verbose log when retrieving fails.

It also add the pre-checkin npm task as it exists on the other setup-X

I don't see how to fix this but the "Verify no unstaged changes" is failing. I'm working on Windows but usually it doesn't matter.
Any idea ?

Related issue:
N/A

Check list:

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

@KengoTODA
Copy link
Contributor

I don't see how to fix this but the "Verify no unstaged changes" is failing. I'm working on Windows but usually it doesn't matter.

It is caused by the difference between LF (Ubuntu) and CRLF (Windows). I also faced this problem before.
I solved it by replacing the dist with files built on linux machine, we can upload the dist file to workflow by actions/upload-artifact:

https://github.com/KengoTODA/setup-java/blob/aae89bdd784d68e53c1bda327075e7af68d50ac5/.github/workflows/build.yml#L31-L34

@KengoTODA
Copy link
Contributor

Note about the context of current key naming:

  1. First, it followed the actions/cache documentation: ${process.env['RUNNER_OS']}-${packageManager.id}-${hash}
  2. Second, get reviewed and suggested to add a prefix, so
  3. Added a prefix and now it's ${CACHE_KEY_PREFIX}-${process.env['RUNNER_OS']}-${packageManager.id}-${hash}

Signed-off-by: Matthieu MOREL <mmorel-35@users.noreply.github.com>
@mmorel-35
Copy link
Author

mmorel-35 commented Sep 3, 2021

Hi @KengoTODA, I found another way , to correct the file, I'm creating a workflow where I execute the build and then have a task to commit the changed file directly on my repository, see here for example

Concerning the notion of prefix I'm not sure to really understand the reason behind that. Does it means that the provided examples in actions/cache are wrong ? Their is no explanation behind that change in the comment.
What I'm doing here is just moving this prefix after the operating system which quite more common in the community as most of the people take example on that same page I referenced earlier.

@KengoTODA
Copy link
Contributor

Does it means that the provided examples in actions/cache are wrong ?

It's not wrong. And there is no strict official rule to design the cache key... what we need is just grab how actions/cache matches a cache key and design optimized key and restore keys. So I just shared the context of current change... repository owner will judge how the key should be designed.

@IvanZosimov IvanZosimov added feature request New feature or request to improve the current logic needs eyes labels Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic needs eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants