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 an option for repository-local caching #52

Closed
wants to merge 1 commit into from

Conversation

hasundue
Copy link

This PR adds a feature to store Deno installation to repository-local cache with @actions/cache, which generally speeds up the workflow.

I believe it should be the default behavior, but it's turned off by default in this PR for safety.

I also modified log messages to make the difference between runner's cache of tools and repository-local caching clear, since I find it a little bit confusing that setup-deno always downloads Deno from remote regardless of saying "Cached Deno to ...".

I refered to setup-zig for implementation: https://github.com/goto-bus-stop/setup-zig

Not sure the best way to test the feature, but I turned on the feature only for matrix.deno == "~1.32", since adding cache = [true, false] to the matrix seems too much.

You can see the result of tests on my forked repository: https://github.com/hasundue/setup-deno/actions/runs/6634339100
The first run is without cache in the first place, and the second run hits the caches. We can see some difference in the step duration as expected.

@hasundue
Copy link
Author

We can see some difference in the step duration as expected.

Maybe not... 😅

@hasundue
Copy link
Author

.@kt3k kindly pointed me out that actions/cache fetches repository-local cache from remote anyway, and thus it doesn't speed up the workflow as expected.

The feature is, however, still useful when you use a custom runner, like act, for example, where requests to the cache server may be redirected to a local server.

But the complexity introduced here may not be acceptable for those limited benefit, so please close this PR in that case.

@kt3k
Copy link
Member

kt3k commented Oct 31, 2023

As you pointed, we are not in favor of adding these complexities and also many dependencies because the benefit of this feature looks too limited. Closing without merge.

Thanks for your contribution anyway @hasundue

@kt3k kt3k closed this Oct 31, 2023
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.

2 participants