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(turborepo): pass through arguments only for root tasks #8089

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

Conversation

HelloWorld017
Copy link

@HelloWorld017 HelloWorld017 commented May 4, 2024

Description

In the document, it says that cli arguments are not passed through task dependencies.

Note that these additional arguments will not be passed to any additional tasks that are run due to dependencies from the pipeline configuration.

But when I actually run a task with additional arguments (e.g. turbo run dev -- --env=stage) the arguments (e.g. --env=stage) are passed through all the dependencies, and their caches are missed.

Related Issues: #1744 #5743

Fixes

Only pass the additional cli arguments to the root tasks. (= tasks which are not required by other tasks)

Testing Instructions

  1. Open the design-system example.
  2. In docs, run turbo run build --summarize -- --disable-telemetry.
  3. Ensure that --disable-telemetry is only passed in @repo/docs#build, not @acme/ui#build
  4. In docs, run turbo run build --summarize -- --test
  5. Ensure that @acme/ui#build is cached.

@HelloWorld017 HelloWorld017 requested a review from a team as a code owner May 4, 2024 16:14
@turbo-orchestrator turbo-orchestrator bot added needs: triage New issues get this label. Remove it after triage owned-by: turborepo labels May 4, 2024
Copy link

vercel bot commented May 4, 2024

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

7 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-native-web ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview May 5, 2024 7:52am

Copy link

vercel bot commented May 4, 2024

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

A member of the Team first needs to authorize it.

@HelloWorld017 HelloWorld017 force-pushed the fix/wrong-passthrough-args-cache-key branch from 11e8bb9 to 05409ce Compare May 5, 2024 07:52
@marisuxma
Copy link

This would be great!

@marisuxma
Copy link

@jaredpalmer

@mehulkar mehulkar requested review from chris-olszewski and removed request for gsoltis May 30, 2024 05:37
@chris-olszewski chris-olszewski self-assigned this May 30, 2024
@anthonyshew
Copy link
Contributor

Hey, sorry about this but that behavior must have been misdocumented. The behavior you're seeing is the one we intend to have and the docs reflect that today.

It's possible that I could bring up this behavior with the team and see if it could be a configuration or flag. Does that interest the both of you?

(Sidebar: @marisuxma, Jared doesn't work on this project anymore.) 😄

@anthonyshew anthonyshew removed the needs: triage New issues get this label. Remove it after triage label Jun 15, 2024
@turbo-orchestrator turbo-orchestrator bot added the needs: triage New issues get this label. Remove it after triage label Jun 15, 2024
@anthonyshew anthonyshew added the needs: team input Filter for core team meetings label Jun 16, 2024
@HelloWorld017
Copy link
Author

HelloWorld017 commented Jun 16, 2024

the docs reflect that today.

Actually the document does not describe the exact behaviour. Here's the summarized current behaviour.

  1. The arguments are passed to all tasks.
  2. The arguments are passed to all tasks, with same command name1
  3. The arguments are passed to only root tasks
1 2 3 Reference
Documentation
Cache Key task_hash.rs#L390
--summarize json summary/task_factory.rs#L162
Actual Running Task task_graph/visitor.rs#L637 opts.rs#L156
This PR

It's possible that I could bring up this behavior with the team and see if it could be a configuration or flag

Currently the Actual task running is different with cache key, which can create falsy cache, so I think they should be fixed.
And if all flags are passed through, ^build dependencies might cause turbo run test --coverage passing --coverage to build and turbo run dev --host passing --host to build.

Conclusion

So, if the 3 behavior (which is suggested by this PR) is not the preferred solution, I suggest making doucment / cache key / --summarize to reflect the current behavior (which is 2). And that can be represented like:

1 2 3 Reference
Documentation
Cache Key
--summarize json
Actual Running Task

If you want, I can open another PR, to do that.

Footnotes

  1. e.g. for test --coverage with ^build and ^test deps, the --coverage flag is only passed through the ^test deps.

@HelloWorld017
Copy link
Author

@anthonyshew (as a gentle reminder) would you check the comment I wrote?

@hchangjae
Copy link

@HelloWorld017
If you want to prevent unexpected additional parameters, This may help you.

#1744 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: team input Filter for core team meetings needs: triage New issues get this label. Remove it after triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants