Allow testing step registry changes via payload-job-with-prs#4965
Allow testing step registry changes via payload-job-with-prs#4965jianzhangbjz wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@jianzhangbjz: This PR was included in a payload test run from openshift/operator-framework-olm#1236
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3bdae7b0-1233-11f1-8b95-7266b558161e-0 |
WalkthroughAdds detection for when Changes
Sequence DiagramsequenceDiagram
participant Generator as ProwJob Generator
participant Analyzer as PR Analyzer
participant Mutator as Registry Mutator
participant PodSpec as Pod Spec
Generator->>Analyzer: hasReleaseRepoPR(prs)
Analyzer-->>Generator: true/false
alt release repo present
Generator->>Mutator: Registry(releaseStepRegistryPath)
Mutator->>PodSpec: append --registry=<path> to first container
PodSpec-->>Mutator: mutated PodSpec
Mutator-->>Generator: mutated PodSpec
else release repo absent
Generator->>Mutator: Registry("") (no-op)
Mutator-->>Generator: original PodSpec
end
Generator-->>Generator: configure ProwJob with resulting PodSpec
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (3)
pkg/prowgen/podspec_test.go (1)
532-574: Consider usingtesthelper.CompareWithFixturefor consistency with the rest of the file.Every other mutator test in this file (
TestVariantviaTestNewCiOperatorPodSpecGenerator,TestGitHubToken,TestReleaseLatest,TestReleaseInitial,TestTargets, etc.) delegates assertion totesthelper.CompareWithFixture.TestRegistryis the only exception, relying on ad-hoc arg scanning. Using the fixture approach would also provide a golden-file record of the full expected PodSpec rather than just the presence of one arg.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/podspec_test.go` around lines 532 - 574, TestRegistry uses ad-hoc scanning for a single arg instead of the project's standard golden-file assertion; change the test to generate the podspec using NewCiOperatorPodSpecGenerator(), call g.Add(Registry(...)) and g.Build() as before, then replace the manual arg loop with testhelper.CompareWithFixture(t, "TestRegistry", podspec) (or an appropriate fixture name) so the full PodSpec is compared against the fixture; ensure the fixture file is added/updated to reflect both test cases (non-empty and empty registry) or split into two fixtures if needed.pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go (1)
626-633: Subtests are missingt.Parallel().
TestReconcile's subtests callt.Parallel(), but these don't. Minor inconsistency.♻️ Proposed fix
for _, tc := range tests { + tc := tc t.Run(tc.name, func(t *testing.T) { + t.Parallel() got := hasReleaseRepoPR(tc.prs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go` around lines 626 - 633, Subtests iterating over tests and calling t.Run should be marked parallel and must capture the loop variable to avoid races: inside the t.Run closure for the table-driven loop that calls hasReleaseRepoPR, add t.Parallel() as the first statement and also add a local capture like tc := tc (or copy the loop variable) before using it in the closure so the test data for each subtest remains isolated.pkg/controller/prpqr_reconciler/prpqr_reconciler.go (1)
644-649: Extract the hardcoded registry path to a named constant.The path
/go/src/github.com/openshift/release/ci-operator/step-registryis a magic string with no symbolic name. Extracting it makes future updates and codebase searches easier.♻️ Proposed refactor
At the top of the file alongside the existing
constblock:const releaseStepRegistryPath = "/go/src/github.com/openshift/release/ci-operator/step-registry"Then at lines 647-649:
- if hasReleaseRepoPR(prs) { - jobBaseGen.PodSpec.Add(prowgen.Registry("/go/src/github.com/openshift/release/ci-operator/step-registry")) - } + if hasReleaseRepoPR(prs) { + jobBaseGen.PodSpec.Add(prowgen.Registry(releaseStepRegistryPath)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/prpqr_reconciler/prpqr_reconciler.go` around lines 644 - 649, Extract the hardcoded path string used in the Registry call into a named constant and use that constant instead of the literal; add a constant like releaseStepRegistryPath = "/go/src/github.com/openshift/release/ci-operator/step-registry" in the existing const block at the top of the file, then replace the literal in the hasReleaseRepoPR handling (the call jobBaseGen.PodSpec.Add(prowgen.Registry(...))) with prowgen.Registry(releaseStepRegistryPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/podspec_test.go`:
- Around line 560-571: Add an explicit negative assertion for the empty-path
case so the test fails if a flag like "--registry=" is emitted; in the
podspec_test loop where tc.wantArg is checked, when tc.wantArg == "" iterate
podspec.Containers[0].Args and assert that no arg has the prefix "--registry"
(or equals "--registry=") — if any such arg is found call t.Errorf; keep the
existing positive check for non-empty tc.wantArg unchanged and reuse the podspec
and tc.wantArg identifiers.
---
Nitpick comments:
In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go`:
- Around line 626-633: Subtests iterating over tests and calling t.Run should be
marked parallel and must capture the loop variable to avoid races: inside the
t.Run closure for the table-driven loop that calls hasReleaseRepoPR, add
t.Parallel() as the first statement and also add a local capture like tc := tc
(or copy the loop variable) before using it in the closure so the test data for
each subtest remains isolated.
In `@pkg/controller/prpqr_reconciler/prpqr_reconciler.go`:
- Around line 644-649: Extract the hardcoded path string used in the Registry
call into a named constant and use that constant instead of the literal; add a
constant like releaseStepRegistryPath =
"/go/src/github.com/openshift/release/ci-operator/step-registry" in the existing
const block at the top of the file, then replace the literal in the
hasReleaseRepoPR handling (the call
jobBaseGen.PodSpec.Add(prowgen.Registry(...))) with
prowgen.Registry(releaseStepRegistryPath).
In `@pkg/prowgen/podspec_test.go`:
- Around line 532-574: TestRegistry uses ad-hoc scanning for a single arg
instead of the project's standard golden-file assertion; change the test to
generate the podspec using NewCiOperatorPodSpecGenerator(), call
g.Add(Registry(...)) and g.Build() as before, then replace the manual arg loop
with testhelper.CompareWithFixture(t, "TestRegistry", podspec) (or an
appropriate fixture name) so the full PodSpec is compared against the fixture;
ensure the fixture file is added/updated to reflect both test cases (non-empty
and empty registry) or split into two fixtures if needed.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/controller/prpqr_reconciler/prpqr_reconciler.gopkg/controller/prpqr_reconciler/prpqr_reconciler_test.gopkg/prowgen/podspec.gopkg/prowgen/podspec_test.go
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bear-redhat, jianzhangbjz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@jianzhangbjz This looks like an attack vector since there can be an unmerged PR in the release repo that mutates the step registry. Can you elaborate more on why merging the step registry changes first in the release repo is a problem? /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Hi @droslean, good point about the security concern! The use case is testing step registry changes (like
Which approach would you prefer? Or any suggestion? Thanks! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/prowgen/podspec_test.go (1)
533-583: Add idempotency coverage for repeatedRegistry()mutations.This test covers add/no-op, but not repeated mutation behavior. Please add a case that applies
Registry(path)multiple times and asserts only one--registry=arg remains.Proposed test extension
func TestRegistry(t *testing.T) { t.Parallel() tests := []struct { name string registryPath string wantArg string + repeat int }{ { name: "registry path is added", registryPath: "/go/src/github.com/openshift/release/ci-operator/step-registry", wantArg: "--registry=/go/src/github.com/openshift/release/ci-operator/step-registry", + repeat: 1, + }, + { + name: "registry mutator is idempotent", + registryPath: "/go/src/github.com/openshift/release/ci-operator/step-registry", + wantArg: "--registry=/go/src/github.com/openshift/release/ci-operator/step-registry", + repeat: 2, }, { name: "empty registry path is a nop", registryPath: "", wantArg: "", + repeat: 1, }, } for _, tc := range tests { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() g := NewCiOperatorPodSpecGenerator() - g.Add(Registry(tc.registryPath)) + for i := 0; i < tc.repeat; i++ { + g.Add(Registry(tc.registryPath)) + } podspec, err := g.Build() if err != nil { t.Fatalf("Unexpected error: %v", err) } + registryArgCount := 0 + for _, arg := range podspec.Containers[0].Args { + if strings.HasPrefix(arg, "--registry=") { + registryArgCount++ + } + } + if registryArgCount > 1 { + t.Errorf("Expected at most one --registry arg, got %d: %v", registryArgCount, podspec.Containers[0].Args) + } if tc.wantArg != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/podspec_test.go` around lines 533 - 583, Extend TestRegistry to include a case that applies Registry(path) more than once and asserts only one --registry= argument is present: create a subtest (within TestRegistry using NewCiOperatorPodSpecGenerator, g.Add(Registry(path)) called multiple times) call g.Build(), then iterate podspec.Containers[0].Args and count entries with strings.HasPrefix(arg, "--registry=") (or exact match for the expected "--registry="+path) and fail if count != 1; do this for a non-empty path to verify idempotency of Registry and optionally for an empty path to verify repeated no-op behavior. Ensure you reference the existing symbols NewCiOperatorPodSpecGenerator, Registry, g.Add, Build and podspec.Containers[0].Args when locating where to add the new assertion.pkg/prowgen/podspec.go (1)
757-765: Handle conflicting--registry=args deterministically.
addUniqueParameteronly avoids exact duplicates, so multipleRegistry()mutators with different paths can emit multiple--registry=flags. Prefer replacing an existing registry flag instead of appending another one.Proposed refactor
func Registry(registryPath string) PodSpecMutator { return func(spec *corev1.PodSpec) error { if registryPath == "" { return nil } container := &spec.Containers[0] - addUniqueParameter(container, fmt.Sprintf("--registry=%s", registryPath)) + wantArg := fmt.Sprintf("--registry=%s", registryPath) + for i := range container.Args { + if strings.HasPrefix(container.Args[i], "--registry=") { + container.Args[i] = wantArg + return nil + } + } + addUniqueParameter(container, wantArg) return nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/podspec.go` around lines 757 - 765, The Registry mutator currently calls addUniqueParameter which only prevents exact duplicates, so multiple Registry(...) calls with different registryPath values can leave multiple --registry= flags; update Registry (the func Registry(registryPath string) PodSpecMutator) to scan the target container (container := &spec.Containers[0]) for any existing arg that starts with "--registry=" and replace that entry with fmt.Sprintf("--registry=%s", registryPath), otherwise append the flag—do this instead of calling addUniqueParameter so subsequent Registry mutators deterministically override prior values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/prowgen/podspec_test.go`:
- Around line 533-583: Extend TestRegistry to include a case that applies
Registry(path) more than once and asserts only one --registry= argument is
present: create a subtest (within TestRegistry using
NewCiOperatorPodSpecGenerator, g.Add(Registry(path)) called multiple times) call
g.Build(), then iterate podspec.Containers[0].Args and count entries with
strings.HasPrefix(arg, "--registry=") (or exact match for the expected
"--registry="+path) and fail if count != 1; do this for a non-empty path to
verify idempotency of Registry and optionally for an empty path to verify
repeated no-op behavior. Ensure you reference the existing symbols
NewCiOperatorPodSpecGenerator, Registry, g.Add, Build and
podspec.Containers[0].Args when locating where to add the new assertion.
In `@pkg/prowgen/podspec.go`:
- Around line 757-765: The Registry mutator currently calls addUniqueParameter
which only prevents exact duplicates, so multiple Registry(...) calls with
different registryPath values can leave multiple --registry= flags; update
Registry (the func Registry(registryPath string) PodSpecMutator) to scan the
target container (container := &spec.Containers[0]) for any existing arg that
starts with "--registry=" and replace that entry with
fmt.Sprintf("--registry=%s", registryPath), otherwise append the flag—do this
instead of calling addUniqueParameter so subsequent Registry mutators
deterministically override prior values.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
pkg/controller/prpqr_reconciler/prpqr_reconciler.gopkg/controller/prpqr_reconciler/prpqr_reconciler_test.gopkg/prowgen/podspec.gopkg/prowgen/podspec_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go
- pkg/controller/prpqr_reconciler/prpqr_reconciler.go
|
@jianzhangbjz: The following tests failed, say
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. |
|
CI Failures are infrastructure issues, not code problems:
|
Testing the step registry changes is already being done by the pj-rehearse tool. Why do you need to combine the testing PR with other PRs using the payload job plugin? |
|
For example, I want to test openshift/operator-framework-olm#1236 (unmerged) together with openshift/release#75271 (step registry fix). How can I use |
4 participants
Summary
When
openshift/releaseis included in the PRs being tested via/payload-job-with-prs,configure ci-operator to use the step registry from the local checkout instead of the
remote config resolver.
Problem
When using
/payload-job-with-prswith a PR fromopenshift/releasethat modifies thestep registry, the changes were not applied because ci-operator was still loading the
step registry from
config.ci.openshift.org.Example error:
ERRO Failed to load arguments. error=[tests[...].steps.pre[3].from: unknown image "root"
(configuration is missing build_root)]
Solution
Registry()PodSpecMutator function inpkg/prowgen/podspec.gothat adds the--registryflag to ci-operatorprpqr_reconciler.go, detect whenopenshift/releaseis among the PRs and addthe
--registryflag pointing to the local checkout path(
/go/src/github.com/openshift/release/ci-operator/step-registry)Summary by CodeRabbit
New Features
Tests