Allow merging Default Setup config with config file#3994
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR adds feature-flagged support for merging Default Setup’s config input with a repository-provided config-file, enabling both sources to contribute supported configuration fields in Default Setup runs. It also refactors config loading logic to make this behavior testable and expands the environment abstraction used for parallelizable tests.
Changes:
- Introduces
Feature.AllowMergeConfigFilesand refactors config initialization by extractingdetermineUserConfig. - Adds
mergeUserConfigsto combine supported config properties (notablythreat-modelsand reserveddefault-setup) and adds unit tests for the merge behavior. - Extends the
Envabstraction (entries,set) and threads optionalEnvthrough several actions util helpers.
Show a summary per file
| File | Description |
|---|---|
| src/util.ts | Extends getEnv to support listing and mutating env entries (entries, set). |
| src/testing-utils.ts | Allows setting base Actions env vars into a provided Env for tests. |
| src/feature-flags.ts | Adds the AllowMergeConfigFiles feature flag configuration. |
| src/environment.ts | Adds CODEQL_ACTION_TEMP and expands the Env interface. |
| src/config/db-config.ts | Adds config types for Default Setup and implements mergeUserConfigs. |
| src/config/db-config.test.ts | Adds unit tests for mergeUserConfigs. |
| src/config-utils.ts | Extracts determineUserConfig and implements merge logic under FF + Default Setup. |
| src/config-utils.test.ts | Adds targeted tests for determineUserConfig behavior (including merge cases). |
| src/actions-util.ts | Threads optional Env through several helpers and uses EnvVar.TEMP for temp dir selection. |
| lib/entry-points.js | Not reviewed (excluded by policy; also generated output). |
Review details
Files excluded by content exclusion policy (1)
- lib/entry-points.js
- Files reviewed: 9/10 changed files
- Comments generated: 5
- Review effort level: Low
This makes the implementations more consistent for now, and less error-prone
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Review details
Files excluded by content exclusion policy (1)
- lib/entry-points.js
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Low
| * @param fromConfigFile The user-supplied configuration. | ||
| * @returns The combination of both configuration files. | ||
| */ | ||
| export function mergeUserConfigs( |
There was a problem hiding this comment.
Can we indicate in the function name that this is about merging a user config with a default setup config?
| result["threat-models"] = Array.from(threatModels); | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
It is probably worth logging a warning that is sent to telemetry if we find any other properties in fromConfigInput. If we format this as a telemetry diagnostic, we can set the internal-error tag on the reporting descriptor to send exceptions to Sentry.
| test("determineUserConfig - empty config when neither input is specified", async (t) => { | ||
| await withTmpDir(async (tmpDir) => { | ||
| const logger = new RecordingLogger(); | ||
| const env = util.getEnv(DEFAULT_ACTIONS_VARS); |
There was a problem hiding this comment.
Should this act on a copy of DEFAULT_ACTIONS_VARS, to avoid potential calls to Env.set mutating DEFAULT_ACTIONS_VARS? I know that determineUserConfig is read only, but this could be confusing if behaviour changes.
| // The loaded configuration should match the result of merging | ||
| // `defaultSetupConfigInput` and `simpleConfigFileContents`. |
There was a problem hiding this comment.
Let's also check that inputs.configFile was mutated and that the contents on disk match the returned config
This PR introduces support for merging the
configinputs that are provided by Default Setup with custom configuration files. That can be the case if, for example, a configuration file is configured by the corresponding repository property. With the change in this PR, a supported scenario is that e.g.:github-codeql-config-fileis set.getConfigFileInputreturns the value from the repository property.getOptionalInput("config")returns the configuration provided by Default Setup.determineUserConfig, the two configurations are merged if the corresponding FF is enabled.Notes for reviewers / deliberate implementation aspects
determineUserConfigis refactored out ofinitConfig. This PR does not make any changes to the refactored code that aren't required for the objective of this PR.inputs.configFileis mutated to the resulting configuration file path as ininitConfigbefore. No change in behaviour. The new tests fordetermineUserConfigcheck that this behaves as expected in relevant cases.inputsproperties is performed. Just likeinitConfigbefore,determineUserConfigassumes that the necessary validation before this step has taken place. Further validation may occur in e.g.loadUserConfig.Feature.ValidateDbConfig. This is deliberate because there are already tests exercisingloadUserConfig's behaviour if that FF is enabled. In any case,Feature.ValidateDbConfigneeds more work to work correctly that is not in the scope of this PR.process.env#3993 to allow the tests to run in parallel.determineUserConfig. That's for the existingloadUserConfigfunction and its tests.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist