ROSAENG-6811: Allow Gangway pod_spec_options.envs to override step parameters#5218
ROSAENG-6811: Allow Gangway pod_spec_options.envs to override step parameters#5218dustman9000 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@dustman9000: This pull request references ROSAENG-6811 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds environment variable parameter override capability to the multi-stage step parameter generation. The ChangesEnvironment Variable Parameter Override
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dustman9000 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/steps/multi_stage/gen.go (1)
404-420: ⚡ Quick winDocument the parameter resolution order.
The function now implements a three-tier parameter resolution with specific precedence:
os.Getenv(pod env vars) >s.env(test config) >Default. This non-obvious priority order should be documented to help maintainers understand the override behavior, especially since pod env vars now take precedence over test configuration.📝 Suggested documentation
+// generateParams resolves step parameters using a three-tier priority system: +// 1. Process environment variables (os.Getenv) - highest priority, enables Gangway pod_spec_options.envs overrides +// 2. Test environment config (s.env) - from ci-operator config TestEnvironment +// 3. Parameter default values (env.Default) - lowest priority fallback +// Only parameters explicitly declared in the step's Environment list are considered. func (s *multiStageTestStep) generateParams(env []api.StepParameter) []coreapi.EnvVar { var ret []coreapi.EnvVar🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/steps/multi_stage/gen.go` around lines 404 - 420, Update the documentation/comment for the function generateParams to explicitly state the parameter resolution precedence: values from os.Getenv(env.Name) (pod/environment variables) override values in s.env (test configuration), which in turn override env.Default; mention the exact symbols used (generateParams, s.env, os.Getenv, env.Default/env.Name) and indicate that returned coreapi.EnvVar uses the chosen value so reviewers and maintainers understand the three-tier override behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/steps/multi_stage/gen.go`:
- Around line 414-416: Add a unit test in package pkg/steps/multi_stage (e.g.,
TestEnvVarOverridePrecedence) that exercises the StepParameter resolution logic
around os.Getenv(env.Name): create a StepParameter with a Name and Default,
prepare s.env with a different value, then use t.Setenv to set that environment
variable to a non-empty value and assert the resolver returns the t.Setenv value
(env var wins over s.env and env.Default); also add a subcase where t.Setenv
sets the variable to the empty string and assert the resolver falls back to
s.env or env.Default (empty env var should not override). Locate the resolver
invocation near the code that references env.Name, s.env[env.Name], and
env.Default in gen.go to call the same function/path you test.
---
Nitpick comments:
In `@pkg/steps/multi_stage/gen.go`:
- Around line 404-420: Update the documentation/comment for the function
generateParams to explicitly state the parameter resolution precedence: values
from os.Getenv(env.Name) (pod/environment variables) override values in s.env
(test configuration), which in turn override env.Default; mention the exact
symbols used (generateParams, s.env, os.Getenv, env.Default/env.Name) and
indicate that returned coreapi.EnvVar uses the chosen value so reviewers and
maintainers understand the three-tier override behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a7cf3cf1-0224-4d24-ba41-acfc259d59d6
📒 Files selected for processing (1)
pkg/steps/multi_stage/gen.go
|
Scheduling tests matching the |
When Gangway triggers a periodic job with pod_spec_options.envs, those env vars are set on the ci-operator pod but not passed through to step containers. This change checks os.Getenv for each declared step parameter, allowing pod-level env vars to override config defaults. Only env vars that match declared step parameter names are considered, so arbitrary pod env vars (PATH, HOME, etc.) cannot leak into steps. Jira: https://redhat.atlassian.net/browse/ROSAENG-6811
dustman9000
force-pushed
the
rosaeng-6811/gangway-env-passthrough
branch
from
1807274 to
8260e95
Compare
|
Scheduling tests matching the |
|
@dustman9000: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
When Gangway triggers a periodic job with pod_spec_options.envs, those env vars are appended to the ci-operator pod. However, ci-operator builds step container envs from its YAML config (TestEnvironment map), not from os.Getenv(). Runtime env overrides from Gangway never reach step containers.
This PR adds a check to generateParams in pkg/steps/multi_stage/gen.go: after checking the config env value, check os.Getenv for the same parameter name. If set, the pod-level env var takes precedence.
Safety: Only env vars matching declared step parameter names (from the step ref env list) are considered. Arbitrary pod env vars cannot leak into steps.
Use case: SAPM promotion pipelines trigger Prow periodic jobs via Gangway to run operator e2e tests. Different OCM environments need different OCM_LOGIN_ENV values. Without this change, each combination needs a separate periodic job. With it, one periodic per operator is sufficient.
Test plan
Jira: https://redhat.atlassian.net/browse/ROSAENG-6811
Summary
This PR changes ci-operator's multi-stage step parameter resolution so pod-level environment variables (e.g., those injected via Gangway's pod_spec_options.envs) can override step parameter values when the env var name matches a declared step parameter.
What Changed
Practical Impact for CI Users / Operators
ci-operator jobs triggered via Gangway can now pass runtime environment overrides (for example OCM_LOGIN_ENV) through pod_spec_options.envs and have those values reach step containers without changing ci-operator YAML. This enables scenarios like SAPM promotion pipelines that use Gangway to trigger periodic jobs across multiple environments without creating separate periodic jobs for each environment.
Component Affected
Safety & Scope