No env var inference in strict mode. #4527

nathanhammond · 2023-04-11T07:15:55Z

We presently use framework inference to determine if we should automatically include certain environment variables into the hash. In strict environment variable mode we should not automatically include any env vars into the hash or the execution environment.

You must instead:

Specify all environment variables that you want available in your build environment.
Specify whether you want those considered for the hash (env) or merely passed through (passthroughEnv).

We will also apply this to "inferred strict" on a per-task basis. If you specify passthroughEnv we infer that your task is supposed to be in strict mode.

vercel · 2023-04-11T07:16:06Z

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

Name	Status	Preview	Comments	Updated (UTC)
examples-basic-web	🔄 Building (Inspect)			Apr 28, 2023 10:09am
examples-vite-web	🔄 Building (Inspect)			Apr 28, 2023 10:09am
turbo-site	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 28, 2023 10:09am

8 Ignored Deployments

Name	Status	Updated (UTC)
examples-cra-web	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-designsystem-docs	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-gatsby-web	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-native-web	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-nonmonorepo	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-svelte-web	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am
examples-tailwind-web	⬜️ Ignored (Inspect)	Apr 28, 2023 10:09am

vercel · 2023-04-11T07:16:07Z

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

A member of the Team first needs to authorize it.

gsoltis · 2023-04-11T16:59:39Z

I don't think we should skip inferred vars in strict mode. The point is to allow users to discover if they are relying on an env var that doesn't make it into the hash. If the env var does make it into the hash, for whatever reason, it should be fine to rely on it.

Is there perhaps another way that users could get the information about what was inferred? I think it's in the Run Summary, right?

nathanhammond

Reviewer's guide! Review commit-by-commit.

Adds a strict mode check.
Makes strict mode stricter, but creates a level underneath it that includes framework vars.
Is a docs patch for naming issues.

nathanhammond · 2023-04-27T11:44:30Z

cli/internal/run/real_run.go

