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.

Sign up for GitHub

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

Jump to bottom

refactor(turborepo): derive Opts from Config #8759

Merged
merged 2 commits into from
Jul 16, 2024
Merged

refactor(turborepo): derive Opts from Config #8759

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e294a86
Refactored opts to be derived from config, so we have a single, immut…
NicholasLYang Jul 15, 2024
012c161
Update tests
NicholasLYang Jul 16, 2024
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
10 changes: 10 additions & 0 deletions crates/turborepo-api-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,16 @@ pub struct APIAuth {
pub team_slug: Option<String>,
}

impl std::fmt::Debug for APIAuth {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor detail because OptsInputs derives Debug. This is a custom impl because printing the token field seemed like a bad idea

Copy link
Contributor

@dimitropoulos dimitropoulos Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible this caused the CI failure by obscuring the token as --token=***?

turbo run test --filter=turborepo-tests-integration --color --env-mode=strict --token=*** --team=vercel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think so. Lemme see what's causing that

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<APIAuth>) -> bool {
api_auth
.as_ref()
Expand Down
29 changes: 7 additions & 22 deletions crates/turborepo-lib/src/cli/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer assume Strict when the flag is not here. This allows the config override logic to work correctly.

pub env_mode: Option<EnvMode>,
/// 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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
Expand All @@ -1452,28 +1452,13 @@ mod test {
} ;
"env_mode: not fully-specified"
)]
#[test_case::test_case(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test is no longer needed since this behavior is not needed.

&["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())
Expand All @@ -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())
Expand Down
129 changes: 83 additions & 46 deletions crates/turborepo-lib/src/opts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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)]
Expand Down Expand Up @@ -76,20 +80,25 @@ impl Opts {
}
}

impl<'a> TryFrom<&'a Args> for Opts {
type Error = self::Error;
impl Opts {
pub fn new(base: &CommandBase) -> Result<Self, Error> {
let args = base.args();
let config = base.config()?;
let api_auth = base.api_auth()?;

fn try_from(args: &'a Args) -> Result<Self, Self::Error> {
let Some(Command::Run {
run_args,
execution_args,
}) = &args.command
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);
Expand All @@ -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<APIAuth>,
}

#[derive(Debug, Default)]
Expand All @@ -119,12 +129,12 @@ pub struct RunCacheOpts {
pub(crate) task_output_logs_override: Option<OutputLogsMode>,
}

impl<'a> From<RunAndExecutionArgs<'a>> for RunCacheOpts {
fn from(args: RunAndExecutionArgs<'a>) -> Self {
impl<'a> From<OptsInputs<'a>> 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,
}
}
}
Expand Down Expand Up @@ -187,28 +197,28 @@ pub enum ResolvedLogPrefix {

const DEFAULT_CONCURRENCY: u32 = 10;

impl<'a> TryFrom<RunAndExecutionArgs<'a>> for RunOpts {
impl<'a> TryFrom<OptsInputs<'a>> for RunOpts {
type Error = self::Error;

fn try_from(args: RunAndExecutionArgs) -> Result<Self, Self::Error> {
let concurrency = args
fn try_from(inputs: OptsInputs) -> Result<Self, Self::Error> {
let concurrency = inputs
.execution_args
.concurrency
.as_deref()
.map(parse_concurrency)
.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,
},
Expand All @@ -218,33 +228,37 @@ impl<'a> TryFrom<RunAndExecutionArgs<'a>> 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,
})
}
Expand Down Expand Up @@ -288,33 +302,56 @@ pub struct ScopeOpts {
pub filter_patterns: Vec<String>,
}

impl<'a> TryFrom<RunAndExecutionArgs<'a>> for ScopeOpts {
impl<'a> TryFrom<OptsInputs<'a>> for ScopeOpts {
type Error = self::Error;

fn try_from(args: RunAndExecutionArgs<'a>) -> Result<Self, Self::Error> {
let pkg_inference_root = args
fn try_from(inputs: OptsInputs<'a>) -> Result<Self, Self::Error> {
let pkg_inference_root = inputs
.execution_args
.pkg_inference_root
.as_ref()
.map(AnchoredSystemPathBuf::from_raw)
.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<RunAndExecutionArgs<'a>> for CacheOpts {
fn from(args: RunAndExecutionArgs<'a>) -> Self {
impl<'a> From<OptsInputs<'a>> 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,
}
}
}
Expand Down
28 changes: 4 additions & 24 deletions crates/turborepo-lib/src/run/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -69,33 +69,13 @@ pub struct RunBuilder {

impl RunBuilder {
pub fn new(base: CommandBase) -> Result<Self, Error> {
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(
Expand All @@ -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,
Expand Down
Loading