From 3c816a6cd4ccbaa596decaa9ef34b3458b415582 Mon Sep 17 00:00:00 2001 From: Fletcher Nichol Date: Mon, 5 Feb 2024 20:14:26 -0700 Subject: [PATCH] fix: treat `--no-color`/`--force-color` as CLI flags This change ensures that the new color CLI flags are flags and *not* options. That means that passing `--no-color` sets the value to `true` rather than an option where `--no-color false` is valid (this was not intended behavior). In an effort to determine whether a CLI flag was set the telemetry config was also updated so that the right config precedence is in effect. Signed-off-by: Fletcher Nichol --- bin/council/src/args.rs | 10 ++++++---- bin/council/src/main.rs | 15 +++++---------- bin/cyclone/src/args.rs | 10 ++++++---- bin/cyclone/src/main.rs | 15 +++++---------- bin/module-index/src/args.rs | 10 ++++++---- bin/module-index/src/main.rs | 15 +++++---------- bin/pinga/src/args.rs | 10 ++++++---- bin/pinga/src/main.rs | 15 +++++---------- bin/sdf/src/args.rs | 10 ++++++---- bin/sdf/src/main.rs | 15 +++++---------- bin/veritech/src/args.rs | 10 ++++++---- bin/veritech/src/main.rs | 15 +++++---------- lib/telemetry-application-rs/src/lib.rs | 18 ++++++++---------- 13 files changed, 74 insertions(+), 94 deletions(-) diff --git a/bin/council/src/args.rs b/bin/council/src/args.rs index 152a2ca5ab..866bec9675 100644 --- a/bin/council/src/args.rs +++ b/bin/council/src/args.rs @@ -23,23 +23,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// NATS connection URL [example: demo.nats.io] #[arg(long)] diff --git a/bin/council/src/main.rs b/bin/council/src/main.rs index 4dc07f0be4..b2f95a826a 100644 --- a/bin/council/src/main.rs +++ b/bin/council/src/main.rs @@ -27,19 +27,14 @@ async fn async_main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("council") .service_namespace("si") .log_env_var_prefix("SI") - .app_modules(vec!["council", "council_server"]); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .app_modules(vec!["council", "council_server"]) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/bin/cyclone/src/args.rs b/bin/cyclone/src/args.rs index e2fa235736..c777079d54 100644 --- a/bin/cyclone/src/args.rs +++ b/bin/cyclone/src/args.rs @@ -28,23 +28,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// Binds service to a socket address [example: 0.0.0.0:5157] #[arg(long, group = "bind")] diff --git a/bin/cyclone/src/main.rs b/bin/cyclone/src/main.rs index f211ff501f..0452053027 100644 --- a/bin/cyclone/src/main.rs +++ b/bin/cyclone/src/main.rs @@ -21,20 +21,15 @@ async fn main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("cyclone") .service_namespace("si") .log_env_var_prefix("SI") .app_modules(vec!["cyclone", "cyclone_server"]) - .custom_default_tracing_level(CUSTOM_DEFAULT_TRACING_LEVEL); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .custom_default_tracing_level(CUSTOM_DEFAULT_TRACING_LEVEL) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/bin/module-index/src/args.rs b/bin/module-index/src/args.rs index c6e4a09c84..caaccbc110 100644 --- a/bin/module-index/src/args.rs +++ b/bin/module-index/src/args.rs @@ -25,23 +25,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// PostgreSQL connection pool dbname [example: myapp] #[arg(long, env)] diff --git a/bin/module-index/src/main.rs b/bin/module-index/src/main.rs index 9b3da91e9d..62c3adc39c 100644 --- a/bin/module-index/src/main.rs +++ b/bin/module-index/src/main.rs @@ -27,19 +27,14 @@ async fn async_main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("module-index") .service_namespace("si") .log_env_var_prefix("SI") - .app_modules(vec!["module_index", "module_index_server"]); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .app_modules(vec!["module_index", "module_index_server"]) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/bin/pinga/src/args.rs b/bin/pinga/src/args.rs index 7b10c348ce..02bb3eb062 100644 --- a/bin/pinga/src/args.rs +++ b/bin/pinga/src/args.rs @@ -25,23 +25,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// PostgreSQL connection pool dbname [example: myapp] #[arg(long)] diff --git a/bin/pinga/src/main.rs b/bin/pinga/src/main.rs index 967a36c752..8beb8980e9 100644 --- a/bin/pinga/src/main.rs +++ b/bin/pinga/src/main.rs @@ -27,19 +27,14 @@ async fn async_main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("pinga") .service_namespace("si") .log_env_var_prefix("SI") - .app_modules(vec!["pinga", "pinga_server"]); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .app_modules(vec!["pinga", "pinga_server"]) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/bin/sdf/src/args.rs b/bin/sdf/src/args.rs index db4b1874de..5e1866e936 100644 --- a/bin/sdf/src/args.rs +++ b/bin/sdf/src/args.rs @@ -27,23 +27,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// PostgreSQL connection pool dbname [example: myapp] #[arg(long)] diff --git a/bin/sdf/src/main.rs b/bin/sdf/src/main.rs index 7e81d46f3c..821ec368b8 100644 --- a/bin/sdf/src/main.rs +++ b/bin/sdf/src/main.rs @@ -38,19 +38,14 @@ async fn async_main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("sdf") .service_namespace("si") .log_env_var_prefix("SI") - .app_modules(vec!["sdf", "sdf_server"]); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .app_modules(vec!["sdf", "sdf_server"]) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/bin/veritech/src/args.rs b/bin/veritech/src/args.rs index 36d0c2415e..73ac173e7c 100644 --- a/bin/veritech/src/args.rs +++ b/bin/veritech/src/args.rs @@ -21,23 +21,25 @@ pub(crate) struct Args { /// /// For more details, visit: . #[arg( - long, + long = "no-color", + default_value = "false", env = "SI_NO_COLOR", hide_env_values = true, conflicts_with = "force_color" )] - pub(crate) no_color: Option, + pub(crate) no_color: bool, /// Forces ANSI coloring, even if standard output refers to a terminal/TTY. /// /// For more details, visit: . #[arg( - long, + long = "force-color", + default_value = "false", env = "SI_FORCE_COLOR", hide_env_values = true, conflicts_with = "no_color" )] - pub(crate) force_color: Option, + pub(crate) force_color: bool, /// NATS connection URL [example: 0.0.0.0:4222] #[arg(long, short = 'u')] diff --git a/bin/veritech/src/main.rs b/bin/veritech/src/main.rs index afd7ebb99f..4ca0fcedb0 100644 --- a/bin/veritech/src/main.rs +++ b/bin/veritech/src/main.rs @@ -13,19 +13,14 @@ async fn main() -> Result<()> { color_eyre::install()?; let args = args::parse(); let (mut telemetry, telemetry_shutdown) = { - let mut builder = TelemetryConfig::builder(); - builder + let config = TelemetryConfig::builder() + .force_color(args.force_color.then_some(true)) + .no_color(args.no_color.then_some(true)) .service_name("veritech") .service_namespace("si") .log_env_var_prefix("SI") - .app_modules(vec!["veritech", "veritech_server"]); - if let Some(force_color) = args.force_color { - builder.force_color(force_color); - } - if let Some(no_color) = args.no_color { - builder.no_color(no_color); - } - let config = builder.build()?; + .app_modules(vec!["veritech", "veritech_server"]) + .build()?; telemetry_application::init(config, &task_tracker, shutdown_token.clone())? }; diff --git a/lib/telemetry-application-rs/src/lib.rs b/lib/telemetry-application-rs/src/lib.rs index 07da44e04d..1fcf26528b 100644 --- a/lib/telemetry-application-rs/src/lib.rs +++ b/lib/telemetry-application-rs/src/lib.rs @@ -122,11 +122,11 @@ pub struct TelemetryConfig { )] secondary_log_span_events_env_var: Option, - #[builder(setter(into, strip_option), default = "self.default_no_color()")] - no_color: bool, + #[builder(setter(into), default = "self.default_no_color()")] + no_color: Option, - #[builder(setter(into, strip_option), default = "false")] - force_color: bool, + #[builder(setter(into), default = "None")] + force_color: Option, #[builder(default = "true")] signal_handlers: bool, @@ -202,15 +202,13 @@ impl TelemetryConfigBuilder { } } - fn default_no_color(&self) -> bool { + fn default_no_color(&self) -> Option { // Checks a known/standard var as a fallback. Code upstack will check for an `SI_*` // prefixed version which should have a higher precendence. // // See: #[allow(clippy::disallowed_methods)] // See rationale in comment above - std::env::var_os("NO_COLOR") - .map(|value| !value.is_empty()) - .unwrap_or(false) + std::env::var_os("NO_COLOR").map(|value| !value.is_empty()) } } @@ -407,14 +405,14 @@ fn create_client( } fn should_add_ansi(config: &TelemetryConfig) -> bool { - if config.force_color { + if config.force_color.filter(|fc| *fc).unwrap_or(false) { // If we're forcing colors, then this is unconditionally true true } else { // Otherwise 2 conditions must be met: // 1. did we *not* ask for `no_color` (or: is `no_color` unset) // 2. is the standard output file descriptor refer to a terminal or TTY - !config.no_color && io::stdout().is_terminal() + !config.no_color.filter(|nc| *nc).unwrap_or(false) && io::stdout().is_terminal() } }