From 021c084e4315f6fbc40ca0b9feed1cf0930d43d7 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 15 Sep 2020 13:43:07 -0500 Subject: [PATCH] Move log level defaults into setDefaults This will allow cliArgs to be only the actual arguments the user passed which will be used for some logic around opening in existing instances. --- src/node/cli.ts | 30 ++++++++++----------- test/cli.test.ts | 69 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 61 insertions(+), 38 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 3d4d0659c..a52796a14 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -327,6 +327,21 @@ export const parse = ( logger.debug("parsed command line", field("args", args)) + return args +} + +export async function setDefaults(args: Args): Promise { + args = { ...args } + + if (!args["user-data-dir"]) { + await copyOldMacOSDataDir() + args["user-data-dir"] = paths.data + } + + if (!args["extensions-dir"]) { + args["extensions-dir"] = path.join(args["user-data-dir"], "extensions") + } + // --verbose takes priority over --log and --log takes priority over the // environment variable. if (args.verbose) { @@ -369,21 +384,6 @@ export const parse = ( return args } -export async function setDefaults(args: Args): Promise { - args = { ...args } - - if (!args["user-data-dir"]) { - await copyOldMacOSDataDir() - args["user-data-dir"] = paths.data - } - - if (!args["extensions-dir"]) { - args["extensions-dir"] = path.join(args["user-data-dir"], "extensions") - } - - return args -} - async function defaultConfigFile(): Promise { return `bind-addr: 127.0.0.1:8080 auth: password diff --git a/test/cli.test.ts b/test/cli.test.ts index f4f6c8849..fe78659d8 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1,20 +1,23 @@ -import { logger, Level } from "@coder/logger" +import { Level, logger } from "@coder/logger" import * as assert from "assert" import * as path from "path" -import { parse } from "../src/node/cli" +import { parse, setDefaults } from "../src/node/cli" +import { paths } from "../src/node/util" describe("cli", () => { beforeEach(() => { delete process.env.LOG_LEVEL }) - // The parser will always fill these out. + // The parser should not set any defaults so the caller can determine what + // values the user actually set. These are set after calling `setDefaults`. const defaults = { - _: [], + "extensions-dir": path.join(paths.data, "extensions"), + "user-data-dir": paths.data, } it("should set defaults", () => { - assert.deepEqual(parse([]), defaults) + assert.deepEqual(parse([]), { _: [] }) }) it("should parse all available options", () => { @@ -69,7 +72,7 @@ describe("cli", () => { help: true, host: "0.0.0.0", json: true, - log: "trace", + log: "error", open: true, port: 8081, socket: path.resolve("mumble"), @@ -83,19 +86,20 @@ describe("cli", () => { it("should work with short options", () => { assert.deepEqual(parse(["-vvv", "-v"]), { - ...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", () => { + it("should use log level env var", async () => { + const args = parse([]) + assert.deepEqual(args, { _: [] }) + process.env.LOG_LEVEL = "debug" - assert.deepEqual(parse([]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "debug", verbose: false, }) @@ -103,8 +107,9 @@ describe("cli", () => { assert.equal(logger.level, Level.Debug) process.env.LOG_LEVEL = "trace" - assert.deepEqual(parse([]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "trace", verbose: true, }) @@ -113,9 +118,16 @@ describe("cli", () => { }) it("should prefer --log to env var and --verbose to --log", async () => { + let args = parse(["--log", "info"]) + assert.deepEqual(args, { + _: [], + log: "info", + }) + process.env.LOG_LEVEL = "debug" - assert.deepEqual(parse(["--log", "info"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "info", verbose: false, }) @@ -123,17 +135,26 @@ describe("cli", () => { assert.equal(logger.level, Level.Info) process.env.LOG_LEVEL = "trace" - assert.deepEqual(parse(["--log", "info"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "info", verbose: false, }) assert.equal(process.env.LOG_LEVEL, "info") assert.equal(logger.level, Level.Info) + args = parse(["--log", "info", "--verbose"]) + assert.deepEqual(args, { + _: [], + log: "info", + verbose: true, + }) + process.env.LOG_LEVEL = "warn" - assert.deepEqual(parse(["--log", "info", "--verbose"]), { + assert.deepEqual(await setDefaults(args), { ...defaults, + _: [], log: "trace", verbose: true, }) @@ -141,9 +162,12 @@ describe("cli", () => { assert.equal(logger.level, Level.Trace) }) - it("should ignore invalid log level env var", () => { + it("should ignore invalid log level env var", async () => { process.env.LOG_LEVEL = "bogus" - assert.deepEqual(parse([]), defaults) + assert.deepEqual(await setDefaults(parse([])), { + _: [], + ...defaults, + }) }) it("should error if value isn't provided", () => { @@ -166,7 +190,7 @@ describe("cli", () => { it("should not error if the value is optional", () => { assert.deepEqual(parse(["--cert"]), { - ...defaults, + _: [], cert: { value: undefined, }, @@ -177,7 +201,7 @@ 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"]), { - ...defaults, + _: [], socket: path.resolve("--socket-path-value"), }) assert.throws(() => parse(["--cert", "--socket-path-value"]), /Unknown option --socket-path-value/) @@ -185,7 +209,6 @@ describe("cli", () => { it("should allow positional arguments before options", () => { assert.deepEqual(parse(["foo", "test", "--auth", "none"]), { - ...defaults, _: ["foo", "test"], auth: "none", }) @@ -193,11 +216,11 @@ describe("cli", () => { it("should support repeatable flags", () => { assert.deepEqual(parse(["--proxy-domain", "*.coder.com"]), { - ...defaults, + _: [], "proxy-domain": ["*.coder.com"], }) assert.deepEqual(parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "test.com"]), { - ...defaults, + _: [], "proxy-domain": ["*.coder.com", "test.com"], }) })