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

chore: make env mode strict by default #8182

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -201,22 +201,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
Loading