merge: #3271

3271: fix: treat `--no-color`/`--force-color` as CLI flags r=fnichol a=fnichol 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. <img src="https://media4.giphy.com/media/i6IqXuLaTdqRW/giphy.gif"/> Co-authored-by: Fletcher Nichol <[email protected]>
systeminit · Feb 6, 2024 · 04757ab · 04757ab
2 parents dc4de4a + 3c816a6
commit 04757ab
Show file tree

Hide file tree

Showing 13 changed files with 74 additions and 94 deletions.
diff --git a/bin/council/src/args.rs b/bin/council/src/args.rs
@@ -23,23 +23,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -28,23 +28,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -25,23 +25,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -25,23 +25,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -27,23 +27,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -21,23 +21,25 @@ pub(crate) struct Args {
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    pub(crate) no_color: bool,
 
     /// Forces ANSI coloring, even if standard output refers to a terminal/TTY.
     ///
     /// For more details, visit: <http://no-color.org/>.
     #[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<bool>,
+    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
@@ -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
@@ -122,11 +122,11 @@ pub struct TelemetryConfig {
     )]
     secondary_log_span_events_env_var: Option<String>,
 
-    #[builder(setter(into, strip_option), default = "self.default_no_color()")]
-    no_color: bool,
+    #[builder(setter(into), default = "self.default_no_color()")]
+    no_color: Option<bool>,
 
-    #[builder(setter(into, strip_option), default = "false")]
-    force_color: bool,
+    #[builder(setter(into), default = "None")]
+    force_color: Option<bool>,
 
     #[builder(default = "true")]
     signal_handlers: bool,
@@ -202,15 +202,13 @@ impl TelemetryConfigBuilder {
         }
     }
 
-    fn default_no_color(&self) -> bool {
+    fn default_no_color(&self) -> Option<bool> {
         // Checks a known/standard var as a fallback. Code upstack will check for an `SI_*`
         // prefixed version which should have a higher precendence.
         //
         // See: <http://no-color.org/>
         #[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()
     }
 }