Define each feature flag once in the job environment
What does this MR do?
It keeps the feature flags set in the runner's environment setting out of the base variables that GetAllVariables returns, the same way it already keeps them out of the job variables. Each feature flag is then defined once in the job environment, with the value the runner resolved for it.
It is five commits:
- A test that pins what
GetAllVariablesdoes with the runner environment today: an ordinary variable, and one named like a feature flag that no feature flag has, each reach it once. Nothing tested that before. getNonFeatureFlagJobVariablesbecomeswithoutFeatureFlags, which takes the list to filter. No change in behavior.- The fix:
GetAllVariablesfilters the feature flags out ofgetBaseVariablesBeforeJob(). That function itself is unchanged, because the feature flag resolution reads the runner environment through it. Tests incommon, and aTestSetupBuildPodcase for the Kubernetes build container. - The feature flags page states the order in which the places that set a feature flag take precedence, and that a feature flag only usable in
[runners.feature_flags]takes its value from that section or its default value. - The two suggestions from the documentation review, applied as proposed.
Why was this MR needed?
Since !5782 (merged), GetAllVariables starts with every feature flag at its resolved value and drops feature flags from the job variables, so a flag set in .gitlab-ci.yml, the case the issue describes, is defined once. The runner environment setting, the case reported later in the issue thread, was not filtered: getBaseVariablesBeforeJob appends Runner.GetVariables(), so environment = ["FF_X=..."] was defined a second time, after the resolved value, with its raw value.
Whatever reads the job environment in order keeps the last definition. The job script exports every variable in order (exportVariables in shells/abstract.go), and the Kubernetes executor has dropped duplicate names keeping the last one since !5325 (merged), so its container specs no longer define a name twice, which is what the warnings quoted in the issue came from. Both therefore give the job, and the helper commands its script runs, the runner environment's value even where the runner uses another one:
[runners.feature_flags]setsFF_USE_FASTZIP = trueandenvironmentsetsFF_USE_FASTZIP=false: the runner resolvestrue, the job environment ends withfalse.environmentsetsFF_USE_FASTZIP=falseand a job sets the variableFF_USE_FASTZIP: "true": a job variable takes precedence over the runner environment, so the runner resolvestrue, and the job environment still ends withfalse.
The helper reads some flags from that environment. The artifacts upload stage exports the job variables and then runs the helper, which picks its archiver from FF_USE_FASTZIP (registerArchivers in commands/helpers/archiver.go), so in the second case the job's opt-in did not reach it. I have shown this with the tests below, not with a real job.
What's the best way to test this MR?
go test ./common/ -run 'TestGetAllVariables|TestIsFeatureFlagOn' -count=1 -v
go test ./executors/kubernetes/ -run 'TestSetupBuildPod/passes_the_resolved_value' -count=1 -vThe tests the third commit adds fail on the second commit and pass on the third. I also ran on each commit go build ./..., go vet and go test for ./common/ and ./executors/kubernetes/; on the last one go test ./common/... ./executors/... ./shells/... ./helpers/featureflags/..., go test -race ./common/ ./executors/kubernetes/, the feature flag tests with RUNNER_TEST_FEATURE_FLAGS set as the CI jobs set it, golangci-lint 2.12.2 from the CI runner-linters:2.12.2-go1.26.5 image on both packages (no issues), the check-test-directives script, and markdownlint-cli2 0.23.2 and Vale 3.21.0 on the docs page (no error or warning in the new section).
The third commit's tests on the second commit (excerpt)
Error: "[FF_NETWORK_PER_BUILD=true FF_NETWORK_PER_BUILD=false]" should have 1 item(s), but has 2
Error: "[FF_NETWORK_PER_BUILD=true FF_NETWORK_PER_BUILD=1]" should have 1 item(s), but has 2
--- FAIL: TestGetAllVariables_FeatureFlagFromRunnerEnvironment (0.00s)
--- FAIL: TestGetAllVariables_FeatureFlagFromRunnerEnvironment/TOML_overrides_the_runner_environment (0.00s)
--- FAIL: TestGetAllVariables_FeatureFlagFromRunnerEnvironment/job_variable_overrides_the_runner_environment (0.00s)
--- FAIL: TestGetAllVariables_FeatureFlagFromRunnerEnvironment/runner_environment_enables_the_flag (0.00s)
--- FAIL: TestGetAllVariables_FeatureFlagFromRunnerEnvironment/runner_environment_enables_the_flag_with_1 (0.00s)
Error: Not equal:
expected: []string{"true"}
actual : []string{"false"}
--- FAIL: TestSetupBuildPod (0.00s)
--- FAIL: TestSetupBuildPod/passes_the_resolved_value_of_a_feature_flag_set_in_the_runner_environment (0.00s)What are the relevant issue numbers?
Closes #36893