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

handle VERCEL_ARTIFACTS_* env vars override #9249

Merged
merged 18 commits into from
Oct 18, 2024

Conversation

dimitropoulos
Copy link
Contributor

@dimitropoulos dimitropoulos commented Oct 11, 2024

Description

The prior intended state of EnvVars overrides looked like this:

`VERCEL_ARTIFACTS_*` always wins



But we've realized this is not good enough. We want to better handle the scenario where users have provided custom remote cache - even if they deploy on Vercel.

After this PR, the precedence works like this.

  1. if both TURBO_TEAMID+TURBO_TOKEN are set we use those. if TURBO_TEAM is also set, we pass that along, too.
  2. if both TURBO_TEAM+TURBO_TOKEN are set we use those. if TURBO_TEAMID is also set we pass that along, too.
  3. if both VERCEL_ARTIFACTS_OWNER+VERCEL_ARTIFACTS_TOKEN are set, we use those (all else is ignored).
  4. if both TURBO_TEAMID+TURBO_TEAM, we pass those along.
  5. if just TURBO_TEAMID is set, we pass that along.
  6. if just TURBO_TEAM is set, we pass that along.
  7. if both TURBO_TEAM and VERCEL_ARTIFACTS_OWNER are set, we pass those along
  8. if just VERCEL_ARTIFACTS_OWNER is set, we pass that along
  9. we do not support or consider partial configurations. For example if only VERCEL_ARTIFACTS_TOKEN and TURBO_TEAMID are set, this is considered undefined/unsupported behavior. The reasons are that:
  • this shouldn't ever happen - Vercel sets both or none of the VERCEL_ARTIFACTS_* family
  • we tried doing this, but the combinatorial explosion that results is pretty major, and some of the cases are very strange to reason about, considering the above point that we don't expect there to be any genuine scenario where this happens, anyway.

Testing Instructions

Take a look at the newly added tests in override_env.rs. You could run turbo while manually setting those same env vars.

Copy link

vercel bot commented Oct 11, 2024

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 Oct 17, 2024 9:58pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Oct 17, 2024 9:58pm

@dimitropoulos dimitropoulos force-pushed the dimitri/vercel-env-vars-override branch from cc28be2 to 5de06ba Compare October 15, 2024 23:56
@dimitropoulos dimitropoulos changed the title [WIP] VERCEL_ARTIFACTS_* env vars override handle VERCEL_ARTIFACTS_* env vars override Oct 15, 2024
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.

This is an amazing write up of the existing + desired behavior.

The only blocking comment is making sure that we keep the same level of user facing error messaging if we encounter a non-UTF8 value.

I feel that changing the parameter for get_configuration_options doesn't provide us with much, but that's just an opinion.

crates/turbo-trace/src/main.rs Show resolved Hide resolved
crates/turborepo-lib/src/config/env.rs Outdated Show resolved Hide resolved
crates/turborepo-lib/src/config/mod.rs Show resolved Hide resolved
crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
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.

LGTM!

Comment on the Output::from implementation is total taste and up to you.

crates/turborepo-lib/src/config/override_env.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Hi there 👋

It looks like this PR introduces broken links to the docs, please take a moment to fix them before merging:

Broken link Type File
/repo/docs/guides/tools/typescript#you-likely-dont-need-a-tsconfigjson-file-in-the-root-of-your-project hash /repo-docs/crafting-your-repository/structuring-a-repository.mdx
#package-entrypoint-wildcards hash /repo-docs/guides/tools/typescript.mdx
#package-entrypoint-wildcards hash /repo-docs/guides/tools/typescript.mdx

Thank you 🙏

@dimitropoulos dimitropoulos force-pushed the dimitri/vercel-env-vars-override branch from 531c833 to 65a4203 Compare October 18, 2024 00:20
@turbo-orchestrator turbo-orchestrator bot removed the area: docs Improvements or additions to documentation label Oct 18, 2024
- ConfigurationOptions.token corresponds to TURBO_TOKEN or VERCEL_ARTIFACTS_TOKEN
- ConfigurationOptions.team_id corresponds to TURBO_TEAMID or VERCEL_ARTIFACTS_OWNER
- ConfigurationOptions.team_slug corresponds to TURBO_TEAM
1. We're ultimately poking around the env vars looking for _paris_ that make sense.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. We're ultimately poking around the env vars looking for _paris_ that make sense.
1. We're ultimately poking around the env vars looking for _pairs_ that make sense.

Comment on lines +10 to +17
Hi! If you're new here:
1. The general pattern is that:
- ConfigurationOptions.token corresponds to TURBO_TOKEN or VERCEL_ARTIFACTS_TOKEN
- ConfigurationOptions.team_id corresponds to TURBO_TEAMID or VERCEL_ARTIFACTS_OWNER
- ConfigurationOptions.team_slug corresponds to TURBO_TEAM
1. We're ultimately poking around the env vars looking for _pairs_ that make sense.
Since we presume that users are the only ones sending TURBO_* and Vercel is the only one sending VERCEL_*, we can make some assumptions. Namely, we assume that if we have one of VERCEL_ARTIFACTS_OWNER or VERCEL_ARTIFACTS_TOKEN we will always have both.
1. Watch out for mixing up `TURBO_TEAM` and `TURBO_TEAMID`. Same for ConfigurationOptions.team_id and ConfigurationOptions.team_slug.
Copy link
Member

Choose a reason for hiding this comment

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

🙏🏼

@tknickman tknickman merged commit 9375ef9 into main Oct 18, 2024
67 checks passed
@tknickman tknickman deleted the dimitri/vercel-env-vars-override branch October 18, 2024 20:24
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.

3 participants