From 1ee3f85f51c7897c2c93c047b6c8af3b34a08960 Mon Sep 17 00:00:00 2001 From: Nicholas Yang Date: Tue, 16 Jul 2024 13:03:20 -0400 Subject: [PATCH] refactor(turborepo): derive `Opts` from `Config` (#8759) ### Description Changed `Opts` to be derived from more than just `RunArgs` and `ExecutionArgs`. This means that `Opts` contains the fully resolved config, which gets rid of some messy mutable logic in `RunBuilder` and creates a single source of truth. ### Testing Instructions Existing tests cover this pretty well. --- crates/turborepo-api-client/src/lib.rs | 10 ++ crates/turborepo-lib/src/cli/mod.rs | 29 +--- crates/turborepo-lib/src/opts.rs | 129 +++++++++++------- crates/turborepo-lib/src/run/builder.rs | 28 +--- turborepo-tests/integration/tests/no-args.t | 2 +- .../integration/tests/turbo-help.t | 3 +- 6 files changed, 106 insertions(+), 95 deletions(-) diff --git a/crates/turborepo-api-client/src/lib.rs b/crates/turborepo-api-client/src/lib.rs index e541fca9e7ed1..02863c59ee3c8 100644 --- a/crates/turborepo-api-client/src/lib.rs +++ b/crates/turborepo-api-client/src/lib.rs @@ -123,6 +123,16 @@ pub struct APIAuth { pub team_slug: Option, } +impl std::fmt::Debug for APIAuth { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("APIAuth") + .field("team_id", &self.team_id) + .field("token", &"***") + .field("team_slug", &self.team_slug) + .finish() + } +} + pub fn is_linked(api_auth: &Option) -> bool { api_auth .as_ref() diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index 7f54a52368df1..f4b399b434cfc 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -682,8 +682,8 @@ pub struct ExecutionArgs { /// Environment variable mode. /// Use "loose" to pass the entire existing environment. /// Use "strict" to use an allowlist specified in turbo.json. - #[clap(long = "env-mode", default_value = "strict", num_args = 0..=1, default_missing_value = "strict")] - pub env_mode: EnvMode, + #[clap(long = "env-mode", num_args = 0..=1, default_missing_value = "strict")] + pub env_mode: Option, /// Use the given selector to specify package(s) to act as /// entry points. The syntax mirrors pnpm's syntax, and /// additional documentation and examples can be found in @@ -755,8 +755,8 @@ impl ExecutionArgs { ); } - if self.env_mode != EnvMode::default() { - telemetry.track_arg_value("env-mode", self.env_mode, EventType::NonSensitive); + if let Some(env_mode) = self.env_mode { + telemetry.track_arg_value("env-mode", env_mode, EventType::NonSensitive); } if let Some(output_logs) = &self.output_logs { @@ -1443,7 +1443,7 @@ mod test { command: Some(Command::Run { execution_args: Box::new(ExecutionArgs { tasks: vec!["build".to_string()], - env_mode: EnvMode::Strict, + env_mode: Some(EnvMode::Strict), ..get_default_execution_args() }), run_args: Box::new(get_default_run_args()) @@ -1452,28 +1452,13 @@ mod test { } ; "env_mode: not fully-specified" )] - #[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::Strict, - ..get_default_execution_args() - }), - run_args: Box::new(get_default_run_args()) - }), - ..Args::default() - } ; - "env_mode: default strict" - )] #[test_case::test_case( &["turbo", "run", "build", "--env-mode", "loose"], Args { command: Some(Command::Run { execution_args: Box::new(ExecutionArgs { tasks: vec!["build".to_string()], - env_mode: EnvMode::Loose, + env_mode: Some(EnvMode::Loose), ..get_default_execution_args() }), run_args: Box::new(get_default_run_args()) @@ -1488,7 +1473,7 @@ mod test { command: Some(Command::Run { execution_args: Box::new(ExecutionArgs { tasks: vec!["build".to_string()], - env_mode: EnvMode::Strict, + env_mode: Some(EnvMode::Strict), ..get_default_execution_args() }), run_args: Box::new(get_default_run_args()) diff --git a/crates/turborepo-lib/src/opts.rs b/crates/turborepo-lib/src/opts.rs index cb109a49f9ac9..1e31ead1b7279 100644 --- a/crates/turborepo-lib/src/opts.rs +++ b/crates/turborepo-lib/src/opts.rs @@ -2,14 +2,16 @@ use std::{backtrace, backtrace::Backtrace}; use thiserror::Error; use turbopath::AnchoredSystemPathBuf; -use turborepo_cache::CacheOpts; +use turborepo_api_client::APIAuth; +use turborepo_cache::{CacheOpts, RemoteCacheOpts}; use crate::{ cli::{ Command, DryRunMode, EnvMode, ExecutionArgs, LogOrder, LogPrefix, OutputLogsMode, RunArgs, }, + commands::CommandBase, + config::ConfigurationOptions, run::task_id::TaskId, - Args, }; #[derive(Debug, Error)] @@ -30,6 +32,8 @@ pub enum Error { ConcurrencyOutOfBounds(#[backtrace] backtrace::Backtrace, String), #[error(transparent)] Path(#[from] turbopath::PathError), + #[error(transparent)] + Config(#[from] crate::config::Error), } #[derive(Debug)] @@ -76,10 +80,12 @@ impl Opts { } } -impl<'a> TryFrom<&'a Args> for Opts { - type Error = self::Error; +impl Opts { + pub fn new(base: &CommandBase) -> Result { + let args = base.args(); + let config = base.config()?; + let api_auth = base.api_auth()?; - fn try_from(args: &'a Args) -> Result { let Some(Command::Run { run_args, execution_args, @@ -87,9 +93,12 @@ impl<'a> TryFrom<&'a Args> for Opts { else { return Err(Error::ExpectedRun(Backtrace::capture())); }; - let run_and_execution_args = RunAndExecutionArgs { + + let run_and_execution_args = OptsInputs { run_args: run_args.as_ref(), execution_args: execution_args.as_ref(), + config, + api_auth: &api_auth, }; let run_opts = RunOpts::try_from(run_and_execution_args)?; let cache_opts = CacheOpts::from(run_and_execution_args); @@ -105,11 +114,12 @@ impl<'a> TryFrom<&'a Args> for Opts { } } -// This is not ideal, but it allows us to impl From #[derive(Debug, Clone, Copy)] -struct RunAndExecutionArgs<'a> { +struct OptsInputs<'a> { run_args: &'a RunArgs, execution_args: &'a ExecutionArgs, + config: &'a ConfigurationOptions, + api_auth: &'a Option, } #[derive(Debug, Default)] @@ -119,12 +129,12 @@ pub struct RunCacheOpts { pub(crate) task_output_logs_override: Option, } -impl<'a> From> for RunCacheOpts { - fn from(args: RunAndExecutionArgs<'a>) -> Self { +impl<'a> From> for RunCacheOpts { + fn from(inputs: OptsInputs<'a>) -> Self { RunCacheOpts { - skip_reads: args.execution_args.force.flatten().is_some_and(|f| f), - skip_writes: args.run_args.no_cache, - task_output_logs_override: args.execution_args.output_logs, + skip_reads: inputs.execution_args.force.flatten().is_some_and(|f| f), + skip_writes: inputs.run_args.no_cache, + task_output_logs_override: inputs.execution_args.output_logs, } } } @@ -187,11 +197,11 @@ pub enum ResolvedLogPrefix { const DEFAULT_CONCURRENCY: u32 = 10; -impl<'a> TryFrom> for RunOpts { +impl<'a> TryFrom> for RunOpts { type Error = self::Error; - fn try_from(args: RunAndExecutionArgs) -> Result { - let concurrency = args + fn try_from(inputs: OptsInputs) -> Result { + let concurrency = inputs .execution_args .concurrency .as_deref() @@ -199,16 +209,16 @@ impl<'a> TryFrom> for RunOpts { .transpose()? .unwrap_or(DEFAULT_CONCURRENCY); - let graph = args.run_args.graph.as_deref().map(|file| match file { + let graph = inputs.run_args.graph.as_deref().map(|file| match file { "" => GraphOpts::Stdout, f => GraphOpts::File(f.to_string()), }); - let (is_github_actions, log_order, log_prefix) = match args.execution_args.log_order { + let (is_github_actions, log_order, log_prefix) = match inputs.execution_args.log_order { LogOrder::Auto if turborepo_ci::Vendor::get_constant() == Some("GITHUB_ACTIONS") => ( true, ResolvedLogOrder::Grouped, - match args.execution_args.log_prefix { + match inputs.execution_args.log_prefix { LogPrefix::Task => ResolvedLogPrefix::Task, _ => ResolvedLogPrefix::None, }, @@ -218,33 +228,37 @@ impl<'a> TryFrom> for RunOpts { LogOrder::Auto | LogOrder::Stream => ( false, ResolvedLogOrder::Stream, - args.execution_args.log_prefix.into(), + inputs.execution_args.log_prefix.into(), ), LogOrder::Grouped => ( false, ResolvedLogOrder::Grouped, - args.execution_args.log_prefix.into(), + inputs.execution_args.log_prefix.into(), ), }; Ok(Self { - tasks: args.execution_args.tasks.clone(), + tasks: inputs.execution_args.tasks.clone(), log_prefix, log_order, - summarize: args.run_args.summarize, - experimental_space_id: args.run_args.experimental_space_id.clone(), - framework_inference: args.execution_args.framework_inference, - env_mode: args.execution_args.env_mode, + summarize: inputs.run_args.summarize, + experimental_space_id: inputs + .run_args + .experimental_space_id + .clone() + .or(inputs.config.spaces_id().map(|s| s.to_owned())), + framework_inference: inputs.execution_args.framework_inference, concurrency, - parallel: args.run_args.parallel, - profile: args.run_args.profile.clone(), - continue_on_error: args.execution_args.continue_execution, - pass_through_args: args.execution_args.pass_through_args.clone(), - only: args.execution_args.only, - daemon: args.run_args.daemon(), - single_package: args.execution_args.single_package, + parallel: inputs.run_args.parallel, + profile: inputs.run_args.profile.clone(), + continue_on_error: inputs.execution_args.continue_execution, + pass_through_args: inputs.execution_args.pass_through_args.clone(), + only: inputs.execution_args.only, + daemon: inputs.run_args.daemon(), + single_package: inputs.execution_args.single_package, graph, - dry_run: args.run_args.dry_run, + dry_run: inputs.run_args.dry_run, + env_mode: inputs.execution_args.env_mode.unwrap_or_default(), is_github_actions, }) } @@ -288,11 +302,11 @@ pub struct ScopeOpts { pub filter_patterns: Vec, } -impl<'a> TryFrom> for ScopeOpts { +impl<'a> TryFrom> for ScopeOpts { type Error = self::Error; - fn try_from(args: RunAndExecutionArgs<'a>) -> Result { - let pkg_inference_root = args + fn try_from(inputs: OptsInputs<'a>) -> Result { + let pkg_inference_root = inputs .execution_args .pkg_inference_root .as_ref() @@ -300,21 +314,44 @@ impl<'a> TryFrom> for ScopeOpts { .transpose()?; Ok(Self { - global_deps: args.execution_args.global_deps.clone(), + global_deps: inputs.execution_args.global_deps.clone(), pkg_inference_root, - filter_patterns: args.execution_args.filter.clone(), + filter_patterns: inputs.execution_args.filter.clone(), }) } } -impl<'a> From> for CacheOpts { - fn from(args: RunAndExecutionArgs<'a>) -> Self { +impl<'a> From> for CacheOpts { + fn from(inputs: OptsInputs<'a>) -> Self { + let is_linked = turborepo_api_client::is_linked(inputs.api_auth); + let skip_remote = if !is_linked { + true + } else if let Some(enabled) = inputs.config.enabled { + // We're linked, but if the user has explicitly enabled or disabled, use that + // value + !enabled + } else { + false + }; + + // Note that we don't currently use the team_id value here. In the future, we + // should probably verify that we only use the signature value when the + // configured team_id matches the final resolved team_id. + let unused_remote_cache_opts_team_id = + inputs.config.team_id().map(|team_id| team_id.to_string()); + let signature = inputs.config.signature(); + let remote_cache_opts = Some(RemoteCacheOpts::new( + unused_remote_cache_opts_team_id, + signature, + )); + CacheOpts { - override_dir: args.execution_args.cache_dir.clone(), - skip_filesystem: args.execution_args.remote_only, - remote_cache_read_only: args.run_args.remote_cache_read_only, - workers: args.run_args.cache_workers, - ..CacheOpts::default() + override_dir: inputs.execution_args.cache_dir.clone(), + skip_filesystem: inputs.execution_args.remote_only, + remote_cache_read_only: inputs.run_args.remote_cache_read_only, + workers: inputs.run_args.cache_workers, + skip_remote, + remote_cache_opts, } } } diff --git a/crates/turborepo-lib/src/run/builder.rs b/crates/turborepo-lib/src/run/builder.rs index 3bc96d3a5543c..f8aa2d62424ce 100644 --- a/crates/turborepo-lib/src/run/builder.rs +++ b/crates/turborepo-lib/src/run/builder.rs @@ -10,7 +10,7 @@ use tracing::debug; use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPath}; use turborepo_analytics::{start_analytics, AnalyticsHandle, AnalyticsSender}; use turborepo_api_client::{APIAuth, APIClient}; -use turborepo_cache::{AsyncCache, RemoteCacheOpts}; +use turborepo_cache::AsyncCache; use turborepo_env::EnvironmentVariableMap; use turborepo_errors::Spanned; use turborepo_repository::{ @@ -69,33 +69,13 @@ pub struct RunBuilder { impl RunBuilder { pub fn new(base: CommandBase) -> Result { - let api_auth = base.api_auth()?; let api_client = base.api_client()?; - let mut opts: Opts = base.args().try_into()?; + let opts = Opts::new(&base)?; + let api_auth = base.api_auth()?; let config = base.config()?; let allow_missing_package_manager = config.allow_no_package_manager(); - let is_linked = turborepo_api_client::is_linked(&api_auth); - if !is_linked { - opts.cache_opts.skip_remote = true; - } else if let Some(enabled) = config.enabled { - // We're linked, but if the user has explicitly enabled or disabled, use that - // value - opts.cache_opts.skip_remote = !enabled; - } - // Note that we don't currently use the team_id value here. In the future, we - // should probably verify that we only use the signature value when the - // configured team_id matches the final resolved team_id. - let unused_remote_cache_opts_team_id = config.team_id().map(|team_id| team_id.to_string()); - let signature = config.signature(); - opts.cache_opts.remote_cache_opts = Some(RemoteCacheOpts::new( - unused_remote_cache_opts_team_id, - signature, - )); - if opts.run_opts.experimental_space_id.is_none() { - opts.run_opts.experimental_space_id = config.spaces_id().map(|s| s.to_owned()); - } let version = base.version(); let experimental_ui = config.ui(); let processes = ProcessManager::new( @@ -111,11 +91,11 @@ impl RunBuilder { processes, opts, api_client, - api_auth, repo_root, ui, version, experimental_ui, + api_auth, analytics_sender: None, entrypoint_packages: None, should_print_prelude_override: None, diff --git a/turborepo-tests/integration/tests/no-args.t b/turborepo-tests/integration/tests/no-args.t index 8c74321153b0c..dc079a8b5b31e 100644 --- a/turborepo-tests/integration/tests/no-args.t +++ b/turborepo-tests/integration/tests/no-args.t @@ -96,7 +96,7 @@ Make sure exit code is 2 when no args are passed --global-deps Specify glob of global filesystem dependencies to be hashed. Useful for .env and files --env-mode [] - Environment variable mode. Use "loose" to pass the entire existing environment. Use "strict" to use an allowlist specified in turbo.json [default: strict] [possible values: loose, strict] + Environment variable mode. Use "loose" to pass the entire existing environment. Use "strict" to use an allowlist specified in turbo.json [possible values: loose, strict] -F, --filter Use the given selector to specify package(s) to act as entry points. The syntax mirrors pnpm's syntax, and additional documentation and examples can be found in turbo's documentation https://turbo.build/repo/docs/reference/command-line-reference/run#--filter --output-logs diff --git a/turborepo-tests/integration/tests/turbo-help.t b/turborepo-tests/integration/tests/turbo-help.t index 1ec3f0c34c423..bb2bad7144ec1 100644 --- a/turborepo-tests/integration/tests/turbo-help.t +++ b/turborepo-tests/integration/tests/turbo-help.t @@ -96,7 +96,7 @@ Test help flag --global-deps Specify glob of global filesystem dependencies to be hashed. Useful for .env and files --env-mode [] - Environment variable mode. Use "loose" to pass the entire existing environment. Use "strict" to use an allowlist specified in turbo.json [default: strict] [possible values: loose, strict] + Environment variable mode. Use "loose" to pass the entire existing environment. Use "strict" to use an allowlist specified in turbo.json [possible values: loose, strict] -F, --filter Use the given selector to specify package(s) to act as entry points. The syntax mirrors pnpm's syntax, and additional documentation and examples can be found in turbo's documentation https://turbo.build/repo/docs/reference/command-line-reference/run#--filter --output-logs @@ -268,7 +268,6 @@ Test help flag --env-mode [] Environment variable mode. Use "loose" to pass the entire existing environment. Use "strict" to use an allowlist specified in turbo.json - [default: strict] [possible values: loose, strict] -F, --filter