From 74149569db25deae64d5e61fd327b18a336de187 Mon Sep 17 00:00:00 2001
From: Paul Masurel <paul@quickwit.io>
Date: Mon, 23 Oct 2023 16:54:40 +0900
Subject: [PATCH] Fix the NO_COLOR environment variable interpretation. (#4011)

Before this PR, NO_COLOR had to be true or false.
This PR interprets the environment variable as true for any value that
is non empty as described in https://no-color.org/

This fixes the config in our own configuration:
https://github.com/quickwit-oss/quickwit/blob/kzf2f6aacca1cd645a473b7a9ec0707a6efc6e323/docs/log-management/send-logs/using-otel-collector-with-helm.md?plain=1#L75
---
 quickwit/quickwit-cli/src/cli.rs    |  2 ++
 quickwit/quickwit-cli/src/logger.rs |  6 ++---
 quickwit/quickwit-cli/src/main.rs   | 34 +++++++++++++++++++++++++++--
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/quickwit/quickwit-cli/src/cli.rs b/quickwit/quickwit-cli/src/cli.rs
index e915311a3f7..7e7db98683d 100644
--- a/quickwit/quickwit-cli/src/cli.rs
+++ b/quickwit/quickwit-cli/src/cli.rs
@@ -30,6 +30,7 @@ use crate::tool::{build_tool_command, ToolCliCommand};
 pub fn build_cli() -> Command {
     Command::new("Quickwit")
         .arg(
+            // Following https://no-color.org/
             Arg::new("no-color")
                 .long("no-color")
                 .help(
@@ -37,6 +38,7 @@ pub fn build_cli() -> Command {
                      output",
                 )
                 .env("NO_COLOR")
+                .value_parser(clap::builder::FalseyValueParser::new())
                 .global(true)
                 .action(ArgAction::SetTrue),
         )
diff --git a/quickwit/quickwit-cli/src/logger.rs b/quickwit/quickwit-cli/src/logger.rs
index 4d2a4a60e2a..4cd9599d8ed 100644
--- a/quickwit/quickwit-cli/src/logger.rs
+++ b/quickwit/quickwit-cli/src/logger.rs
@@ -37,7 +37,7 @@ use crate::QW_ENABLE_TOKIO_CONSOLE_ENV_KEY;
 
 pub fn setup_logging_and_tracing(
     level: Level,
-    ansi: bool,
+    ansi_colors: bool,
     build_info: &BuildInfo,
 ) -> anyhow::Result<()> {
     #[cfg(feature = "tokio-console")]
@@ -88,7 +88,7 @@ pub fn setup_logging_and_tracing(
             .with(
                 tracing_subscriber::fmt::layer()
                     .event_format(event_format)
-                    .with_ansi(ansi),
+                    .with_ansi(ansi_colors),
             )
             .try_init()
             .context("Failed to set up tracing.")?;
@@ -97,7 +97,7 @@ pub fn setup_logging_and_tracing(
             .with(
                 tracing_subscriber::fmt::layer()
                     .event_format(event_format)
-                    .with_ansi(ansi),
+                    .with_ansi(ansi_colors),
             )
             .try_init()
             .context("Failed to set up tracing.")?;
diff --git a/quickwit/quickwit-cli/src/main.rs b/quickwit/quickwit-cli/src/main.rs
index 91bb31b91bc..5d8ec6676da 100644
--- a/quickwit/quickwit-cli/src/main.rs
+++ b/quickwit/quickwit-cli/src/main.rs
@@ -50,7 +50,7 @@ async fn main_impl() -> anyhow::Result<()> {
 
     let app = build_cli().about(about_text).version(version_text);
     let matches = app.get_matches();
-    let ansi = !matches.get_flag("no-color");
+    let ansi_colors = !matches.get_flag("no-color");
 
     let command = match CliCommand::parse_cli_args(matches) {
         Ok(command) => command,
@@ -63,7 +63,7 @@ async fn main_impl() -> anyhow::Result<()> {
     #[cfg(feature = "jemalloc")]
     start_jemalloc_metrics_loop();
 
-    setup_logging_and_tracing(command.default_log_level(), ansi, build_info)?;
+    setup_logging_and_tracing(command.default_log_level(), ansi_colors, build_info)?;
     let return_code: i32 = if let Err(err) = command.execute().await {
         eprintln!("{} Command failed: {:?}\n", "✘".color(RED_COLOR), err);
         1
@@ -704,4 +704,34 @@ mod tests {
         ));
         Ok(())
     }
+
+    #[test]
+    fn test_parse_no_color() {
+        let previous_no_color_res = std::env::var("NO_COLOR");
+        {
+            std::env::set_var("NO_COLOR", "whatever_interpreted_as_true");
+            let app = build_cli().no_binary_name(true);
+            let matches = app.try_get_matches_from(["run"]).unwrap();
+            let no_color = matches.get_flag("no-color");
+            assert!(no_color);
+        }
+        {
+            // empty string is false.
+            std::env::set_var("NO_COLOR", "");
+            let app = build_cli().no_binary_name(true);
+            let matches = app.try_get_matches_from(["run"]).unwrap();
+            let no_color = matches.get_flag("no-color");
+            assert!(!no_color);
+        }
+        {
+            // empty string is false.
+            let app = build_cli().no_binary_name(true);
+            let matches = app.try_get_matches_from(["run", "--no-color"]).unwrap();
+            let no_color = matches.get_flag("no-color");
+            assert!(no_color);
+        }
+        if let Ok(previous_no_color) = previous_no_color_res {
+            std::env::set_var("NO_COLOR", previous_no_color);
+        }
+    }
 }