From 2f0a75b50537dfe922becc29104a3c74512fe365 Mon Sep 17 00:00:00 2001 From: Nicholas Yang Date: Wed, 4 Dec 2024 15:36:56 -0500 Subject: [PATCH] refactor(opts): remove config from opts (#9562) ### Description Remove the config field from `Opts` entirely. I'm doing this because the config data already gets partially moved into `Opts`, so by keeping the config around, we have multiple sources for the same data. Now `Opts` is the fully transformed, normalized config for a run. You can review this commit by commit if you like ### Testing Instructions --- crates/turborepo-cache/src/http.rs | 7 +- crates/turborepo-cache/src/lib.rs | 2 +- crates/turborepo-lib/src/cli/mod.rs | 3 +- crates/turborepo-lib/src/commands/config.rs | 11 +- crates/turborepo-lib/src/commands/link.rs | 9 +- crates/turborepo-lib/src/commands/login.rs | 10 +- crates/turborepo-lib/src/commands/mod.rs | 27 ++--- crates/turborepo-lib/src/commands/prune.rs | 2 +- crates/turborepo-lib/src/commands/unlink.rs | 6 +- crates/turborepo-lib/src/diagnostics.rs | 4 +- crates/turborepo-lib/src/opts.rs | 108 ++++++++++++++++++-- crates/turborepo-lib/src/run/builder.rs | 28 ++--- crates/turborepo-lib/src/run/watch.rs | 10 +- 13 files changed, 155 insertions(+), 72 deletions(-) diff --git a/crates/turborepo-cache/src/http.rs b/crates/turborepo-cache/src/http.rs index d8f89973bf568..892f8c344d369 100644 --- a/crates/turborepo-cache/src/http.rs +++ b/crates/turborepo-cache/src/http.rs @@ -337,7 +337,12 @@ mod test { "2.0.0", true, )?; - let opts = CacheOpts::default(); + let opts = CacheOpts { + cache_dir: ".turbo/cache".into(), + cache: Default::default(), + workers: 0, + remote_cache_opts: None, + }; let api_auth = APIAuth { team_id: Some("my-team".to_string()), token: "my-token".to_string(), diff --git a/crates/turborepo-cache/src/lib.rs b/crates/turborepo-cache/src/lib.rs index 23ba603ed6836..b0aa4c607ab0f 100644 --- a/crates/turborepo-cache/src/lib.rs +++ b/crates/turborepo-cache/src/lib.rs @@ -172,7 +172,7 @@ impl Default for CacheActions { } } -#[derive(Clone, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct CacheOpts { pub cache_dir: Utf8PathBuf, pub cache: CacheConfig, diff --git a/crates/turborepo-lib/src/cli/mod.rs b/crates/turborepo-lib/src/cli/mod.rs index e072ee5d99d16..8bf6660c7efb2 100644 --- a/crates/turborepo-lib/src/cli/mod.rs +++ b/crates/turborepo-lib/src/cli/mod.rs @@ -1283,8 +1283,7 @@ pub async fn run( } } Command::Config => { - let base = CommandBase::new(cli_args.clone(), repo_root, version, color_config)?; - config::run(base).await?; + config::run(repo_root, cli_args).await?; Ok(0) } Command::Ls { diff --git a/crates/turborepo-lib/src/commands/config.rs b/crates/turborepo-lib/src/commands/config.rs index e67a3bc0a4c30..b0c2873ff4b50 100644 --- a/crates/turborepo-lib/src/commands/config.rs +++ b/crates/turborepo-lib/src/commands/config.rs @@ -1,8 +1,9 @@ use camino::Utf8Path; use serde::Serialize; +use turbopath::AbsoluteSystemPathBuf; use turborepo_repository::{package_graph::PackageGraph, package_json::PackageJson}; -use crate::{cli, cli::EnvMode, commands::CommandBase, turbo_json::UIMode}; +use crate::{cli, cli::EnvMode, commands::CommandBase, turbo_json::UIMode, Args}; #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] @@ -26,11 +27,11 @@ struct ConfigOutput<'a> { cache_dir: &'a Utf8Path, } -pub async fn run(base: CommandBase) -> Result<(), cli::Error> { - let config = base.config(); - let root_package_json = PackageJson::load(&base.repo_root.join_component("package.json"))?; +pub async fn run(repo_root: AbsoluteSystemPathBuf, args: Args) -> Result<(), cli::Error> { + let config = CommandBase::load_config(&repo_root, &args)?; + let root_package_json = PackageJson::load(&repo_root.join_component("package.json"))?; - let package_graph = PackageGraph::builder(&base.repo_root, root_package_json) + let package_graph = PackageGraph::builder(&repo_root, root_package_json) .build() .await?; diff --git a/crates/turborepo-lib/src/commands/link.rs b/crates/turborepo-lib/src/commands/link.rs index de55c5738baa2..a90f643e431de 100644 --- a/crates/turborepo-lib/src/commands/link.rs +++ b/crates/turborepo-lib/src/commands/link.rs @@ -174,8 +174,9 @@ pub async fn link( let api_client = base.api_client()?; let token = base .opts() - .config - .token() + .api_client_opts + .token + .as_deref() .ok_or_else(|| Error::TokenNotFound { command: base.color_config.apply(BOLD.apply_to("`npx turbo login`")), })?; @@ -639,7 +640,7 @@ mod test { .build()?; let mut base = CommandBase::from_opts( - Opts::new(&Args::default(), config)?, + Opts::new(&repo_root, &Args::default(), config)?, repo_root.clone(), "1.0.0", ColorConfig::new(false), @@ -701,7 +702,7 @@ mod test { .build()?; let mut base = CommandBase::from_opts( - Opts::new(&Args::default(), config)?, + Opts::new(&repo_root, &Args::default(), config)?, repo_root.clone(), "", ColorConfig::new(false), diff --git a/crates/turborepo-lib/src/commands/login.rs b/crates/turborepo-lib/src/commands/login.rs index 98a787b19812e..7ffa54e81185f 100644 --- a/crates/turborepo-lib/src/commands/login.rs +++ b/crates/turborepo-lib/src/commands/login.rs @@ -15,9 +15,9 @@ pub async fn sso_login( telemetry.track_login_method(LoginMethod::SSO); let api_client: APIClient = base.api_client()?; let color_config = base.color_config; - let login_url_config = base.config().login_url().to_string(); + let login_url_config = base.opts.api_client_opts.login_url.to_string(); let options = LoginOptions { - existing_token: base.config().token(), + existing_token: base.opts.api_client_opts.token.as_deref(), sso_team: Some(sso_team), force, ..LoginOptions::new( @@ -72,9 +72,11 @@ pub async fn login( let api_client: APIClient = base.api_client()?; let color_config = base.color_config; - let login_url_config = base.config().login_url().to_string(); + let login_url_config = base.opts.api_client_opts.login_url.to_string(); + let existing_token = base.opts.api_client_opts.token.as_deref(); + let options = LoginOptions { - existing_token: base.config().token(), + existing_token, force, ..LoginOptions::new( &color_config, diff --git a/crates/turborepo-lib/src/commands/mod.rs b/crates/turborepo-lib/src/commands/mod.rs index 8c2e343976e05..eebe1bfdd55c5 100644 --- a/crates/turborepo-lib/src/commands/mod.rs +++ b/crates/turborepo-lib/src/commands/mod.rs @@ -44,8 +44,8 @@ impl CommandBase { version: &'static str, color_config: ColorConfig, ) -> Result { - let config = Self::config_init(&repo_root, &args)?; - let opts = Opts::new(&args, config)?; + let config = Self::load_config(&repo_root, &args)?; + let opts = Opts::new(&repo_root, &args, config)?; Ok(Self { repo_root, @@ -69,7 +69,7 @@ impl CommandBase { } } - fn config_init( + pub fn load_config( repo_root: &AbsoluteSystemPath, args: &Args, ) -> Result { @@ -143,11 +143,10 @@ impl CommandBase { } pub fn api_auth(&self) -> Result, ConfigError> { - let config = self.config(); - let team_id = config.team_id(); - let team_slug = config.team_slug(); + let team_id = self.opts.api_client_opts.team_id.as_ref(); + let team_slug = self.opts.api_client_opts.team_slug.as_ref(); - let Some(token) = config.token() else { + let Some(token) = &self.opts.api_client_opts.token else { return Ok(None); }; @@ -158,18 +157,12 @@ impl CommandBase { })) } - pub fn config(&self) -> &ConfigurationOptions { - &self.opts.config - } - pub fn api_client(&self) -> Result { - let config = &self.opts.config; - let api_url = config.api_url(); - let timeout = config.timeout(); - let upload_timeout = config.upload_timeout(); + let timeout = self.opts.api_client_opts.timeout; + let upload_timeout = self.opts.api_client_opts.upload_timeout; APIClient::new( - api_url, + &self.opts.api_client_opts.api_url, if timeout > 0 { Some(Duration::from_secs(timeout)) } else { @@ -181,7 +174,7 @@ impl CommandBase { None }, self.version, - config.preflight(), + self.opts.api_client_opts.preflight, ) .map_err(ConfigError::ApiClient) } diff --git a/crates/turborepo-lib/src/commands/prune.rs b/crates/turborepo-lib/src/commands/prune.rs index f4101d96d8404..9a6d18230e041 100644 --- a/crates/turborepo-lib/src/commands/prune.rs +++ b/crates/turborepo-lib/src/commands/prune.rs @@ -263,7 +263,7 @@ impl<'a> Prune<'a> { output_dir: &str, telemetry: CommandEventBuilder, ) -> Result { - let allow_missing_package_manager = base.config().allow_no_package_manager(); + let allow_missing_package_manager = base.opts().repo_opts.allow_no_package_manager; telemetry.track_arg_usage( "dangerously-allow-missing-package-manager", allow_missing_package_manager, diff --git a/crates/turborepo-lib/src/commands/unlink.rs b/crates/turborepo-lib/src/commands/unlink.rs index ef25dab0f5410..c532103bf3a03 100644 --- a/crates/turborepo-lib/src/commands/unlink.rs +++ b/crates/turborepo-lib/src/commands/unlink.rs @@ -16,7 +16,8 @@ enum UnlinkSpacesResult { } fn unlink_remote_caching(base: &mut CommandBase) -> Result<(), cli::Error> { - let needs_disabling = base.config().team_id().is_some() || base.config().team_slug().is_some(); + let needs_disabling = base.opts.api_client_opts.team_id.is_some() + || base.opts.api_client_opts.team_slug.is_some(); let output = if needs_disabling { let local_config_path = base.local_config_path(); @@ -56,7 +57,8 @@ fn unlink_remote_caching(base: &mut CommandBase) -> Result<(), cli::Error> { } fn unlink_spaces(base: &mut CommandBase) -> Result<(), cli::Error> { - let needs_disabling = base.config().team_id().is_some() || base.config().team_slug().is_some(); + let needs_disabling = base.opts().api_client_opts.team_id.is_some() + || base.opts().api_client_opts.team_slug.is_some(); if needs_disabling { let local_config_path = base.local_config_path(); diff --git a/crates/turborepo-lib/src/diagnostics.rs b/crates/turborepo-lib/src/diagnostics.rs index 896f88908948f..ed2780d4c3465 100644 --- a/crates/turborepo-lib/src/diagnostics.rs +++ b/crates/turborepo-lib/src/diagnostics.rs @@ -406,8 +406,8 @@ impl Diagnostic for RemoteCacheDiagnostic { let (has_team_id, has_team_slug) = { let base = base.lock().await; ( - base.config().team_id().is_some(), - base.config().team_slug().is_some(), + base.opts().api_client_opts.team_id.is_some(), + base.opts().api_client_opts.team_slug.is_some(), ) }; diff --git a/crates/turborepo-lib/src/opts.rs b/crates/turborepo-lib/src/opts.rs index 1f234b827b5ec..70ad5dda35acf 100644 --- a/crates/turborepo-lib/src/opts.rs +++ b/crates/turborepo-lib/src/opts.rs @@ -2,7 +2,7 @@ use std::backtrace; use camino::Utf8PathBuf; use thiserror::Error; -use turbopath::AnchoredSystemPathBuf; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, AnchoredSystemPathBuf}; use turborepo_api_client::APIAuth; use turborepo_cache::{CacheOpts, RemoteCacheOpts}; @@ -43,11 +43,32 @@ pub enum Error { Config(#[from] crate::config::Error), } +#[derive(Debug, Clone)] +pub struct APIClientOpts { + pub api_url: String, + pub timeout: u64, + pub upload_timeout: u64, + pub token: Option, + pub team_id: Option, + pub team_slug: Option, + pub login_url: String, + pub preflight: bool, +} + +#[derive(Debug, Clone)] +pub struct RepoOpts { + pub root_turbo_json_path: AbsoluteSystemPathBuf, + pub allow_no_package_manager: bool, + pub allow_no_turbo_json: bool, +} + /// The fully resolved options for Turborepo. This is the combination of config, -/// including all the layers (env, args, etc.), and the command line arguments. +/// including all the layers (env, args, defaults, etc.), and the command line +/// arguments. #[derive(Debug, Clone)] pub struct Opts { - pub config: ConfigurationOptions, + pub repo_opts: RepoOpts, + pub api_client_opts: APIClientOpts, pub cache_opts: CacheOpts, pub run_opts: RunOpts, pub runcache_opts: RunCacheOpts, @@ -95,7 +116,11 @@ impl Opts { } impl Opts { - pub fn new(args: &Args, config: ConfigurationOptions) -> Result { + pub fn new( + repo_root: &AbsoluteSystemPath, + args: &Args, + config: ConfigurationOptions, + ) -> Result { let team_id = config.team_id(); let team_slug = config.team_slug(); @@ -125,6 +150,7 @@ impl Opts { }; let inputs = OptsInputs { + repo_root, run_args: run_args.as_ref(), execution_args: execution_args.as_ref(), config: &config, @@ -134,19 +160,23 @@ impl Opts { let cache_opts = CacheOpts::try_from(inputs)?; let scope_opts = ScopeOpts::try_from(inputs)?; let runcache_opts = RunCacheOpts::from(inputs); + let api_client_opts = APIClientOpts::from(inputs); + let repo_opts = RepoOpts::from(inputs); Ok(Self { - config, + repo_opts, run_opts, cache_opts, scope_opts, runcache_opts, + api_client_opts, }) } } #[derive(Debug, Clone, Copy)] struct OptsInputs<'a> { + repo_root: &'a AbsoluteSystemPath, run_args: &'a RunArgs, execution_args: &'a ExecutionArgs, config: &'a ConfigurationOptions, @@ -241,6 +271,20 @@ pub enum ResolvedLogPrefix { None, } +impl<'a> From> for RepoOpts { + fn from(inputs: OptsInputs<'a>) -> Self { + let root_turbo_json_path = inputs.config.root_turbo_json_path(inputs.repo_root); + let allow_no_package_manager = inputs.config.allow_no_package_manager(); + let allow_no_turbo_json = inputs.config.allow_no_turbo_json(); + + RepoOpts { + root_turbo_json_path, + allow_no_package_manager, + allow_no_turbo_json, + } + } +} + const DEFAULT_CONCURRENCY: u32 = 10; impl<'a> TryFrom> for RunOpts { @@ -380,6 +424,30 @@ impl<'a> TryFrom> for ScopeOpts { } } +impl<'a> From> for APIClientOpts { + fn from(inputs: OptsInputs<'a>) -> Self { + let api_url = inputs.config.api_url().to_string(); + let timeout = inputs.config.timeout(); + let upload_timeout = inputs.config.upload_timeout(); + let preflight = inputs.config.preflight(); + let token = inputs.config.token().map(|s| s.to_string()); + let team_id = inputs.config.team_id().map(|s| s.to_string()); + let team_slug = inputs.config.team_slug().map(|s| s.to_string()); + let login_url = inputs.config.login_url().to_string(); + + APIClientOpts { + api_url, + timeout, + upload_timeout, + token, + team_id, + team_slug, + login_url, + preflight, + } + } +} + impl<'a> TryFrom> for CacheOpts { type Error = self::Error; @@ -466,7 +534,7 @@ mod test { use turborepo_cache::CacheOpts; use turborepo_ui::ColorConfig; - use super::RunOpts; + use super::{APIClientOpts, RepoOpts, RunOpts}; use crate::{ cli::{Command, DryRunMode, RunArgs}, commands::CommandBase, @@ -588,7 +656,12 @@ mod test { is_github_actions: false, daemon: None, }; - let cache_opts = CacheOpts::default(); + let cache_opts = CacheOpts { + cache_dir: ".turbo/cache".into(), + cache: Default::default(), + workers: 0, + remote_cache_opts: None, + }; let runcache_opts = RunCacheOpts::default(); let scope_opts = ScopeOpts { pkg_inference_root: None, @@ -598,11 +671,28 @@ mod test { .affected .map(|(base, head)| (Some(base), Some(head))), }; + let config = ConfigurationOptions::default(); + let root_turbo_json_path = config.root_turbo_json_path(&AbsoluteSystemPathBuf::default()); + let opts = Opts { - config: ConfigurationOptions::default(), + repo_opts: RepoOpts { + root_turbo_json_path, + allow_no_package_manager: false, + allow_no_turbo_json: false, + }, + api_client_opts: APIClientOpts { + api_url: "".to_string(), + timeout: 0, + upload_timeout: 0, + token: None, + team_id: None, + team_slug: None, + login_url: "".to_string(), + preflight: false, + }, + scope_opts, run_opts, cache_opts, - scope_opts, runcache_opts, }; let synthesized = opts.synthesize_command(); diff --git a/crates/turborepo-lib/src/run/builder.rs b/crates/turborepo-lib/src/run/builder.rs index be251d6345bd6..50dd72810fb87 100644 --- a/crates/turborepo-lib/src/run/builder.rs +++ b/crates/turborepo-lib/src/run/builder.rs @@ -56,7 +56,6 @@ pub struct RunBuilder { opts: Opts, api_auth: Option, repo_root: AbsoluteSystemPathBuf, - root_turbo_json_path: AbsoluteSystemPathBuf, color_config: ColorConfig, version: &'static str, api_client: APIClient, @@ -66,8 +65,6 @@ pub struct RunBuilder { // this package. entrypoint_packages: Option>, should_print_prelude_override: Option, - allow_missing_package_manager: bool, - allow_no_turbo_json: bool, // In query, we don't want to validate the engine. Defaults to `true` should_validate_engine: bool, // If true, we will add all tasks to the graph, even if they are not specified @@ -80,8 +77,6 @@ impl RunBuilder { let opts = base.opts(); let api_auth = base.api_auth()?; - let config = base.config(); - let allow_missing_package_manager = config.allow_no_package_manager(); let version = base.version(); let processes = ProcessManager::new( @@ -91,8 +86,6 @@ impl RunBuilder { // - if we're on windows, we're using the UI (!cfg!(windows) || matches!(opts.run_opts.ui_mode, UIMode::Tui)), ); - let root_turbo_json_path = config.root_turbo_json_path(&base.repo_root); - let allow_no_turbo_json = config.allow_no_turbo_json(); let CommandBase { repo_root, @@ -112,9 +105,6 @@ impl RunBuilder { analytics_sender: None, entrypoint_packages: None, should_print_prelude_override: None, - allow_missing_package_manager, - root_turbo_json_path, - allow_no_turbo_json, should_validate_engine: true, add_all_tasks: false, }) @@ -238,7 +228,7 @@ impl RunBuilder { run_telemetry.track_is_linked(is_linked); run_telemetry.track_arg_usage( "dangerously_allow_missing_package_manager", - self.allow_missing_package_manager, + self.opts.repo_opts.allow_no_package_manager, ); // we only track the remote cache if we're linked because this defaults to // Vercel @@ -299,11 +289,11 @@ impl RunBuilder { let mut pkg_dep_graph = { let builder = PackageGraph::builder(&self.repo_root, root_package_json.clone()) .with_single_package_mode(self.opts.run_opts.single_package) - .with_allow_no_package_manager(self.allow_missing_package_manager); + .with_allow_no_package_manager(self.opts.repo_opts.allow_no_package_manager); // Daemon package discovery depends on packageManager existing in package.json let graph = if cfg!(feature = "daemon-package-discovery") - && !self.allow_missing_package_manager + && !self.opts.repo_opts.allow_no_package_manager { match (&daemon, self.opts.run_opts.daemon) { (None, Some(true)) => { @@ -391,18 +381,18 @@ impl RunBuilder { let mut turbo_json_loader = if task_access.is_enabled() { TurboJsonLoader::task_access( self.repo_root.clone(), - self.root_turbo_json_path.clone(), + self.opts.repo_opts.root_turbo_json_path.clone(), root_package_json.clone(), ) } else if is_single_package { TurboJsonLoader::single_package( self.repo_root.clone(), - self.root_turbo_json_path.clone(), + self.opts.repo_opts.root_turbo_json_path.clone(), root_package_json.clone(), ) - } else if !self.root_turbo_json_path.exists() && + } else if !self.opts.repo_opts.root_turbo_json_path.exists() && // Infer a turbo.json if allowing no turbo.json is explicitly allowed or if MFE configs are discovered - (self.allow_no_turbo_json || micro_frontend_configs.is_some()) + (self.opts.repo_opts.allow_no_turbo_json || micro_frontend_configs.is_some()) { TurboJsonLoader::workspace_no_turbo_json( self.repo_root.clone(), @@ -412,14 +402,14 @@ impl RunBuilder { } else if let Some(micro_frontends) = µ_frontend_configs { TurboJsonLoader::workspace_with_microfrontends( self.repo_root.clone(), - self.root_turbo_json_path.clone(), + self.opts.repo_opts.root_turbo_json_path.clone(), pkg_dep_graph.packages(), micro_frontends.clone(), ) } else { TurboJsonLoader::workspace( self.repo_root.clone(), - self.root_turbo_json_path.clone(), + self.opts.repo_opts.root_turbo_json_path.clone(), pkg_dep_graph.packages(), ) }; diff --git a/crates/turborepo-lib/src/run/watch.rs b/crates/turborepo-lib/src/run/watch.rs index 3dd0b78563c12..2220a6caa417e 100644 --- a/crates/turborepo-lib/src/run/watch.rs +++ b/crates/turborepo-lib/src/run/watch.rs @@ -112,14 +112,14 @@ impl WatchClient { pub async fn new(base: CommandBase, telemetry: CommandEventBuilder) -> Result { let signal = commands::run::get_signal()?; let handler = SignalHandler::new(signal); - let config = &base.opts.config; - let root_turbo_json_path = config.root_turbo_json_path(&base.repo_root); - if root_turbo_json_path != base.repo_root.join_component(CONFIG_FILE) { + + if base.opts.repo_opts.root_turbo_json_path != base.repo_root.join_component(CONFIG_FILE) { return Err(Error::NonStandardTurboJsonPath( - root_turbo_json_path.to_string(), + base.opts.repo_opts.root_turbo_json_path.to_string(), )); } - if matches!(config.daemon(), Some(false)) { + + if matches!(base.opts.run_opts.daemon, Some(false)) { warn!("daemon is required for watch, ignoring request to disable daemon"); }