@@ -292,7 +292,7 @@ func (ec *execContext) exec(ctx gocontext.Context, packageTask *nodes.PackageTas
 	currentState := env.GetEnvMap()
 	passthroughEnv := env.EnvironmentVariableMap{}

-	if packageTask.EnvMode == util.Strict {
+	if packageTask.EnvMode.IsStrict() {


There are now two strict modes with slightly different meanings.

nathanhammond · 2023-04-27T11:48:51Z

cli/internal/taskhash/taskhash.go

+	var framework *inference.Framework
+	envVarContainingExcludePrefix := ""
+
+	// In Strict environment variable mode we don't do framework env var inference.
+	// You must instead:
+	// - Specify all environment variables that you want available in your build environment.
+	// - Specify whether you want those considered for the hash or merely passed through.
+	//
+	// This allows advanced users to improve both cache hit ratio _and_ correctness with
+	// the tradeoff of increased configuration verbosity.
+	if packageTask.EnvMode != util.Strict {
+		envVarContainingExcludePrefix = "TURBO_CI_VENDOR_ENV_KEY"
+		framework := inference.InferFramework(packageTask.Pkg)
+		if framework != nil && framework.EnvMatcher != "" {
+			// log auto detected framework and env prefix
+			logger.Debug(fmt.Sprintf("auto detected framework for %s", packageTask.PackageName), "framework", framework.Slug, "env_prefix", framework.EnvMatcher)
+			keyMatchers = append(keyMatchers, framework.EnvMatcher)
+		}


In whitespace-ignoring review mode, you'll see that this is just an if statement, a delayed variable configuration, and a comment. It's the only material behavior change in this PR.

nathanhammond · 2023-04-27T11:49:16Z

cli/internal/taskhash/taskhash.go

 		keyMatchers,
-		"TURBO_CI_VENDOR_ENV_KEY",
+		envVarContainingExcludePrefix,


These remain unset in Strict mode, triggering the behavior change.

nathanhammond · 2023-04-27T11:51:15Z

cli/internal/util/run_opts.go

-	return []byte(strings.ToLower(string(s))), nil
+func (em EnvMode) MarshalText() (text []byte, err error) {
+	if em == StrictIncludeFrameworkVars {
+		return []byte("strict-include-framework-vars"), nil


I could include some fancy kebab-case handling thing, write my own, or just hardcode the singular edge case. I chose the one that wasn't a waste of time and bytes.

There are tests that include this value, so it won't regress.

nathanhammond · 2023-04-27T11:52:24Z

cli/internal/util/run_opts.go

+// IsStrict collapses the two Strict variants.
+func (em EnvMode) IsStrict() bool {
+	return em == Strict || em == StrictIncludeFrameworkVars


We sometimes need to check these together. This makes it less error-prone.

nathanhammond · 2023-04-27T11:54:57Z

turborepo-tests/integration/tests/run_summary/strict_env.t

+    "passthrough": null,
+    "globalPassthrough": null


That these are properly set is tested in framework_inference.t.

nathanhammond · 2023-04-27T12:00:12Z

crates/turborepo-lib/src/cli.rs

@@ -56,6 +56,7 @@ pub enum DryRunMode {
 pub enum EnvMode {
    Infer,
    Loose,
+    StrictIncludeFrameworkVars,


The name here is a bikeshed. Note that the name introducing friction for using this is intentional.

Happy to entertain other options but this is already less aggressive than my original. 😅

I think we should go with something a bit shorter, at least for the user-facing portions. We're intending to communicate that the env vars available will match the set of env vars that turbo considers to be dependencies, and that means we're going infer some vars. Some ideas:

inferred

hashed

hashed-only

auto

detected

I'm least wild about the "hashed" variants since we don't want to force users to think about hashing. For better or worse they already need to think about inference at least a little, so I think that's less bad. "auto" and "detected" are pretty general. After writing this out, my vote goes to "auto". It suggests that turbo is doing something beyond what the user has specified, and I think it also contrasts nicely with "strict": "do your best guess at what will work" vs "don't do anything, I'm going to specify everything". I'm open to other [short] suggestions though.

After implementation I don't think we can drop infer so we're going to be four states:

--infer: we figure it out per-task based on turbo.json. (collapses to loose or strict-include-framework-vars)

--loose: you get everything.

--strict: we include only what you tell us.

--strict-include-framework-vars: we include what you tell us and the framework vars.

This primary way to get this is: "you defined passThroughEnv but don't pass in a command line flag". This is what gets you strict-include-framework-vars. In general the user should rarely want or need to specify this.

The primary use case for manually specifying this flag is "I want strict mode for all my tasks, and for tasks that are Next.js I want to include NEXT_PUBLIC_*." Packages basically never change frameworks, so using this is really just a feature request for "globbing over env vars" in turbo.json."

The problem here is that "auto", "detect", and "infer" are all synonyms and are extremely difficult to differentiate so we need two names for things which will basically never be specified on the command line, one of which is the "wrong" solution to the problem:

"we pull it from turbo.json"

"strict + framework vars"

A lot of words to say:

I think the clarity of length is valuable. This length improves the run summary.

I believe that users will rarely, if ever, actually specify it.

The friction will push people toward doing it the way we want them to, which is more-easily shareable: in turbo.json, in env and passThroughEnv, and using a yet-unimplemented globbing feature.

nathanhammond · 2023-05-03T13:20:40Z

Closing in favor of #4788.

turbo-orchestrator bot added the team: turborepo label Apr 11, 2023

This comment was marked as resolved.

Sign in to view

turbo-orchestrator bot added the area: docs Improvements or additions to documentation label Apr 11, 2023

nathanhammond force-pushed the no-infer-when-strict branch from 6e1f7ec to 90b302c Compare April 11, 2023 13:50

nathanhammond force-pushed the no-infer-when-strict branch 2 times, most recently from 5c2e19a to 4055a6d Compare April 13, 2023 09:48

nathanhammond requested a review from mehulkar April 13, 2023 09:49

nathanhammond marked this pull request as ready for review April 13, 2023 09:50

nathanhammond requested a review from a team as a code owner April 13, 2023 09:50

nathanhammond requested a review from chris-olszewski April 13, 2023 09:50

vercel bot had a problem deploying to Preview – turbo-site April 13, 2023 09:52 Failure

nathanhammond force-pushed the no-infer-when-strict branch 2 times, most recently from ee55f32 to f0a80da Compare April 19, 2023 07:41

vercel deleted a comment from socket-security bot Apr 27, 2023

nathanhammond force-pushed the no-infer-when-strict branch 2 times, most recently from f1b82e3 to 7e8e4f2 Compare April 27, 2023 11:40

vercel bot had a problem deploying to Preview – turbo-site April 27, 2023 11:42 Failure

nathanhammond commented Apr 27, 2023

View reviewed changes

nathanhammond force-pushed the no-infer-when-strict branch from 7e8e4f2 to 81dbf96 Compare April 27, 2023 12:12

nathanhammond requested a review from gsoltis April 28, 2023 04:08

nathanhammond mentioned this pull request Apr 28, 2023

Stabilize --env-mode #4739

Merged

nathanhammond added 3 commits April 28, 2023 18:02

Do not infer for strict mode.

1e0c9f9

Add strict-include-framework-vars mode.

1b34f05

Clean up docs.

394d691

nathanhammond force-pushed the no-infer-when-strict branch from 81dbf96 to 394d691 Compare April 28, 2023 10:07

vercel bot deployed to Preview – turbo-site April 28, 2023 10:09 View deployment

nathanhammond closed this May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No env var inference in strict mode. #4527

No env var inference in strict mode. #4527

nathanhammond commented Apr 11, 2023 •

edited

vercel bot commented Apr 11, 2023 •

edited

vercel bot commented Apr 11, 2023

This comment was marked as resolved.

gsoltis commented Apr 11, 2023

nathanhammond left a comment

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

nathanhammond Apr 27, 2023

gsoltis Apr 27, 2023

nathanhammond Apr 28, 2023

nathanhammond Apr 28, 2023 •

edited

nathanhammond commented May 3, 2023

No env var inference in strict mode. #4527

No env var inference in strict mode. #4527

Conversation

nathanhammond commented Apr 11, 2023 • edited

vercel bot commented Apr 11, 2023 • edited

vercel bot commented Apr 11, 2023

This comment was marked as resolved.

gsoltis commented Apr 11, 2023

nathanhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanhammond Apr 28, 2023 • edited

Choose a reason for hiding this comment

nathanhammond commented May 3, 2023

nathanhammond commented Apr 11, 2023 •

edited

vercel bot commented Apr 11, 2023 •

edited

nathanhammond Apr 28, 2023 •

edited