Skip to content

[DPTP-5001] feat(prpqr-reconciler): inject credentials and hide jobs for openshift-priv PRs#5287

Open
smith-xyz wants to merge 7 commits into
openshift:mainfrom
smith-xyz:prpqr-openshift-priv-support
Open

[DPTP-5001] feat(prpqr-reconciler): inject credentials and hide jobs for openshift-priv PRs#5287
smith-xyz wants to merge 7 commits into
openshift:mainfrom
smith-xyz:prpqr-openshift-priv-support

Conversation

@smith-xyz

@smith-xyz smith-xyz commented Jun 30, 2026

Copy link
Copy Markdown

Adds cred injection for prpqr usage in openshift-priv repositories

This update teaches the PR payload qualification flow to recognize private repositories and handle them like CI needs to for OpenShift private repos.

In practice, payload-testing-prow-plugin now marks a PRPQR as private when any participating repository is private, and the prpqr-reconciler uses that signal to inject GitHub credentials and hide the generated ProwJobs. That applies to both regular and aggregated jobs, so private PR processing can run without exposing those jobs publicly.

The API/CRD was extended with a new spec.private flag to carry this state through the PRPQR object, and tests were added to cover private-repo behavior end to end.

Copilot AI review requested due to automatic review settings June 30, 2026 19:23
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds a Private boolean field to the PullRequestPayloadQualificationRun spec (types and CRD), sets it in the payload-testing prow plugin based on PR repo visibility, and uses it in the PRPQR reconciler to hide generated ProwJobs and inject GitHub credentials, with accompanying tests and fixtures.

Changes

Private repo propagation and hidden job handling

Layer / File(s) Summary
API field addition
pkg/api/pullrequestpayloadqualification/v1/types.go, pkg/api/pullrequestpayloadqualification/v1/ci.openshift.io_pullrequestpayloadqualificationruns.yaml
Adds Private bool field to PullRequestPayloadTestSpec and matching CRD schema property.
Plugin sets Private flag
cmd/payload-testing-prow-plugin/server.go
prpqrBuilder gains a private field set from the triggering PR and any additional PR's repo visibility; build() writes it to Spec.Private.
Plugin tests
cmd/payload-testing-prow-plugin/server_test.go
TestBuild and TestHandle add cases and assertions verifying Spec.Private is set correctly.
Reconciler hidden/credential logic
pkg/controller/prpqr_reconciler/prpqr_reconciler.go
generateProwjob/generateAggregatedProwjobs gain a private parameter that adds GitHubToken(false) and marks jobs Hidden; aggregator job hidden flag set from prpqr.Spec.Private; constants reformatted.
Reconciler tests and fixtures
pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go, pkg/controller/prpqr_reconciler/testdata/zz_fixture_*private*.yaml
New TestReconcile cases for private single and aggregated jobs, with matching ProwJob and PRPQR YAML fixtures.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Poem

A rabbit hops through repos dark and private,
Tokens tucked in, jobs kept quiet and pliant.
Hidden flags flip true from base to spec,
Fixtures stacked up, checked and correct.
Carrots for privacy, done with care! 🥕🔒

