Skip to content

Commit

Permalink
chore: make env mode strict by default (#8182)
Browse files Browse the repository at this point in the history
### Description

Commandeered from #8151

Changes default env mode to strict and remove the "infer" option

### Testing Instructions

Existing test suite

(Currently some Windows integration tests are failing due to a missing
env var, opening for review while I work on this)

---------

Co-authored-by: nicholaslyang <[email protected]>
  • Loading branch information
chris-olszewski and NicholasLYang committed May 28, 2024
1 parent d560bb2 commit dedd0c0
Show file tree
Hide file tree
Showing 60 changed files with 291 additions and 430 deletions.
16 changes: 0 additions & 16 deletions crates/turborepo-env/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,6 @@ use thiserror::Error;

const DEFAULT_ENV_VARS: [&str; 1] = ["VERCEL_ANALYTICS_ID"];

/// Environment mode after we've resolved the `Infer` variant
#[derive(Debug, Clone, Copy)]
pub enum ResolvedEnvMode {
Loose,
Strict,
}

impl std::fmt::Display for ResolvedEnvMode {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ResolvedEnvMode::Loose => write!(f, "loose"),
ResolvedEnvMode::Strict => write!(f, "strict"),
}
}
}

#[derive(Clone, Debug, Error)]
pub enum Error {
#[error("Failed to parse regex: {0}")]
Expand Down
38 changes: 10 additions & 28 deletions crates/turborepo-lib/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,16 @@ impl Display for DryRunMode {
}

#[derive(Copy, Clone, Debug, Default, PartialEq, Serialize, ValueEnum)]
#[serde(rename_all = "lowercase")]
pub enum EnvMode {
#[default]
Infer,
Loose,
#[default]
Strict,
}

impl fmt::Display for EnvMode {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(match self {
EnvMode::Infer => "infer",
EnvMode::Loose => "loose",
EnvMode::Strict => "strict",
})
Expand Down Expand Up @@ -677,9 +676,7 @@ pub struct ExecutionArgs {
/// Environment variable mode.
/// Use "loose" to pass the entire existing environment.
/// Use "strict" to use an allowlist specified in turbo.json.
/// Use "infer" to defer to existence of "passThroughEnv" or
/// "globalPassThroughEnv" in turbo.json. (default infer)
#[clap(long = "env-mode", default_value = "infer", num_args = 0..=1, default_missing_value = "infer")]
#[clap(long = "env-mode", default_value = "strict", num_args = 0..=1, default_missing_value = "strict")]
pub env_mode: EnvMode,
/// Use the given selector to specify package(s) to act as
/// entry points. The syntax mirrors pnpm's syntax, and
Expand Down Expand Up @@ -1449,49 +1446,34 @@ mod test {
"framework_inference: flag set to false"
)]
#[test_case::test_case(
&["turbo", "run", "build"],
Args {
command: Some(Command::Run {
execution_args: Box::new(ExecutionArgs {
tasks: vec!["build".to_string()],
env_mode: EnvMode::Infer,
..get_default_execution_args()
}),
run_args: Box::new(get_default_run_args())
}),
..Args::default()
} ;
"env_mode: default infer"
)]
#[test_case::test_case(
&["turbo", "run", "build", "--env-mode"],
&["turbo", "run", "build", "--env-mode"],
Args {
command: Some(Command::Run {
execution_args: Box::new(ExecutionArgs {
tasks: vec!["build".to_string()],
env_mode: EnvMode::Infer,
env_mode: EnvMode::Strict,
..get_default_execution_args()
}),
run_args: Box::new(get_default_run_args())
}),
..Args::default()
} ;
} ;
"env_mode: not fully-specified"
)]
)]
#[test_case::test_case(
&["turbo", "run", "build", "--env-mode", "infer"],
&["turbo", "run", "build"],
Args {
command: Some(Command::Run {
execution_args: Box::new(ExecutionArgs {
tasks: vec!["build".to_string()],
env_mode: EnvMode::Infer,
env_mode: EnvMode::Strict,
..get_default_execution_args()
}),
run_args: Box::new(get_default_run_args())
}),
..Args::default()
} ;
"env_mode: specified infer"
"env_mode: default strict"
)]
#[test_case::test_case(
&["turbo", "run", "build", "--env-mode", "loose"],
Expand Down
22 changes: 9 additions & 13 deletions crates/turborepo-lib/src/hash/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ use std::collections::HashMap;

use capnp::message::{Builder, HeapAllocator};
pub use traits::TurboHash;
use turborepo_env::{EnvironmentVariablePairs, ResolvedEnvMode};
use turborepo_env::EnvironmentVariablePairs;

use crate::{cli::EnvMode, task_graph::TaskOutputs};

