fix(cli): only overwrite terminal writer if using Ink (#5688)

<!--  Thanks for sending a pull request! Here are some tips for you:

1. If this is your first pull request, please read our contributor
guidelines in the
https://github.com/garden-io/garden/blob/main/CONTRIBUTING.md file.
2. Please label this pull request according to what type of issue you
are addressing (see "What type of PR is this?" below)
3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add `WIP:` at the beginning of the title or
use the GitHub Draft PR feature.
5. Please add at least two reviewers to the PR. Currently active
maintainers are: @edvald, @thsig, @eysi09, @shumailxyz, @stefreak,
@TimBeyer, @mkhq, and @vvagaytsev.
-->

**What this PR does / why we need it**:

Before this fix, we'd reset the terminal writer type after initialising
the command at the CLI level.

That's basically fine in and off itself except that it broke the test
logger and overwrote the settings we use there.

Instead of adding even more config options for the log level I opted for
simplifying the flow and only overwriting the type if using the 'Ink'
terminal write, since that's only use case for it anyway.

That implicitly fixes the issue with the test logger and slightly
simplifies the overall flow.

**Which issue(s) this PR fixes**:

Fixes #

**Special notes for your reviewer**:

core/src/cli/cli.ts

@@ -222,20 +222,16 @@ ${renderCommands(commands)}
     workingDir: string
     log: Log
   }) {
-    const {
-      "env": environmentName,
-      silent,
-      output,
-      "logger-type": loggerTypeOpt,
-      "force-refresh": forceRefresh,
-      "var": cliVars,
-    } = parsedOpts
+    const { "env": environmentName, silent, output, "force-refresh": forceRefresh, "var": cliVars } = parsedOpts
 
     const parsedCliVars = parseCliVarFlags(cliVars)
     // Some commands may set their own logger type so we update the logger config here,
     // once we've resolved the command.
-    const commandLoggerType = command.getTerminalWriterType({ opts: parsedOpts, args: parsedArgs })
-    getRootLogger().setTerminalWriter(getTerminalWriterType({ silent, output, loggerTypeOpt, commandLoggerType }))
+
+    // For commands that use Ink we overwrite the terminal writer configuration (unless silent/output flags are set)
+    if (command.useInkTerminalWriter({ opts: parsedOpts, args: parsedArgs })) {
+      getRootLogger().setTerminalWriter(getTerminalWriterType({ silent, output, loggerType: "ink" }))
+    }
 
     const globalConfigStore = new GlobalConfigStore()
 
@@ -474,15 +470,15 @@ ${renderCommands(commands)}
       silent,
       output,
       "show-timestamps": showTimestamps,
-      "logger-type": loggerTypeOpt,
+      "logger-type": loggerType,
       "log-level": logLevelStr,
     } = argv
     let logger: RootLogger
     try {
       logger = RootLogger.initialize({
         level: parseLogLevel(logLevelStr),
         storeEntries: false,
-        displayWriterType: getTerminalWriterType({ silent, output, loggerTypeOpt, commandLoggerType: null }),
+        displayWriterType: getTerminalWriterType({ silent, output, loggerType }),
         useEmoji: emoji,
         showTimestamps,
         force: this.initLogger,

core/src/commands/base.ts

@@ -16,7 +16,7 @@ import type { GardenError } from "../exceptions.js"
 import { RuntimeError, InternalError, toGardenError } from "../exceptions.js"
 import type { Garden } from "../garden.js"
 import type { Log } from "../logger/log-entry.js"
-import type { LoggerType, LoggerBase, LoggerConfigBase, LogLevel } from "../logger/logger.js"
+import type { LoggerBase, LoggerConfigBase, LogLevel } from "../logger/logger.js"
 import { eventLogLevel } from "../logger/logger.js"
 import { printEmoji, printFooter } from "../logger/util.js"
 import { getCloudDistributionName, getCloudLogSectionName } from "../util/cloud.js"
@@ -472,8 +472,8 @@ export abstract class Command<
     }
   }
 
-  getTerminalWriterType(_: CommandParamsBase<A, O>): LoggerType {
-    return "default"
+  useInkTerminalWriter(_: CommandParamsBase<A, O>): boolean {
+    return false
   }
 
   describe() {

core/src/commands/deploy.ts

@@ -24,7 +24,6 @@ import { PluginEventBroker } from "../plugin-context.js"
 import { HandlerMonitor } from "../monitors/handler.js"
 import { PortForwardMonitor } from "../monitors/port-forward.js"
 import { LogMonitor } from "../monitors/logs.js"
-import type { LoggerType } from "../logger/logger.js"
 import { parseLogLevel } from "../logger/logger.js"
 import { serveOpts } from "./serve.js"
 import { gardenEnv } from "../constants.js"
@@ -161,8 +160,8 @@ export class DeployCommand extends Command<Args, Opts> {
     printHeader(log, "Deploy", "🚀")
   }
 
-  override getTerminalWriterType(params): LoggerType {
-    return this.maybePersistent(params) ? "ink" : "default"
+  override useInkTerminalWriter(params) {
+    return this.maybePersistent(params) ? true : false
   }
 
   override terminate() {

core/src/commands/dev.tsx

@@ -74,8 +74,8 @@ Use ${styles.bold("up/down")} arrow keys to scroll through your command history.
     )
   }
 
-  override getTerminalWriterType(): LoggerType {
-    return "ink"
+  override useInkTerminalWriter() {
+    return true
   }
 
   override allowInDevCommand() {

core/src/commands/up.ts

@@ -11,7 +11,6 @@ import { Command } from "./base.js"
 import { dedent } from "../util/string.js"
 import type { deployArgs, deployOpts } from "./deploy.js"
 import type { serveOpts } from "./serve.js"
-import type { LoggerType } from "../logger/logger.js"
 import { styles } from "../logger/styles.js"
 
 type UpArgs = typeof deployArgs
@@ -30,8 +29,8 @@ export class UpCommand extends Command<UpArgs, UpOpts> {
     )}, but you can add any arguments and flags supported by the ${styles.highlight("deploy")} command as well.
   `
 
-  override getTerminalWriterType(): LoggerType {
-    return "ink"
+  override useInkTerminalWriter() {
+    return true
   }
 
   async action(params: CommandParams<UpArgs, UpOpts>): Promise<CommandResult> {

core/src/logger/logger.ts

@@ -77,24 +77,21 @@ export function logLevelToString(level: LogLevel): StringLogLevel {
 export const eventLogLevel = LogLevel.debug
 
 /**
- * Return the logger type, depending on what command line args have been set
- * and whether the command specifies a logger type.
+ * Return the logger type, depending on what command line args are used.
  */
 export function getTerminalWriterType({
   silent,
   output,
-  loggerTypeOpt,
-  commandLoggerType,
+  loggerType,
 }: {
   silent: boolean
   output: boolean
-  loggerTypeOpt: LoggerType | null
-  commandLoggerType: LoggerType | null
+  loggerType: LoggerType | null
 }) {
   if (silent || output) {
     return "quiet"
   }
-  return loggerTypeOpt || commandLoggerType || "default"
+  return loggerType || "default"
 }
 
 export function getTerminalWriterInstance(loggerType: LoggerType, level: LogLevel): Writer {

core/test/unit/src/cli/cli.ts

@@ -39,6 +39,40 @@ import { TestGardenCli } from "../../../helpers/cli.js"
 import { NotImplementedError } from "../../../../src/exceptions.js"
 import dedent from "dedent"
 
+/**
+ * Helper functions for removing/resetting the global logger config which is set when
+ * the test runner is initialized.
+ *
+ * By default the logger is set to `quiet` during test runs to hide log output but this
+ * can be used to explicitly test the logger config without the test config getting in the way.
+ *
+ * The `removeGlobalLoggerConfig` function should be used in a `before` hook and the
+ * `resetGlobalLoggerConfig` function should be used in the corresponding `after` hook.
+ */
+function getLoggerConfigSetters() {
+  const envLoggerType = process.env.GARDEN_LOGGER_TYPE
+  const envLogLevel = process.env.GARDEN_LOG_LEVEL
+
+  const removeGlobalLoggerConfig = () => {
+    delete process.env.GARDEN_LOGGER_TYPE
+    delete process.env.GARDEN_LOG_LEVEL
+    gardenEnv.GARDEN_LOGGER_TYPE = ""
+    gardenEnv.GARDEN_LOG_LEVEL = ""
+    RootLogger.clearInstance()
+  }
+
+  const resetGlobalLoggerConfig = () => {
+    process.env.GARDEN_LOGGER_TYPE = envLoggerType
+    process.env.GARDEN_LOG_LEVEL = envLogLevel
+    gardenEnv.GARDEN_LOGGER_TYPE = envLoggerType || ""
+    gardenEnv.GARDEN_LOG_LEVEL = envLogLevel || ""
+    RootLogger.clearInstance()
+    initTestLogger()
+  }
+
+  return { removeGlobalLoggerConfig, resetGlobalLoggerConfig }
+}
+
 describe("cli", () => {
   let cli: GardenCli
   const globalConfigStore = new GlobalConfigStore()
@@ -311,26 +345,16 @@ describe("cli", () => {
     })
 
     context("test logger initialization", () => {
-      const envLoggerType = process.env.GARDEN_LOGGER_TYPE
-      const envLogLevel = process.env.GARDEN_LOG_LEVEL
+      const { removeGlobalLoggerConfig, resetGlobalLoggerConfig } = getLoggerConfigSetters()
 
       // Logger is a singleton and we need to reset it between these tests as we're testing
       // that it's initialised correctly in this block.
       beforeEach(() => {
-        delete process.env.GARDEN_LOGGER_TYPE
-        delete process.env.GARDEN_LOG_LEVEL
-        gardenEnv.GARDEN_LOGGER_TYPE = ""
-        gardenEnv.GARDEN_LOG_LEVEL = ""
-        RootLogger.clearInstance()
+        removeGlobalLoggerConfig()
       })
       // Re-initialise the test logger
       after(() => {
-        process.env.GARDEN_LOGGER_TYPE = envLoggerType
-        process.env.GARDEN_LOG_LEVEL = envLogLevel
-        gardenEnv.GARDEN_LOGGER_TYPE = envLoggerType || ""
-        gardenEnv.GARDEN_LOG_LEVEL = envLogLevel || ""
-        RootLogger.clearInstance()
-        initTestLogger()
+        resetGlobalLoggerConfig()
       })
 
       it("uses the 'TerminalWriter' by default", async () => {
@@ -351,7 +375,7 @@ describe("cli", () => {
 
         await cli.run({ args: ["test-command"], exitOnError: false })
 
-        const logger = log.root
+        const logger = getRootLogger()
         const writers = logger.getWriters()
         expect(writers.display.type).to.equal("default")
       })
@@ -1038,11 +1062,14 @@ describe("cli", () => {
 
     describe("Command error handling", async () => {
       let hook: ReturnType<typeof captureStream>
+      const { removeGlobalLoggerConfig, resetGlobalLoggerConfig } = getLoggerConfigSetters()
 
       beforeEach(() => {
+        removeGlobalLoggerConfig()
         hook = captureStream(process.stdout)
       })
       afterEach(() => {
+        resetGlobalLoggerConfig()
         hook.unhook()
       })
       it("handles GardenError on the command level correctly", async () => {