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.

Sign up for GitHub

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

Jump to bottom

feat(turbo): add cache flag #9348

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat(turbo): add cache flag #9348

wants to merge 11 commits into from
+777 −137

Conversation

NicholasLYang
Copy link
Contributor

@NicholasLYang NicholasLYang commented Oct 29, 2024 •
edited
Loading

Description

Adds a cache flag that lets you set cache permissions per cache type and per action (read or write)

Testing Instructions

Adds tests in turborepo_cache/src/config.rs for parsing and in turborepo-lib/src/opts.rs for loading and resolving

@vercel Vercel
Copy link

vercel bot commented Oct 29, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 4, 2024 7:04pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 7:04pm

@NicholasLYang NicholasLYang marked this pull request as ready for review October 30, 2024 20:48
@NicholasLYang NicholasLYang requested a review from a team as a code owner October 30, 2024 20:48
chris-olszewski
chris-olszewski requested changes Oct 31, 2024
View reviewed changes
Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

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

I'm excited for this, biggest thing I'd like to see is some tests regarding how the new cache struct gets resolved wrt existing cache config settings.

crates/turborepo-lib/src/config/env.rs Show resolved Hide resolved
crates/turborepo-lib/src/opts.rs Outdated
@@ -362,16 +358,37 @@ impl<'a> TryFrom<OptsInputs<'a>> for ScopeOpts {
impl<'a> From<OptsInputs<'a>> for CacheOpts {
fn from(inputs: OptsInputs<'a>) -> Self {
let is_linked = turborepo_api_client::is_linked(inputs.api_auth);
let skip_remote = if !is_linked {
true
let mut cache = inputs.config.cache();
Copy link
Member

Choose a reason for hiding this comment

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

I think this layering logic is confusing since env vars or config settings can override CLI level cache settings:
e.g. TURBO_FORCE=1 turbo run build --force=false would result in remote:rw,local:rw cache behavior, but TURBO_REMOTE_ONLY=1 turbo run build --cache='local:rw would result in no caches are enabled.

I think it would make more sense to fold the existing cache flags into the new cache struct at each level in the config, but I'm not convinced that wouldn't be confusing. Could we get away with having the new cache struct conflict with existing cache flags at each level e.g turbo run build --remote-only --cache='...' would throw but TURBO_REMOTE_ONLY=1 turbo run build --cache='...' be fine where --cache wins out completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to have a group of flags conflict with a single flag but not with each other? I wasn't sure if that's possible in clap. Ofc I can manually add that as a restriction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The layering issue is a tricky one. If the config specifies remote:rw in a file, but then we add on local:rw, do we want to do an OR or an AND for those? I could see either one working

crates/turborepo-cache/src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tknickman tknickman Awaiting requested review from tknickman tknickman is a code owner automatically assigned from vercel/turbo-oss

@chris-olszewski chris-olszewski Awaiting requested review from chris-olszewski

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
created-by: turborepo owned-by: turborepo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NicholasLYang @chris-olszewski