From 334b5d179112564515260428587f8cbdd7c484c9 Mon Sep 17 00:00:00 2001 From: ANGkeith Date: Sat, 11 Jan 2025 12:00:26 +0800 Subject: [PATCH 1/2] fix: predefinedVariables should have the lowest precedence --- src/job.ts | 82 ++++++++++--------- .../variable-order/integration.test.ts | 2 +- 2 files changed, 46 insertions(+), 38 deletions(-) diff --git a/src/job.ts b/src/job.ts index a7132e5f0..709e33c94 100644 --- a/src/job.ts +++ b/src/job.ts @@ -135,14 +135,11 @@ export class Job { constructor (opt: JobOptions) { const jobData = opt.data; - const gitData = opt.gitData; const jobVariables = jobData.variables ?? {}; const variablesFromFiles = opt.variablesFromFiles; const argv = opt.argv; const cwd = argv.cwd; - const stateDir = argv.stateDir; const argvVariables = argv.variable; - const predefinedVariables = opt.predefinedVariables; const expandVariables = opt.expandVariables ?? true; this.argv = argv; @@ -167,39 +164,8 @@ export class Job { const matrixVariables = opt.matrixVariables ?? {}; const fileVariables = Utils.findEnvMatchedVariables(variablesFromFiles, this.fileVariablesDir); - this._variables = {...this.globalVariables, ...jobVariables, ...matrixVariables, ...predefinedVariables, ...fileVariables, ...argvVariables}; - - let ciProjectDir = `${cwd}`; - if (this.jobData["image"]) { - ciProjectDir = CI_PROJECT_DIR; - } else if (argv.shellIsolation) { - ciProjectDir = `${cwd}/${stateDir}/builds/${this.safeJobName}`; - } - - predefinedVariables["CI_JOB_ID"] = `${this.jobId}`; - predefinedVariables["CI_PIPELINE_ID"] = `${this.pipelineIid + 1000}`; - predefinedVariables["CI_PIPELINE_IID"] = `${this.pipelineIid}`; - predefinedVariables["CI_JOB_NAME"] = `${this.name}`; - predefinedVariables["CI_JOB_NAME_SLUG"] = `${this.name.replace(/[^a-z\d]+/ig, "-").replace(/^-/, "").slice(0, 63).replace(/-$/, "").toLowerCase()}`; - predefinedVariables["CI_JOB_STAGE"] = `${this.stage}`; - predefinedVariables["CI_PROJECT_DIR"] = ciProjectDir; - predefinedVariables["CI_JOB_URL"] = `${this._variables["CI_SERVER_URL"]}/${gitData.remote.group}/${gitData.remote.project}/-/jobs/${this.jobId}`; // Changes on rerun. - predefinedVariables["CI_PIPELINE_URL"] = `${this._variables["CI_SERVER_URL"]}/${gitData.remote.group}/${gitData.remote.project}/pipelines/${this.pipelineIid}`; - predefinedVariables["CI_ENVIRONMENT_NAME"] = this.environment?.name ?? ""; - predefinedVariables["CI_ENVIRONMENT_SLUG"] = this.environment?.name?.replace(/[^a-z\d]+/ig, "-").replace(/^-/, "").slice(0, 23).replace(/-$/, "").toLowerCase() ?? ""; - predefinedVariables["CI_ENVIRONMENT_URL"] = this.environment?.url ?? ""; - predefinedVariables["CI_ENVIRONMENT_TIER"] = this.environment?.deployment_tier ?? ""; - predefinedVariables["CI_ENVIRONMENT_ACTION"] = this.environment?.action ?? ""; - - if (opt.nodeIndex !== null) { - predefinedVariables["CI_NODE_INDEX"] = `${opt.nodeIndex}`; - } - predefinedVariables["CI_NODE_TOTAL"] = `${opt.nodesTotal}`; - predefinedVariables["CI_REGISTRY"] = `local-registry.${this.gitData.remote.host}`; - predefinedVariables["CI_REGISTRY_IMAGE"] = `$CI_REGISTRY/${this._variables["CI_PROJECT_PATH"].toLowerCase()}`; - - // NOTE: Update `this._variables` with the patched predefinedVariables - this._variables = {...this.globalVariables, ...jobVariables, ...matrixVariables, ...predefinedVariables, ...fileVariables, ...argvVariables}; + const predefinedVariables = this._predefinedVariables(opt); + this._variables = {...predefinedVariables, ...this.globalVariables, ...jobVariables, ...matrixVariables, ...fileVariables, ...argvVariables}; // Expand variables in rules:changes if (this.rules && expandVariables) { @@ -236,7 +202,7 @@ export class Job { const envMatchedVariables = Utils.findEnvMatchedVariables(variablesFromFiles, this.fileVariablesDir, this.environment); // Merge and expand after finding env matched variables - this._variables = {...this.globalVariables, ...jobVariables, ...matrixVariables, ...predefinedVariables, ...ruleVariables, ...envMatchedVariables, ...argvVariables}; + this._variables = {...predefinedVariables, ...this.globalVariables, ...jobVariables, ...matrixVariables, ...ruleVariables, ...envMatchedVariables, ...argvVariables}; // Delete variables the user intentionally wants unset for (const unsetVariable of argv.unsetVariables) { delete this._variables[unsetVariable]; @@ -280,6 +246,48 @@ export class Job { } } + /** + * Get the predefinedVariables that's enriched with the additional info that's only available when constructing + */ + private _predefinedVariables (opt: JobOptions) { + const argv = this.argv; + const cwd = argv.cwd; + const stateDir = this.argv.stateDir; + const gitData = this.gitData; + + let ciProjectDir = `${cwd}`; + if (this.jobData["image"]) { + ciProjectDir = CI_PROJECT_DIR; + } else if (argv.shellIsolation) { + ciProjectDir = `${cwd}/${stateDir}/builds/${this.safeJobName}`; + } + + const predefinedVariables = opt.predefinedVariables; + + predefinedVariables["CI_JOB_ID"] = `${this.jobId}`; + predefinedVariables["CI_PIPELINE_ID"] = `${this.pipelineIid + 1000}`; + predefinedVariables["CI_PIPELINE_IID"] = `${this.pipelineIid}`; + predefinedVariables["CI_JOB_NAME"] = `${this.name}`; + predefinedVariables["CI_JOB_NAME_SLUG"] = `${this.name.replace(/[^a-z\d]+/ig, "-").replace(/^-/, "").slice(0, 63).replace(/-$/, "").toLowerCase()}`; + predefinedVariables["CI_JOB_STAGE"] = `${this.stage}`; + predefinedVariables["CI_PROJECT_DIR"] = ciProjectDir; + predefinedVariables["CI_JOB_URL"] = `${predefinedVariables["CI_SERVER_URL"]}/${gitData.remote.group}/${gitData.remote.project}/-/jobs/${this.jobId}`; // Changes on rerun. + predefinedVariables["CI_PIPELINE_URL"] = `${predefinedVariables["CI_SERVER_URL"]}/${gitData.remote.group}/${gitData.remote.project}/pipelines/${this.pipelineIid}`; + predefinedVariables["CI_ENVIRONMENT_NAME"] = this.environment?.name ?? ""; + predefinedVariables["CI_ENVIRONMENT_SLUG"] = this.environment?.name?.replace(/[^a-z\d]+/ig, "-").replace(/^-/, "").slice(0, 23).replace(/-$/, "").toLowerCase() ?? ""; + predefinedVariables["CI_ENVIRONMENT_URL"] = this.environment?.url ?? ""; + predefinedVariables["CI_ENVIRONMENT_TIER"] = this.environment?.deployment_tier ?? ""; + predefinedVariables["CI_ENVIRONMENT_ACTION"] = this.environment?.action ?? ""; + + if (opt.nodeIndex !== null) { + predefinedVariables["CI_NODE_INDEX"] = `${opt.nodeIndex}`; + } + predefinedVariables["CI_NODE_TOTAL"] = `${opt.nodesTotal}`; + predefinedVariables["CI_REGISTRY"] = `local-registry.${this.gitData.remote.host}`; + predefinedVariables["CI_REGISTRY_IMAGE"] = `$CI_REGISTRY/${predefinedVariables["CI_PROJECT_PATH"].toLowerCase()}`; + return predefinedVariables; + } + get jobStatus () { if (this.preScriptsExitCode == null) return "pending"; if (this.preScriptsExitCode == 0) return "success"; diff --git a/tests/test-cases/variable-order/integration.test.ts b/tests/test-cases/variable-order/integration.test.ts index 628a07029..54641ce2c 100644 --- a/tests/test-cases/variable-order/integration.test.ts +++ b/tests/test-cases/variable-order/integration.test.ts @@ -21,7 +21,7 @@ test("variable-order --needs", async () => { chalk`{blueBright test-job} {greenBright >} PIPELINE_VARIABLE=pipeline-value`, chalk`{blueBright test-job} {greenBright >} JOB_VARIABLE=job-value`, chalk`{blueBright test-job} {greenBright >} OVERRIDDEN_BY_JOB=job-value`, - chalk`{blueBright test-job} {greenBright >} CI_PIPELINE_ID=1000`, + chalk`{blueBright test-job} {greenBright >} CI_PIPELINE_ID=pipeline-value`, chalk`{blueBright test-job} {greenBright >} HOME_VARIABLE=home-value`, chalk`{blueBright test-job} {greenBright >} PROJECT_VARIABLE=project-value`, chalk`{blueBright test-job} {greenBright >} PROD_ONLY_VARIABLE=notprod`, From 340e46c2d1637d96267eddfce41d0e18ac047683 Mon Sep 17 00:00:00 2001 From: ANGkeith Date: Sat, 11 Jan 2025 12:30:38 +0800 Subject: [PATCH 2/2] feat: discourage overriding of predefined variables --- src/job.ts | 22 ++++++++++++++- .../variable-order/integration.test.ts | 27 +++++++++++-------- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/job.ts b/src/job.ts index 709e33c94..9fff857c8 100644 --- a/src/job.ts +++ b/src/job.ts @@ -17,6 +17,7 @@ import * as yaml from "js-yaml"; import {Parser} from "./parser.js"; import globby from "globby"; import {validateIncludeLocal} from "./parser-includes.js"; +import terminalLink from "terminal-link"; const CI_PROJECT_DIR = "/gcl-builds"; const GCL_SHELL_PROMPT_PLACEHOLDER = ""; @@ -201,8 +202,11 @@ export class Job { } const envMatchedVariables = Utils.findEnvMatchedVariables(variablesFromFiles, this.fileVariablesDir, this.environment); + const userDefinedVariables = {...this.globalVariables, ...jobVariables, ...matrixVariables, ...ruleVariables, ...envMatchedVariables, ...argvVariables}; + this.discourageOverridingOfPredefinedVariables(predefinedVariables, userDefinedVariables); + // Merge and expand after finding env matched variables - this._variables = {...predefinedVariables, ...this.globalVariables, ...jobVariables, ...matrixVariables, ...ruleVariables, ...envMatchedVariables, ...argvVariables}; + this._variables = {...predefinedVariables, ...userDefinedVariables}; // Delete variables the user intentionally wants unset for (const unsetVariable of argv.unsetVariables) { delete this._variables[unsetVariable]; @@ -246,6 +250,22 @@ export class Job { } } + /** + * Warn when overriding of predefined variables is detected + */ + private discourageOverridingOfPredefinedVariables (predefinedVariables: {[name: string]: string}, userDefinedVariables: {[name: string]: string}) { + const predefinedVariablesKeys = Object.keys(predefinedVariables); + const userDefinedVariablesKeys = Object.keys(userDefinedVariables); + + const overridingOfPredefinedVariables = userDefinedVariablesKeys.filter(ele => predefinedVariablesKeys.includes(ele)); + if (overridingOfPredefinedVariables.length == 0) { + return; + } + + const linkToGitlab = "https://gitlab.com/gitlab-org/gitlab/-/blob/v17.7.1-ee/doc/ci/variables/predefined_variables.md?plain=1&ref_type=tags#L15-16"; + this.writeStreams.memoStdout(chalk`{bgYellowBright WARN } ${terminalLink("Avoid overriding predefined variables", linkToGitlab)} [{bold ${overridingOfPredefinedVariables}}] as it can cause the pipeline to behave unexpectedly.\n`); + } + /** * Get the predefinedVariables that's enriched with the additional info that's only available when constructing */ diff --git a/tests/test-cases/variable-order/integration.test.ts b/tests/test-cases/variable-order/integration.test.ts index 54641ce2c..7f2d30131 100644 --- a/tests/test-cases/variable-order/integration.test.ts +++ b/tests/test-cases/variable-order/integration.test.ts @@ -1,6 +1,5 @@ import {WriteStreamsMock} from "../../../src/write-streams.js"; import {handler} from "../../../src/handler.js"; -import chalk from "chalk"; import {initSpawnSpy} from "../../mocks/utils.mock.js"; import {WhenStatics} from "../../mocks/when-statics.js"; @@ -15,16 +14,22 @@ test("variable-order --needs", async () => { job: ["test-job"], variable: ["PROJECT_VARIABLE=project-value"], home: "tests/test-cases/variable-order/.home", + noColor: true, }, writeStreams); - const expected = [ - chalk`{blueBright test-job} {greenBright >} PIPELINE_VARIABLE=pipeline-value`, - chalk`{blueBright test-job} {greenBright >} JOB_VARIABLE=job-value`, - chalk`{blueBright test-job} {greenBright >} OVERRIDDEN_BY_JOB=job-value`, - chalk`{blueBright test-job} {greenBright >} CI_PIPELINE_ID=pipeline-value`, - chalk`{blueBright test-job} {greenBright >} HOME_VARIABLE=home-value`, - chalk`{blueBright test-job} {greenBright >} PROJECT_VARIABLE=project-value`, - chalk`{blueBright test-job} {greenBright >} PROD_ONLY_VARIABLE=notprod`, - ]; - expect(writeStreams.stdoutLines).toEqual(expect.arrayContaining(expected)); + const expected = ` +test-job > PIPELINE_VARIABLE=pipeline-value +test-job > JOB_VARIABLE=job-value +test-job > OVERRIDDEN_BY_JOB=job-value +test-job > CI_PIPELINE_ID=pipeline-value +test-job > CI_JOB_ID=job-value +test-job > HOME_VARIABLE=home-value +test-job > PROJECT_VARIABLE=project-value +test-job > PROD_ONLY_VARIABLE=notprod +`.trim(); + + const filteredStdout = writeStreams.stdoutLines.filter(f => f.startsWith("test-job >")).join("\n"); + expect(filteredStdout).toEqual(expected); + + expect(writeStreams.stdoutLines).toEqual(expect.arrayContaining([" WARN Avoid overriding predefined variables (​https://gitlab.com/gitlab-org/gitlab/-/blob/v17.7.1-ee/doc/ci/variables/predefined_variables.md?plain=1&ref_type=tags#L15-16​) [CI_PIPELINE_ID,CI_JOB_ID] as it can cause the pipeline to behave unexpectedly."])); });