Fix GCL_ array env vars not splitting semicolon-separated values (#1778)

Co-authored-by: Mads Jon Nielsen <mjn@cego.dk>

Author: firecow

src/argv.ts

@@ -8,6 +8,17 @@ import {WriteStreams} from "./write-streams.js";
 import chalkBase from "chalk";
 import chalk from "chalk-template";
 
+export function splitSemicolonEnvVars (argv: Record<string, any>, arrayKeys: Set<string>, env: Record<string, string | undefined>): void {
+    for (const [envKey, envValue] of Object.entries(env)) {
+        if (!envKey.startsWith("GCL_") || envValue == null) continue;
+        const optionName = camelCase(envKey.slice(4));
+        if (!arrayKeys.has(optionName)) continue;
+        const currentVal = argv[optionName];
+        if (!Array.isArray(currentVal) || currentVal.length !== 1 || currentVal[0] !== envValue) continue;
+        argv[optionName] = envValue.split(";");
+    }
+}
+
 async function isInGitRepository () {
     try {
         await Utils.spawn(["git", "rev-parse", "--is-inside-work-tree"]);
@@ -103,36 +114,43 @@ export class Argv {
     }
 
     private injectDotenv (potentialDotenvFilepath: string, argv: any) {
-        if (fs.existsSync(potentialDotenvFilepath)) {
-            const config = dotenv.parse(fs.readFileSync(potentialDotenvFilepath));
-            for (const [key, value] of Object.entries(config)) {
-                const argKey = camelCase(key);
-
-                // Special handle KEY=VALUE variable keys
-                if (argKey === "variable") {
-                    let currentVal = argv[argKey];
-                    if (currentVal == null) {
-                        currentVal = [];
-                        this.map.set(argKey, currentVal);
-                    }
-                    if (!Array.isArray(currentVal)) {
-                        continue;
-                    }
-                    for (const pair of value.split(" ")) {
-                        currentVal.unshift(pair);
-                    }
-                } else if (argv[argKey] == null) {
-                    // Work around `dotenv.parse` limitation https://github.com/motdotla/dotenv/issues/51#issuecomment-552559070
-                    if (value === "true") this.map.set(argKey, true);
-                    else if (value === "false") this.map.set(argKey, false);
-                    else if (value === "null") this.map.set(argKey, null);
-                    else if (!isNaN(Number(value))) this.map.set(argKey, Number(value));
-                    else this.map.set(argKey, value);
+        if (!fs.existsSync(potentialDotenvFilepath)) return;
+
+        const config = dotenv.parse(fs.readFileSync(potentialDotenvFilepath));
+        for (const [key, value] of Object.entries(config)) {
+            const argKey = camelCase(key);
+
+            // variable is additive — merge dotenv values with CLI values
+            if (argKey === "variable") {
+                let currentVal = argv[argKey];
+                if (currentVal == null) {
+                    currentVal = [];
+                    this.map.set(argKey, currentVal);
+                }
+                if (!Array.isArray(currentVal)) {
+                    continue;
                 }
+                for (const pair of value.split(" ")) {
+                    currentVal.unshift(pair);
+                }
+            } else if (argv[argKey] == null) {
+                // Work around `dotenv.parse` limitation https://github.com/motdotla/dotenv/issues/51#issuecomment-552559070
+                if (value === "true") this.map.set(argKey, true);
+                else if (value === "false") this.map.set(argKey, false);
+                else if (value === "null") this.map.set(argKey, null);
+                else if (!isNaN(Number(value))) this.map.set(argKey, Number(value));
+                else this.map.set(argKey, value);
             }
         }
     }
 
+    private getStringArray (key: string): string[] {
+        const val = this.map.get(key) ?? [];
+        if (Array.isArray(val)) return val;
+        if (typeof val === "string") return val.split(" ");
+        return [];
+    }
+
     get cwd (): string {
         let cwd = this.map.get("cwd") ?? ".";
         assert(typeof cwd != "object", "--cwd option cannot be an array");
@@ -163,20 +181,11 @@ export class Argv {
         return (this.map.get("home") ?? process.env.HOME ?? "").replace(/\/$/, "");
     }
 
-    get volume (): string[] {
-        const val = this.map.get("volume") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get volume (): string[] { return this.getStringArray("volume"); }
 
-    get network (): string[] {
-        const val = this.map.get("network") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get network (): string[] { return this.getStringArray("network"); }
 
-    get extraHost (): string[] {
-        const val = this.map.get("extraHost") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get extraHost (): string[] { return this.getStringArray("extraHost"); }
 
     get caFile (): string | null {
         return this.map.get("caFile") ?? null;
@@ -194,32 +203,26 @@ export class Argv {
         return this.map.get("pullPolicy") ?? "if-not-present";
     }
 
-    get remoteVariables (): string[] {
-        const val = this.map.get("remoteVariables") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get remoteVariables (): string[] { return this.getStringArray("remoteVariables"); }
 
     get variable (): {[key: string]: string} {
-        const val = this.map.get("variable");
         const variables: {[key: string]: string} = {};
-        const pairs = typeof val == "string" ? val.split(" ") : val;
-        (pairs ?? []).forEach((variablePair: string) => {
-            const exec = /(?<key>\w*?)(=)(?<value>(.|\n|\r)*)/.exec(variablePair);
-            if (exec?.groups?.key) {
-                variables[exec.groups.key] = exec?.groups?.value;
+        for (const pair of this.getStringArray("variable")) {
+            const eqIndex = pair.indexOf("=");
+            if (eqIndex < 1) continue;
+            const key = pair.substring(0, eqIndex);
+            if (/^\w+$/.test(key)) {
+                variables[key] = pair.substring(eqIndex + 1);
             }
-        });
+        }
         return variables;
     }
 
     get unsetVariables (): string[] {
         return this.map.get("unsetVariable") ?? [];
     }
 
-    get manual (): string[] {
-        const val = this.map.get("manual") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get manual (): string[] { return this.getStringArray("manual"); }
 
     get job (): string[] {
         return this.map.get("job") ?? [];
@@ -250,10 +253,7 @@ export class Argv {
         return this.map.get("privileged") ?? false;
     }
 
-    get device (): string[] {
-        const val = this.map.get("device") ?? [];
-        return typeof val == "string" ? val.split(" ") : val;
-    }
+    get device (): string[] { return this.getStringArray("device"); }
 
     get ulimit (): string | null {
         const ulimit = this.map.get("ulimit");

src/index.ts

@@ -1,6 +1,8 @@
 #!/usr/bin/env node
 import chalk from "chalk-template";
 import yargs from "yargs";
+import camelCase from "camelcase";
+import {splitSemicolonEnvVars} from "./argv.js";
 import {Parser} from "./parser.js";
 import * as state from "./state.js";
 import {WriteStreamsProcess, WriteStreamsMock} from "./write-streams.js";
@@ -42,6 +44,8 @@ process.on("SIGUSR2", async () => {
         .command({
             handler: async (argv) => {
                 try {
+                    const arrayKeys = new Set(yparser.getOptions().array.map((k: string) => camelCase(k)));
+                    splitSemicolonEnvVars(argv, arrayKeys, process.env);
                     injectGclVariableEnvVars(argv, gclVariableEnvVars);
                     await handler(argv, new WriteStreamsProcess(), jobs);
                     const failedJobs = Executor.getFailed(jobs);

tests/test-cases/cli-option-variables/integration.test.ts

@@ -26,3 +26,21 @@ line string`],
     ];
     expect(writeStreams.stdoutLines).toEqual(expect.arrayContaining(expected));
 });
+
+test("cli-option-variables --variable with semicolons in value preserves them", async () => {
+    const writeStreams = new WriteStreamsMock();
+    await handler({
+        cwd: "tests/test-cases/cli-option-variables",
+        job: ["test-job"],
+        variable: ["CLI_VAR=host=db;port=5432", "CLI_VAR_DOT=dotdot", `CLI_MULTILINE=This is a multi
+line string`],
+    }, writeStreams);
+
+    const expected = [
+        chalk`{blueBright test-job} {greenBright >} host=db;port=5432`,
+        chalk`{blueBright test-job} {greenBright >} dotdot`,
+        chalk`{blueBright test-job} {greenBright >} This is a multi`,
+        chalk`{blueBright test-job} {greenBright >} line string`,
+    ];
+    expect(writeStreams.stdoutLines).toEqual(expect.arrayContaining(expected));
+});

tests/test-cases/gcl-env-variable-split/.gitlab-ci.yml

@@ -0,0 +1,6 @@
+---
+test-job:
+  script:
+    - echo ${VAR1}
+    - echo ${VAR2}
+    - echo ${VAR3}