mod proto_capnp {
use turborepo_env::ResolvedEnvMode;

use crate::cli::EnvMode;

Expand All @@ -24,18 +23,17 @@ mod proto_capnp {
impl From<EnvMode> for global_hashable::EnvMode {
fn from(value: EnvMode) -> Self {
match value {
EnvMode::Infer => global_hashable::EnvMode::Infer,
EnvMode::Loose => global_hashable::EnvMode::Loose,
EnvMode::Strict => global_hashable::EnvMode::Strict,
}
}
}

impl From<ResolvedEnvMode> for task_hashable::EnvMode {
fn from(value: ResolvedEnvMode) -> Self {
impl From<EnvMode> for task_hashable::EnvMode {
fn from(value: EnvMode) -> Self {
match value {
ResolvedEnvMode::Loose => task_hashable::EnvMode::Loose,
ResolvedEnvMode::Strict => task_hashable::EnvMode::Strict,
EnvMode::Loose => task_hashable::EnvMode::Loose,
EnvMode::Strict => task_hashable::EnvMode::Strict,
}
}
}
Expand All @@ -59,7 +57,7 @@ pub struct TaskHashable<'a> {
pub(crate) env: &'a [String],
pub(crate) resolved_env_vars: EnvVarPairs,
pub(crate) pass_through_env: &'a [String],
pub(crate) env_mode: ResolvedEnvMode,
pub(crate) env_mode: EnvMode,
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -334,7 +332,6 @@ impl From<GlobalHashable<'_>> for Builder<HeapAllocator> {
}

builder.set_env_mode(match hashable.env_mode {
EnvMode::Infer => proto_capnp::global_hashable::EnvMode::Infer,
EnvMode::Loose => proto_capnp::global_hashable::EnvMode::Loose,
EnvMode::Strict => proto_capnp::global_hashable::EnvMode::Strict,
});
Expand All @@ -361,7 +358,6 @@ impl From<GlobalHashable<'_>> for Builder<HeapAllocator> {
#[cfg(test)]
mod test {
use test_case::test_case;
use turborepo_env::ResolvedEnvMode;
use turborepo_lockfiles::Package;

use super::{
Expand All @@ -386,7 +382,7 @@ mod test {
env: &["env".to_string()],
resolved_env_vars: vec![],
pass_through_env: &["pass_thru_env".to_string()],
env_mode: ResolvedEnvMode::Loose,
env_mode: EnvMode::Loose,
};

assert_eq!(task_hashable.hash(), "1f8b13161f57fca1");
Expand All @@ -408,11 +404,11 @@ mod test {
env: &["env".to_string()],
resolved_env_vars: vec![],
pass_through_env: &["pass_through_env".to_string()],
env_mode: EnvMode::Infer,
env_mode: EnvMode::Strict,
framework_inference: true,
};

assert_eq!(global_hash.hash(), "2144612ff08bddb9");
assert_eq!(global_hash.hash(), "9f06917065be0a72");
}

#[test_case(vec![], "459c029558afe716" ; "empty")]
Expand Down
5 changes: 2 additions & 3 deletions crates/turborepo-lib/src/hash/proto.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ struct GlobalHashable {


enum EnvMode {
infer @0;
loose @1;
strict @2;
loose @0;
strict @1;
}

struct Entry {
Expand Down
8 changes: 1 addition & 7 deletions crates/turborepo-lib/src/run/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use {
};

use crate::{
cli::{DryRunMode, EnvMode},
cli::DryRunMode,
commands::CommandBase,
engine::{Engine, EngineBuilder},
opts::Opts,
Expand Down Expand Up @@ -387,12 +387,6 @@ impl RunBuilder {
self.opts.run_opts.dry_run.is_some(),
));

if matches!(self.opts.run_opts.env_mode, EnvMode::Infer)
&& root_turbo_json.global_pass_through_env.is_some()
{
self.opts.run_opts.env_mode = EnvMode::Strict;
}

let should_print_prelude = self.should_print_prelude_override.unwrap_or_else(|| {
self.opts.run_opts.dry_run.is_none() && self.opts.run_opts.graph.is_none()
});
Expand Down
4 changes: 2 additions & 2 deletions crates/turborepo-lib/src/run/global_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::{

static DEFAULT_ENV_VARS: [&str; 1] = ["VERCEL_ANALYTICS_ID"];

const GLOBAL_CACHE_KEY: &str = "HEY STELLLLLLLAAAAAAAAAAAAA";
const GLOBAL_CACHE_KEY: &str = "I can’t see ya, but I know you’re here";

#[derive(Debug, Error)]
pub enum Error {
Expand Down Expand Up @@ -222,7 +222,7 @@ mod tests {
&env_var_map,
&[],
None,
EnvMode::Infer,
EnvMode::Strict,
false,
&SCM::new(&root),
);
Expand Down
17 changes: 4 additions & 13 deletions crates/turborepo-lib/src/run/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,22 +260,13 @@ impl Run {
is_monorepo.then(|| get_external_deps_hash(&root_workspace.transitive_dependencies));

let global_hash_inputs = {
let (env_mode, pass_through_env) = match self.opts.run_opts.env_mode {
// In infer mode, if there is any pass_through config (even if it is an empty array)
// we'll hash the whole object, so we can detect changes to that config
// Further, resolve the envMode to the concrete value.
EnvMode::Infer if self.root_turbo_json.global_pass_through_env.is_some() => (
EnvMode::Strict,
self.root_turbo_json.global_pass_through_env.as_deref(),
),
let env_mode = self.opts.run_opts.env_mode;
let pass_through_env = match env_mode {
EnvMode::Loose => {
// Remove the passthroughs from hash consideration if we're explicitly loose.
(EnvMode::Loose, None)
None
}
env_mode => (
env_mode,
self.root_turbo_json.global_pass_through_env.as_deref(),
),
EnvMode::Strict => self.root_turbo_json.global_pass_through_env.as_deref(),
};

get_global_hash_inputs(
Expand Down
24 changes: 2 additions & 22 deletions crates/turborepo-lib/src/run/summary/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use self::{
use super::task_id::TaskId;
use crate::{
cli,
cli::DryRunMode,
cli::{DryRunMode, EnvMode},
engine::Engine,
opts::RunOpts,
run::summary::{
Expand Down Expand Up @@ -83,26 +83,6 @@ enum RunType {
DryJson,
}

// Can't reuse `cli::EnvMode` because the serialization
// is different (lowercase vs uppercase)
#[derive(Clone, Copy, Debug, Serialize)]
#[serde(rename_all = "lowercase")]
pub enum EnvMode {
Infer,
Loose,
Strict,
}

impl From<cli::EnvMode> for EnvMode {
fn from(env_mode: cli::EnvMode) -> Self {
match env_mode {
cli::EnvMode::Infer => EnvMode::Infer,
cli::EnvMode::Loose => EnvMode::Loose,
cli::EnvMode::Strict => EnvMode::Strict,
}
}
}

#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct RunSummary<'a> {
Expand Down Expand Up @@ -300,7 +280,7 @@ impl RunTracker {
run_opts,
packages,
global_hash_summary,
global_env_mode.into(),
global_env_mode,
task_factory,
)
.await?;
Expand Down
19 changes: 2 additions & 17 deletions crates/turborepo-lib/src/run/summary/task_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use turborepo_repository::package_graph::{PackageGraph, PackageInfo, PackageName
use super::{
execution::TaskExecutionSummary,
task::{SharedTaskSummary, TaskEnvVarSummary},
EnvMode, SinglePackageTaskSummary, TaskSummary,
SinglePackageTaskSummary, TaskSummary,
};
use crate::{
cli,
Expand Down Expand Up @@ -175,22 +175,7 @@ impl<'a> TaskSummaryFactory<'a> {
framework,
dependencies,
dependents,
// TODO: this is some very messy code that appears in a few places
// we should attempt to calculate this once and reuse it
env_mode: match self.global_env_mode {
cli::EnvMode::Infer => {
if task_definition.pass_through_env.is_some() {
EnvMode::Strict
} else {
// If we're in infer mode we have just detected non-usage of strict env
// vars. But our behavior's actual meaning of this
// state is `loose`.
EnvMode::Loose
}
}
cli::EnvMode::Strict => EnvMode::Strict,
cli::EnvMode::Loose => EnvMode::Loose,
},
env_mode: self.global_env_mode,
environment_variables: TaskEnvVarSummary::new(
task_definition,
env_vars,
Expand Down
15 changes: 2 additions & 13 deletions crates/turborepo-lib/src/task_graph/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use tokio::sync::{mpsc, oneshot};
use tracing::{debug, error, Instrument, Span};
use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPath};
use turborepo_ci::{Vendor, VendorBehavior};
use turborepo_env::{EnvironmentVariableMap, ResolvedEnvMode};
use turborepo_env::EnvironmentVariableMap;
use turborepo_repository::{
package_graph::{PackageGraph, PackageName, ROOT_PKG_NAME},
package_manager::PackageManager,
Expand Down Expand Up @@ -195,18 +195,7 @@ impl<'a> Visitor<'a> {
.task_definition(&info)
.ok_or(Error::MissingDefinition)?;

let task_env_mode = match self.global_env_mode {
// Task env mode is only independent when global env mode is `infer`.
EnvMode::Infer if task_definition.pass_through_env.is_some() => {
ResolvedEnvMode::Strict
}
// If we're in infer mode we have just detected non-usage of strict env vars.
// But our behavior's actual meaning of this state is `loose`.
EnvMode::Infer => ResolvedEnvMode::Loose,
// Otherwise we just use the global env mode.
EnvMode::Strict => ResolvedEnvMode::Strict,
EnvMode::Loose => ResolvedEnvMode::Loose,
};
let task_env_mode = self.global_env_mode;
package_task_event.track_env_mode(&task_env_mode.to_string());

let dependency_set = engine.dependencies(&info).ok_or(Error::MissingDefinition)?;
Expand Down
Loading

0 comments on commit dedd0c0

Please sign in to comment.