🚥 Pre-merge checks | ✅ 16 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (16 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: private PR handling with credential injection and hidden jobs for openshift-priv repositories.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Go Error Handling ✅ Passed No new ignored errors, panics, or unwrapped error paths were added; the private-flag changes preserve existing nil-check flow.
Test Coverage For New Features ✅ Passed PASS: Added table-driven tests for private PRPQR build/handle paths and reconciler private job generation, with fixture comparisons covering hidden/credential injection.
Stable And Deterministic Test Names ✅ Passed New test names are static strings like "private repo sets hidden and injects credentials"; no generated or time-varying titles were added.
Test Structure And Quality ✅ Passed PASS: The added tests are table-driven unit tests with fake clients, no waits/cluster cleanup needs, and reasonable failure messages; no Ginkgo-quality issues found.
Microshift Test Compatibility ✅ Passed The added tests are plain Go unit tests (t.Run), not Ginkgo e2e tests, and they contain no MicroShift-unsupported API usage or missing guards.
Single Node Openshift (Sno) Test Compatibility ✅ Passed Only standard Go unit tests were added; no Ginkgo e2e tests or SNO-only assumptions/skip markers were found in the changed files.
Topology-Aware Scheduling Compatibility ✅ Passed The changes only thread a private flag to add GitHub credentials and mark ProwJobs hidden; no node selectors, affinity, topology spread, or replica assumptions were added.
Ote Binary Stdout Contract ✅ Passed Touched binaries use logrus/panic only; no fmt.Print, klog, os.Stdout, or GinkgoWriter writes appear in process-level code.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Not applicable: the PR adds Go unit tests/fixtures, not new Ginkgo e2e tests, and I found no IPv4-only or external-network assumptions.
No-Weak-Crypto ✅ Passed No MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or secret/token comparisons appear in the touched code; the changes only propagate private flags and keep existing SHA-256 job hashing.
Container-Privileges ✅ Passed No touched manifest or generated pod spec adds privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN or allowPrivilegeEscalation; changes only add Hidden/GitHubToken logic.
No-Sensitive-Data-In-Logs ✅ Passed PASS: The PR only threads private flags; it adds no new log output and does not print tokens, passwords, or PII.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot requested review from deepsm007 and droslean June 30, 2026 19:24
@openshift-ci

openshift-ci Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smith-xyz
Once this PR has been reviewed and has the lgtm label, please assign hector-vido for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds special handling for PRPQR runs targeting openshift-priv repositories so that resulting ProwJobs are hidden in Deck and have GitHub credentials injected for private repo access, plus a config-resolution fallback in the payload-testing prow plugin.

Changes:

  • Hide PRPQR-generated ProwJobs (including aggregator jobs) when any PR under test is in openshift-priv.
  • Inject GitHub OAuth token secret + --oauth-token-path into ProwJobs for openshift-priv PRs.
  • Add a new PRPQR reconciler test case + fixtures for the openshift-priv behavior; add openshift-privopenshift ci-operator config fallback in the payload-testing plugin.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/controller/prpqr_reconciler/prpqr_reconciler.go Implements openshift-priv detection; sets Hidden and injects GitHub token/decoration config.
pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go Adds a reconciler test case for openshift-priv PR behavior.
pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml Golden fixture for expected PRPQR object state for openshift-priv.
pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml Golden fixture for expected ProwJob output (hidden + oauth secret + token arg).
cmd/payload-testing-prow-plugin/server.go Adds fallback to resolve ci-operator config from openshift when org is openshift-priv and config lookup fails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/payload-testing-prow-plugin/server.go Outdated
Comment thread cmd/payload-testing-prow-plugin/server.go Outdated
Comment thread pkg/controller/prpqr_reconciler/prpqr_reconciler.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go (1)

259-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the aggregated private-PR path too.

This case only exercises the non-aggregated generateProwjob flow, so the new aggregatorJob.Spec.Hidden = true branch in pkg/controller/prpqr_reconciler/prpqr_reconciler.go still has no regression coverage. Please add an AggregatedCount > 0 case and assert that the generated aggregator ProwJob is hidden.

🤖 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/controller/prpqr_reconciler/prpqr_reconciler_test.go` around lines 259 -
273, Add regression coverage for the aggregated private-PR path in the
prpqr_reconciler_test case named “openshift-priv PR sets hidden and injects
credentials”: this scenario currently only exercises generateProwjob, so it does
not validate the aggregator path in prpqr_reconciler.go. Extend the test with an
AggregatedCount > 0 setup and assert that the generated aggregator ProwJob has
Spec.Hidden set to true, using the existing prpqr reconciler flow and aggregator
job creation symbols to locate the right branch.
cmd/payload-testing-prow-plugin/server.go (1)

431-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression test for the openshift-priv fallback path.

This new branch changes behavior in a CLI entrypoint, but there’s no accompanying server_test.go coverage in this PR context. A small table-driven case that makes the first lookup fail and asserts the retry against openshift would keep this from regressing silently. As per path instructions, “Prefer table-driven tests.”

🤖 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 `@cmd/payload-testing-prow-plugin/server.go` around lines 431 - 435, Add a
regression test in server_test.go for the openshift-priv fallback path: make the
initial s.ciOpConfigResolver.Config call fail when Metadata.Org is
openshift-priv, then assert the code retries with Org set to openshift and logs
the fallback behavior. Use a small table-driven case around the
ciOpConfigResolver.Config call and the openshift-priv branch in server.go so
this retry path is covered and won’t regress silently.

Source: Path instructions

🤖 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.

Nitpick comments:
In `@cmd/payload-testing-prow-plugin/server.go`:
- Around line 431-435: Add a regression test in server_test.go for the
openshift-priv fallback path: make the initial s.ciOpConfigResolver.Config call
fail when Metadata.Org is openshift-priv, then assert the code retries with Org
set to openshift and logs the fallback behavior. Use a small table-driven case
around the ciOpConfigResolver.Config call and the openshift-priv branch in
server.go so this retry path is covered and won’t regress silently.

In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go`:
- Around line 259-273: Add regression coverage for the aggregated private-PR
path in the prpqr_reconciler_test case named “openshift-priv PR sets hidden and
injects credentials”: this scenario currently only exercises generateProwjob, so
it does not validate the aggregator path in prpqr_reconciler.go. Extend the test
with an AggregatedCount > 0 setup and assert that the generated aggregator
ProwJob has Spec.Hidden set to true, using the existing prpqr reconciler flow
and aggregator job creation symbols to locate the right branch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 4a966847-2d60-4348-b460-8ee869d37867

📥 Commits

Reviewing files that changed from the base of the PR and between 803b3f9 and 0467180.

📒 Files selected for processing (5)
  • cmd/payload-testing-prow-plugin/server.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift/release (manual)
  • openshift/ci-docs (manual)
  • openshift/release-controller (manual)
  • openshift/ci-chat-bot (manual)

Copilot AI review requested due to automatic review settings July 1, 2026 00:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comment thread pkg/controller/prpqr_reconciler/prpqr_reconciler.go Outdated
Comment thread pkg/controller/prpqr_reconciler/prpqr_reconciler.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
cmd/payload-testing-prow-plugin/server.go (2)

888-895: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Function takes 5 parameters.

resolveCIOpConfig(resolver ciOpConfigResolver, org, repo, branch string, logger *logrus.Entry) exceeds the 3-4 parameter guidance; consider grouping org, repo, branch into api.Metadata directly (which is what's constructed internally anyway) to shrink the signature.

As per path instructions, "Keep function signatures small — if a function takes more than 3-4 parameters, consider grouping them into an options struct."

🤖 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 `@cmd/payload-testing-prow-plugin/server.go` around lines 888 - 895, The
resolveCIOpConfig function has too many parameters; replace the separate org,
repo, and branch arguments with a single api.Metadata input so the signature
stays small. Update the fallback logic inside resolveCIOpConfig to read from
that metadata object when calling resolver.Config, and adjust any callers
accordingly so the logger and ciOpConfigResolver usage remain unchanged.

Source: Path instructions


882-895: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Fragile 404 detection via error-string substring match.

isConfigResolverNotFound matches on err.Error() containing "got unexpected http 404 status code from configresolver", a literal string owned by pkg/registry/server/client.go. Any wording/format change there (or wrapping that reformats the message) silently breaks the openshift-priv fallback with no compile-time signal — the code will just stop falling back, and no test in a different package would catch it. Idiomatic Go favors a sentinel/typed error exposed by the configresolver client that callers can check with errors.Is/errors.As instead of string matching.

♻️ Suggested direction
-func isConfigResolverNotFound(err error) bool {
-	return err != nil && strings.Contains(err.Error(), "got unexpected http 404 status code from configresolver")
-}
+// isConfigResolverNotFound reports whether err represents a 404 from the
+// configresolver client. Prefer errors.Is/As against a sentinel exported by
+// pkg/registry/server/client.go if one exists, rather than matching on the
+// error message text.

Please confirm whether pkg/registry/server/client.go (or another shared package) already exposes a typed/sentinel error for a 404 response that this could use instead of string matching.

🤖 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 `@cmd/payload-testing-prow-plugin/server.go` around lines 882 - 895, The 404
fallback in resolveCIOpConfig is relying on a fragile err.Error() substring
match in isConfigResolverNotFound, so switch it to a shared sentinel or typed
error from the configresolver client if one already exists. Check
pkg/registry/server/client.go (or another common package) for an exported 404
error and update resolveCIOpConfig to use errors.Is or errors.As instead of
string matching. If no shared error exists, add one at the client boundary and
make the fallback path in isConfigResolverNotFound consume that exported error.
cmd/payload-testing-prow-plugin/server_test.go (1)

1354-1370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate test double alongside fakeCIOpConfigResolver.

recordingCIOpConfigResolver.Config largely repeats fakeCIOpConfigResolver.Config (lines 1342-1352) — same "openshift org → promoted config, else empty config" branching — just adding call recording and per-org error injection. Consider extending fakeCIOpConfigResolver with these fields instead of maintaining two near-identical fakes.

🤖 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 `@cmd/payload-testing-prow-plugin/server_test.go` around lines 1354 - 1370, The
test double in recordingCIOpConfigResolver duplicates the same Config behavior
already implemented by fakeCIOpConfigResolver, so refactor the existing fake to
support call recording and per-org error injection instead of keeping two
near-identical resolvers. Update the Config method on fakeCIOpConfigResolver to
preserve its current openshift-vs-empty config behavior while also tracking
requested metadata and returning org-specific errors, then remove the redundant
recordingCIOpConfigResolver implementation and switch the affected tests to use
the enhanced fake.
🤖 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.

Nitpick comments:
In `@cmd/payload-testing-prow-plugin/server_test.go`:
- Around line 1354-1370: The test double in recordingCIOpConfigResolver
duplicates the same Config behavior already implemented by
fakeCIOpConfigResolver, so refactor the existing fake to support call recording
and per-org error injection instead of keeping two near-identical resolvers.
Update the Config method on fakeCIOpConfigResolver to preserve its current
openshift-vs-empty config behavior while also tracking requested metadata and
returning org-specific errors, then remove the redundant
recordingCIOpConfigResolver implementation and switch the affected tests to use
the enhanced fake.

In `@cmd/payload-testing-prow-plugin/server.go`:
- Around line 888-895: The resolveCIOpConfig function has too many parameters;
replace the separate org, repo, and branch arguments with a single api.Metadata
input so the signature stays small. Update the fallback logic inside
resolveCIOpConfig to read from that metadata object when calling
resolver.Config, and adjust any callers accordingly so the logger and
ciOpConfigResolver usage remain unchanged.
- Around line 882-895: The 404 fallback in resolveCIOpConfig is relying on a
fragile err.Error() substring match in isConfigResolverNotFound, so switch it to
a shared sentinel or typed error from the configresolver client if one already
exists. Check pkg/registry/server/client.go (or another common package) for an
exported 404 error and update resolveCIOpConfig to use errors.Is or errors.As
instead of string matching. If no shared error exists, add one at the client
boundary and make the fallback path in isConfigResolverNotFound consume that
exported error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 132de267-7816-4d97-8fb8-b783e5c167ab

📥 Commits

Reviewing files that changed from the base of the PR and between 0467180 and 6353e45.

📒 Files selected for processing (6)
  • cmd/payload-testing-prow-plugin/server.go
  • cmd/payload-testing-prow-plugin/server_test.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_aggregated_PR_sets_aggregator_hidden.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_openshift_priv_aggregated_PR_sets_aggregator_hidden.yaml
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift/release (manual)
  • openshift/ci-docs (manual)
  • openshift/release-controller (manual)
  • openshift/ci-chat-bot (manual)
✅ Files skipped from review due to trivial changes (1)
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_openshift_priv_aggregated_PR_sets_aggregator_hidden.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/controller/prpqr_reconciler/prpqr_reconciler.go


func resolveCIOpConfig(resolver ciOpConfigResolver, org, repo, branch string, logger *logrus.Entry) (*api.ReleaseBuildConfiguration, error) {
ciOpConfig, err := resolver.Config(&api.Metadata{Org: org, Repo: repo, Branch: branch})
if err != nil && org == openShiftPrivOrg && isConfigResolverNotFound(err) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how the resolution and the fallback is related to what you need to introduce in this PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah looking at this again I was missing some bigger context where metadata already existed, so all these fallbacks aren't necessary. Reshaped the PR with a cleaner approach, I hope

func resolveCIOpConfig(resolver ciOpConfigResolver, org, repo, branch string, logger *logrus.Entry) (*api.ReleaseBuildConfiguration, error) {
ciOpConfig, err := resolver.Config(&api.Metadata{Org: org, Repo: repo, Branch: branch})
if err != nil && org == openShiftPrivOrg && isConfigResolverNotFound(err) {
logger.WithField("fallback", "openshift").Info("falling back to openshift org config for openshift-priv repo")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback is wrong. There are dedicated configuration for the openshift private. Consider them as a different repos.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above - same issues

@droslean

droslean commented Jul 1, 2026

Copy link
Copy Markdown
Member

I don't think that this is the right approach and it seems like a hack/workaround. The plugin should already know if the repo is private or not, which is not directly related to openshift private. Also the decoration config which needs to be different on private repos, is only related to Prow and not ci-operator.

Copilot AI review requested due to automatic review settings July 1, 2026 14:01

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go (1)

586-588: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider exact org match instead of substring.

strings.Contains(base.Org, "openshift-priv") would also match orgs merely containing that substring (e.g. "not-openshift-priv"). Since this only fakes resolver behavior for tests, low risk, but an exact equality/prefix check is more precise and matches how openshift-priv org detection is generally understood.

♻️ Proposed tweak
-	if strings.Contains(base.Org, "openshift-priv") {
+	if base.Org == "openshift-priv" {
 		cfg.Prowgen = &api.ProwgenOverrides{Private: true}
 	}
🤖 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/controller/prpqr_reconciler/prpqr_reconciler_test.go` around lines 586 -
588, The private-org test setup in prpqr_reconciler_test.go uses a substring
check that can match unrelated org names, so tighten the condition in the test
helper around base.Org and cfg.Prowgen. Replace the
strings.Contains("openshift-priv") check with an exact org match or a stricter
prefix/equality check that only marks the intended openshift-priv org as
private.
🤖 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/controller/prpqr_reconciler/prpqr_reconciler.go`:
- Around line 328-330: The aggregator job hiding logic in PRPQR reconciliation
is missing the same private/expose condition used by prowgen job generation,
causing inconsistent visibility for configs that set both flags. Update the
aggregator job path in the PRPQR reconciler to mirror the `jobbase.go` contract
by checking both `Prowgen.Private` and `Prowgen.Expose` before setting
`aggregatorJob.Spec.Hidden`, using the same `ciopConfig.Prowgen` fields and
related aggregator job setup code.

---

Nitpick comments:
In `@pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go`:
- Around line 586-588: The private-org test setup in prpqr_reconciler_test.go
uses a substring check that can match unrelated org names, so tighten the
condition in the test helper around base.Org and cfg.Prowgen. Replace the
strings.Contains("openshift-priv") check with an exact org match or a stricter
prefix/equality check that only marks the intended openshift-priv org as
private.
🪄 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: 0e21a89a-55a6-4307-9388-2468a6edb85a

📥 Commits

Reviewing files that changed from the base of the PR and between 6353e45 and 7fb1146.

📒 Files selected for processing (5)
  • pkg/controller/prpqr_reconciler/prpqr_reconciler.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_aggregated_PR_sets_aggregator_hidden.yaml
  • pkg/registry/server/server.go
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift/release (manual)
  • openshift/ci-docs (manual)
  • openshift/release-controller (manual)
  • openshift/ci-chat-bot (manual)
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_PR_sets_hidden_and_injects_credentials.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_openshift_priv_aggregated_PR_sets_aggregator_hidden.yaml

Comment thread pkg/controller/prpqr_reconciler/prpqr_reconciler.go Outdated
@smith-xyz smith-xyz requested a review from droslean July 1, 2026 14:48
}
continue
}
if ciopConfig.Prowgen != nil && ciopConfig.Prowgen.Private && !ciopConfig.Prowgen.Expose {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prowgen stanza in the ci-operator config is no being used anywhere but only from prowgen. The controller doesn't care about it. If you make the check here, which makes sense, we will just have another dependency of a configuration that is being used by a specific tool only. When the plugin receives the github comment event, it should know if its private or not. Perhaps it would be better to pass that information through the prpqr resource.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cmd/payload-testing-prow-plugin/server_test.go (1)

674-756: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Missing test coverage for additional-PR privacy detection.

Tests cover privacy detection via the primary triggering PR, but the branch that sets builder.private = true from an additional PR's repo visibility (server.go lines 557-559) has no test exercising it — e.g., a /payload-with-prs//payload-aggregate-with-prs case where the primary repo is public but an additional PR (like the existing privatePR at id 456) is private. Since this flag drives credential injection and job hiding downstream, it's worth covering explicitly.

🤖 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 `@cmd/payload-testing-prow-plugin/server_test.go` around lines 674 - 756, Add a
test in TestHandle that exercises the additional-PR privacy path in
server.Handle: use a public primary PR plus an extra PR entry with a private
repo (the existing privatePR in the fakegithub client), then trigger a
/payload-with-prs or /payload-aggregate-with-prs request so the code in
server.go that sets builder.private from additional PRs is hit. Assert the
resulting behavior/flag reflects private=true, ensuring the additional PR
visibility branch is covered.
🤖 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.

Nitpick comments:
In `@cmd/payload-testing-prow-plugin/server_test.go`:
- Around line 674-756: Add a test in TestHandle that exercises the additional-PR
privacy path in server.Handle: use a public primary PR plus an extra PR entry
with a private repo (the existing privatePR in the fakegithub client), then
trigger a /payload-with-prs or /payload-aggregate-with-prs request so the code
in server.go that sets builder.private from additional PRs is hit. Assert the
resulting behavior/flag reflects private=true, ensuring the additional PR
visibility branch is covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 0de85a7f-8bb0-427b-8a49-8a947d4475a6

📥 Commits

Reviewing files that changed from the base of the PR and between 7fb1146 and 9f6d2db.

📒 Files selected for processing (10)
  • cmd/payload-testing-prow-plugin/server.go
  • cmd/payload-testing-prow-plugin/server_test.go
  • pkg/api/pullrequestpayloadqualification/v1/ci.openshift.io_pullrequestpayloadqualificationruns.yaml
  • pkg/api/pullrequestpayloadqualification/v1/types.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler.go
  • pkg/controller/prpqr_reconciler/prpqr_reconciler_test.go
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_private_repo_aggregated_sets_aggregator_hidden.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_private_repo_sets_hidden_and_injects_credentials.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_private_repo_aggregated_sets_aggregator_hidden.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_private_repo_sets_hidden_and_injects_credentials.yaml
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • openshift/release (manual)
  • openshift/ci-docs (manual)
  • openshift/release-controller (manual)
  • openshift/ci-chat-bot (manual)
💤 Files with no reviewable changes (2)
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_private_repo_sets_hidden_and_injects_credentials.yaml
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prowjobs_TestReconcile_private_repo_aggregated_sets_aggregator_hidden.yaml
✅ Files skipped from review due to trivial changes (1)
  • pkg/controller/prpqr_reconciler/testdata/zz_fixture_prpqr_TestReconcile_private_repo_aggregated_sets_aggregator_hidden.yaml

@openshift-ci

openshift-ci Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

@smith-xyz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/integration 9f6d2db link true /test integration

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants