Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feature: enhance dry-run with cache status #1988

Merged
merged 22 commits into from
Sep 22, 2022
Merged

Conversation

sppatel
Copy link
Contributor

@sppatel sppatel commented Sep 16, 2022

This PR attempts to resolve #1437. Dry run is extended to include a cache state. During dry run execution we reach out to the cache via a HEAD request to verify the cache entry at the location represented by the cache. Note that this may require adoption from remote service implementations to support (e.g implementing a HEAD route).

Would appreciate any feedback/advice.

image
image

@sppatel sppatel requested a review from a team as a code owner September 16, 2022 17:54
@sppatel sppatel requested review from gsoltis and gaspar09 September 16, 2022 17:54
@vercel
Copy link

vercel bot commented Sep 16, 2022

@sppatel is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this!

I have a few hopefully-minor cosmetic requests, and one feature request.

Looking forward to getting this merged!

@sppatel
Copy link
Contributor Author

sppatel commented Sep 16, 2022

Appreciate the quick feed back - will address the review comments.

@sppatel
Copy link
Contributor Author

sppatel commented Sep 20, 2022

Ok - I think I addressed the current set of review comments. One thing I realized was that this "may" require remote cache implementations to support HEAD requests to take advantage of this feature.

@sppatel
Copy link
Contributor Author

sppatel commented Sep 20, 2022

@gsoltis I also locally implemented HEAD support for https://github.com/ducktors/turborepo-remote-cache - in order to verify functionality.

@sppatel sppatel requested review from gsoltis and removed request for gaspar09 September 21, 2022 11:40
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. I think a couple minor details then let's get this merged!

@gsoltis
Copy link
Contributor

gsoltis commented Sep 21, 2022

@sppatel I think the linter issue is real, the other test failure looks unrelated.

@sppatel
Copy link
Contributor Author

sppatel commented Sep 21, 2022

How can I rerun to verify the lint fixes?

Locally i also received this error on the naming of CacheState

exported: type name will be used as cache.CacheState by other packages, and that stutters; consider calling this State (revive)go-golangci-lint

I went with the recommended name of "State" - but if you'd want something different/more explicit - happy to change it.

@gsoltis
Copy link
Contributor

gsoltis commented Sep 21, 2022

make lint-go will run the linter locally. You might need to brew install golangci-lint if you don't have it already.

re: State, maybe ItemStatus or something like that? We're going to have a CacheItem soon, so I think ItemStatus will make sense in context: https://github.com/vercel/turborepo/pull/1991/files#diff-b0746dbe930e7be5e528cdaf2505200affe533e60e22316a1f03616f4e68f2c3

@sppatel
Copy link
Contributor Author

sppatel commented Sep 22, 2022

Mind re-kicking the status checks again when u get a moment to see if we're good now?

@vercel
Copy link

vercel bot commented Sep 22, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
turbo-site ✅ Ready (Inspect) Visit Preview Sep 22, 2022 at 5:13PM (UTC)

@gsoltis
Copy link
Contributor

gsoltis commented Sep 22, 2022

Test failure is unrelated. Force-merging.

@gsoltis gsoltis merged commit 017f5a3 into vercel:main Sep 22, 2022
@gsoltis
Copy link
Contributor

gsoltis commented Sep 22, 2022

@sppatel I know this is something people have been asking for, thanks for diving in on it!

@sppatel
Copy link
Contributor Author

sppatel commented Sep 22, 2022

Absolutely! Appreciate all the help and feedback!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide preflight command to precalculate fingerprints and determine cache hit/miss
2 participants