Fix inconsistencies in log flags and env var

- Fix priority to match the commented behavior.
- Ignore bogus LOG_LEVEL values.

Author: code-asher

src/node/cli.ts

@@ -213,7 +213,7 @@ export const parse = (argv: string[]): Args => {
           ;(args[key] as OptionalString) = new OptionalString(value)
           break
         default: {
-          if (!Object.values(option.type).find((v) => v === value)) {
+          if (!Object.values(option.type).includes(value)) {
             throw new Error(`--${key} valid values: [${Object.values(option.type).join(", ")}]`)
           }
           ;(args[key] as string) = value
@@ -230,20 +230,26 @@ export const parse = (argv: string[]): Args => {
 
   logger.debug("parsed command line", field("args", args))
 
-  // Ensure the environment variable and the flag are synced up. The flag takes
-  // priority over the environment variable.
-  if (args.log === LogLevel.Trace || process.env.LOG_LEVEL === LogLevel.Trace || args.verbose) {
-    args.log = process.env.LOG_LEVEL = LogLevel.Trace
-    args.verbose = true
-  } else if (!args.log && process.env.LOG_LEVEL) {
+  // --verbose takes priority over --log and --log takes priority over the
+  // environment variable.
+  if (args.verbose) {
+    args.log = LogLevel.Trace
+  } else if (
+    !args.log &&
+    process.env.LOG_LEVEL &&
+    Object.values(LogLevel).includes(process.env.LOG_LEVEL as LogLevel)
+  ) {
     args.log = process.env.LOG_LEVEL as LogLevel
-  } else if (args.log) {
-    process.env.LOG_LEVEL = args.log
   }
 
+  // Sync --log, --verbose, the environment variable, and logger level.
+  if (args.log) {
+    process.env.LOG_LEVEL = args.log
+  }
   switch (args.log) {
     case LogLevel.Trace:
       logger.level = Level.Trace
+      args.verbose = true
       break
     case LogLevel.Debug:
       logger.level = Level.Debug

test/cli.test.ts

@@ -1,3 +1,4 @@
+import { logger, Level } from "@coder/logger"
 import * as assert from "assert"
 import * as path from "path"
 import { parse } from "../src/node/cli"
@@ -8,12 +9,15 @@ describe("cli", () => {
     delete process.env.LOG_LEVEL
   })
 
+  // The parser will always fill these out.
+  const defaults = {
+    _: [],
+    "extensions-dir": path.join(xdgLocalDir, "extensions"),
+    "user-data-dir": xdgLocalDir,
+  }
+
   it("should set defaults", () => {
-    assert.deepEqual(parse([]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
-    })
+    assert.deepEqual(parse([]), defaults)
   })
 
   it("should parse all available options", () => {
@@ -82,36 +86,64 @@ describe("cli", () => {
 
   it("should work with short options", () => {
     assert.deepEqual(parse(["-vvv", "-v"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       log: "trace",
       verbose: true,
       version: true,
     })
     assert.equal(process.env.LOG_LEVEL, "trace")
+    assert.equal(logger.level, Level.Trace)
   })
 
   it("should use log level env var", () => {
     process.env.LOG_LEVEL = "debug"
     assert.deepEqual(parse([]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       log: "debug",
     })
     assert.equal(process.env.LOG_LEVEL, "debug")
+    assert.equal(logger.level, Level.Debug)
+
+    process.env.LOG_LEVEL = "trace"
+    assert.deepEqual(parse([]), {
+      ...defaults,
+      log: "trace",
+      verbose: true,
+    })
+    assert.equal(process.env.LOG_LEVEL, "trace")
+    assert.equal(logger.level, Level.Trace)
   })
 
-  it("should prefer --log to env var", () => {
+  it("should prefer --log to env var and --verbose to --log", () => {
     process.env.LOG_LEVEL = "debug"
     assert.deepEqual(parse(["--log", "info"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       log: "info",
     })
     assert.equal(process.env.LOG_LEVEL, "info")
+    assert.equal(logger.level, Level.Info)
+
+    process.env.LOG_LEVEL = "trace"
+    assert.deepEqual(parse(["--log", "info"]), {
+      ...defaults,
+      log: "info",
+    })
+    assert.equal(process.env.LOG_LEVEL, "info")
+    assert.equal(logger.level, Level.Info)
+
+    process.env.LOG_LEVEL = "warn"
+    assert.deepEqual(parse(["--log", "info", "--verbose"]), {
+      ...defaults,
+      log: "trace",
+      verbose: true,
+    })
+    assert.equal(process.env.LOG_LEVEL, "trace")
+    assert.equal(logger.level, Level.Trace)
+  })
+
+  it("should ignore invalid log level env var", () => {
+    process.env.LOG_LEVEL = "bogus"
+    assert.deepEqual(parse([]), defaults)
   })
 
   it("should error if value isn't provided", () => {
@@ -134,9 +166,7 @@ describe("cli", () => {
 
   it("should not error if the value is optional", () => {
     assert.deepEqual(parse(["--cert"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       cert: {
         value: undefined,
       },
@@ -147,34 +177,27 @@ describe("cli", () => {
     assert.throws(() => parse(["--socket", "--socket-path-value"]), /--socket requires a value/)
     // If you actually had a path like this you would do this instead:
     assert.deepEqual(parse(["--socket", "./--socket-path-value"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       socket: path.resolve("--socket-path-value"),
     })
     assert.throws(() => parse(["--cert", "--socket-path-value"]), /Unknown option --socket-path-value/)
   })
 
   it("should allow positional arguments before options", () => {
     assert.deepEqual(parse(["foo", "test", "--auth", "none"]), {
+      ...defaults,
       _: ["foo", "test"],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
       auth: "none",
     })
   })
 
   it("should support repeatable flags", () => {
     assert.deepEqual(parse(["--proxy-domain", "*.coder.com"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       "proxy-domain": ["*.coder.com"],
     })
     assert.deepEqual(parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "test.com"]), {
-      _: [],
-      "extensions-dir": path.join(xdgLocalDir, "extensions"),
-      "user-data-dir": xdgLocalDir,
+      ...defaults,
       "proxy-domain": ["*.coder.com", "test.com"],
     })
   })