enable in-repo ci-op config#5212
Draft
Prucek wants to merge 1 commit into
Draft
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughAdds an in-repo-config Prow webhook plugin (commands: /onboard, /new-test) with bootstrap job generation and ephemeral presubmit creation; extends prowgen to accept single-file inputs, propagate an unresolved-config path into job PodSpecs, and add utilities to write per-branch job shards. ChangesIn-repo-config Prow plugin
prowgen single-file and job utilities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Prucek
changed the title
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek 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
Prucek
force-pushed
the
in-repo-config-plugin
branch
from
d862c2b to
2e3de1a
Compare
Prucek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
images/in-repo-config-plugin/Dockerfile (1)
1-3: ⚡ Quick winAdd HEALTHCHECK for operational monitoring.
The Dockerfile lacks a HEALTHCHECK directive. Since this is a webhook server, adding a health endpoint check improves container orchestration and monitoring.
As per coding guidelines: "HEALTHCHECK defined".
💚 Proposed addition
FROM quay.io/centos/centos:stream8 ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin +HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ + CMD ["/usr/bin/in-repo-config-plugin", "--health-check"] ENTRYPOINT ["/usr/bin/in-repo-config-plugin"]Note: Verify whether the binary supports a
--health-checkflag or similar. If the webhook server exposes an HTTP health endpoint (common in Prow plugins), use a curl-based check instead:HEALTHCHECK --interval=30s --timeout=3s CMD curl -f http://localhost:8888/healthz || exit 1🤖 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 `@images/in-repo-config-plugin/Dockerfile` around lines 1 - 3, The Dockerfile for the in-repo-config-plugin container has no HEALTHCHECK; add a HEALTHCHECK directive so orchestration can monitor the webhook process started by ENTRYPOINT (binary in-repo-config-plugin). If the binary supports a health flag (e.g., --health-check or --health-port) invoke it via the HEALTHCHECK CMD, otherwise use an HTTP probe such as curling the plugin’s health endpoint (for example http://localhost:8888/healthz) with sensible --interval and --timeout settings and a non-zero exit on failure; place this HEALTHCHECK line after the ENTRYPOINT so the liveness probe validates the actual running process.cmd/in-repo-config-plugin/bootstrap.go (1)
81-82: ⚡ Quick winUse canonical in-repo config filename constant
At Line 81,
".ci-operator.yaml"is hardcoded. Reusecioperatorapi.CIOperatorInrepoConfigFileNameto avoid drift between bootstrap and/new-testbehavior.Suggested diff
+ cioperatorapi "github.com/openshift/ci-tools/pkg/api" ... - fmt.Sprintf("--from-file=%s/.ci-operator.yaml", repoPath), + fmt.Sprintf("--from-file=%s/%s", repoPath, cioperatorapi.CIOperatorInrepoConfigFileName),🤖 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/in-repo-config-plugin/bootstrap.go` around lines 81 - 82, The hardcoded filename ".ci-operator.yaml" should be replaced with the canonical constant to avoid drift; update the fmt.Sprintf call that builds the from-file path (the expression currently using fmt.Sprintf("--from-file=%s/.ci-operator.yaml", repoPath)) to use cioperatorapi.CIOperatorInrepoConfigFileName instead (e.g., fmt.Sprintf("--from-file=%s/%s", repoPath, cioperatorapi.CIOperatorInrepoConfigFileName)), ensuring you import the cioperatorapi package if not already imported.
🤖 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 `@cmd/in-repo-config-plugin/server_test.go`:
- Around line 324-415: Add a regression test case in
TestHandleIssueCommentDispatch to assert the `/new-testfoo` edge-case is
ignored: inside the testCases slice add an entry with name like "ignores
/new-testfoo suffix", body "/new-testfoo", action
github.IssueCommentActionCreated, isPR true, and expectComments 0 so that
handleIssueComment (and the fakeGithubClient used in the test) will fail the
test if the `/new-test` matcher wrongly triggers on `/new-testfoo`.
In `@cmd/in-repo-config-plugin/server.go`:
- Around line 122-123: The CreateComment calls on s.ghc
(s.ghc.CreateComment(...)) are ignoring returned errors; update each call
(including the other occurrences you flagged) to capture the error return and
log it (e.g., err := s.ghc.CreateComment(...); if err != nil {
s.logger.Errorf("CreateComment failed for %s/%s#%d: %v", org, repo, number, err)
}) or create a small helper like createCommentAndLog(org, repo, number, body)
that calls s.ghc.CreateComment, checks the error, and logs a descriptive
message; replace the raw calls with that helper to ensure consistent error
handling and logging across server.go.
- Around line 280-289: The success message is built from jobNames (which
includes Presubmits, Postsubmits, and Periodics) but the creation loop only
creates presubmit ProwJobs, causing misleading “ProwJobs created” messages when
nothing was created; update the code that builds the success message to use the
actual created list rather than jobNames (e.g., use the slice/variable you
append created presubmit names to in the creation loop, or filter jobNames to
only the presubmits from allJobs.PresubmitsStatic[orgrepo]) so the message only
reports jobs that were truly created; make the same fix for the corresponding
block covering the other region noted (around the other success message).
- Around line 99-100: The current command match using strings.HasPrefix(body,
newTestPrefix) is too loose and will match inputs like "/new-testfoo"; update
the match in the switch/case around handleNewTest (referencing newTestPrefix and
s.handleNewTest) to only trigger when body == "/new-test" or when body starts
with "/new-test " (slash-new-test followed by a space) so arguments remain
supported but accidental concatenations are rejected; implement the conditional
check accordingly and keep calling s.handleNewTest(l, ic) only when that
stricter match passes.
In `@images/in-repo-config-plugin/Dockerfile`:
- Line 2: Replace the Dockerfile ADD instruction with COPY to avoid ADD's extra
behavior; change the line that currently uses "ADD in-repo-config-plugin
/usr/bin/in-repo-config-plugin" to use "COPY" so the file in-repo-config-plugin
is explicitly copied into /usr/bin/in-repo-config-plugin without unintended
extraction or URL fetching.
- Around line 1-3: The Dockerfile currently runs the plugin as root; add a USER
directive after adding the binary and before ENTRYPOINT to switch to a non-root
UID:GID that matches the EFS and git-sync mounts (e.g., USER 1000:1000 or USER
65532:65532). Update the Dockerfile around the ADD in-repo-config-plugin and
ENTRYPOINT ["/usr/bin/in-repo-config-plugin"] lines to include the chosen USER
and ensure file ownership/permissions on /usr/bin/in-repo-config-plugin and any
expected runtime dirs (like --job-config-dir and --release-repo-dir) are set to
that UID:GID so the container can read/write those mounts.
- Line 1: Replace the non-compliant base image on the Dockerfile's FROM line
(currently "quay.io/centos/centos:stream8") with a UBI minimal or distroless
image sourced from catalog.redhat.com (use the appropriate image name/tag
provided by Red Hat); if you must keep a non-Red Hat base, pin it by digest
(e.g., change "quay.io/centos/centos:stream8" to
"quay.io/centos/centos@sha256:..."). Update the FROM instruction accordingly and
verify the new base meets the project's security rules.
---
Nitpick comments:
In `@cmd/in-repo-config-plugin/bootstrap.go`:
- Around line 81-82: The hardcoded filename ".ci-operator.yaml" should be
replaced with the canonical constant to avoid drift; update the fmt.Sprintf call
that builds the from-file path (the expression currently using
fmt.Sprintf("--from-file=%s/.ci-operator.yaml", repoPath)) to use
cioperatorapi.CIOperatorInrepoConfigFileName instead (e.g.,
fmt.Sprintf("--from-file=%s/%s", repoPath,
cioperatorapi.CIOperatorInrepoConfigFileName)), ensuring you import the
cioperatorapi package if not already imported.
In `@images/in-repo-config-plugin/Dockerfile`:
- Around line 1-3: The Dockerfile for the in-repo-config-plugin container has no
HEALTHCHECK; add a HEALTHCHECK directive so orchestration can monitor the
webhook process started by ENTRYPOINT (binary in-repo-config-plugin). If the
binary supports a health flag (e.g., --health-check or --health-port) invoke it
via the HEALTHCHECK CMD, otherwise use an HTTP probe such as curling the
plugin’s health endpoint (for example http://localhost:8888/healthz) with
sensible --interval and --timeout settings and a non-zero exit on failure; place
this HEALTHCHECK line after the ENTRYPOINT so the liveness probe validates the
actual running process.
🪄 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: 3792b6e7-6ac0-4af1-a70e-c6449770ca46
📒 Files selected for processing (6)
cmd/in-repo-config-plugin/bootstrap.gocmd/in-repo-config-plugin/bootstrap_test.gocmd/in-repo-config-plugin/main.gocmd/in-repo-config-plugin/server.gocmd/in-repo-config-plugin/server_test.goimages/in-repo-config-plugin/Dockerfile
Comment on lines
+324
to
+415
| func TestHandleIssueCommentDispatch(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| body string | ||
| action github.IssueCommentEventAction | ||
| isPR bool | ||
| expectComments int | ||
| }{ | ||
| { | ||
| name: "dispatches /onboard", | ||
| body: "/onboard", | ||
| action: github.IssueCommentActionCreated, | ||
| isPR: true, | ||
| expectComments: 1, | ||
| }, | ||
| { | ||
| name: "dispatches /new-test", | ||
| body: "/new-test e2e", | ||
| action: github.IssueCommentActionCreated, | ||
| isPR: true, | ||
| expectComments: 1, | ||
| }, | ||
| { | ||
| name: "ignores non-created actions", | ||
| body: "/onboard", | ||
| action: github.IssueCommentActionDeleted, | ||
| isPR: true, | ||
| expectComments: 0, | ||
| }, | ||
| { | ||
| name: "ignores non-PR issues", | ||
| body: "/onboard", | ||
| action: github.IssueCommentActionCreated, | ||
| isPR: false, | ||
| expectComments: 0, | ||
| }, | ||
| { | ||
| name: "ignores unrelated comments", | ||
| body: "LGTM", | ||
| action: github.IssueCommentActionCreated, | ||
| isPR: true, | ||
| expectComments: 0, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| ghc := &fakeGithubClient{ | ||
| prs: map[string]*github.PullRequest{ | ||
| "org/repo#1": makePR("org", "repo", 1, "main", "abc1234567890"), | ||
| }, | ||
| dirs: map[string][]github.DirectoryContent{}, | ||
| files: map[string][]byte{}, | ||
| } | ||
|
|
||
| pjc := fakectrlruntimeclient.NewClientBuilder().WithScheme(pjScheme()).Build() | ||
|
|
||
| s := &server{ | ||
| ghc: ghc, | ||
| trustedChecker: &fakeTrustedChecker{trusted: true}, | ||
| pjclient: pjc, | ||
| namespace: "test-ns", | ||
| jobConfigDir: tmpDir, | ||
| releaseRepoDir: tmpDir, | ||
| prowgenImage: "img", | ||
| checkconfigImage: "img", | ||
| } | ||
|
|
||
| ic := github.IssueCommentEvent{ | ||
| Action: tc.action, | ||
| Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"}, | ||
| Issue: github.Issue{ | ||
| Number: 1, | ||
| }, | ||
| Comment: github.IssueComment{ | ||
| Body: tc.body, | ||
| User: github.User{Login: "testuser"}, | ||
| }, | ||
| } | ||
| if tc.isPR { | ||
| ic.Issue.PullRequest = &struct{}{} | ||
| } | ||
|
|
||
| s.handleIssueComment(logrus.NewEntry(logrus.StandardLogger()), ic) | ||
|
|
||
| if len(ghc.comments) != tc.expectComments { | ||
| t.Errorf("expected %d comments, got %d: %+v", tc.expectComments, len(ghc.comments), ghc.comments) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add dispatch regression test for /new-test prefix edge case
Please add a case asserting /new-testfoo is ignored. This prevents accidental command triggering after the matcher fix in handleIssueComment.
As per coding guidelines, "New or modified functionality should include test coverage. Bug fixes should include a regression test that fails without the fix."
🤖 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/in-repo-config-plugin/server_test.go` around lines 324 - 415, Add a
regression test case in TestHandleIssueCommentDispatch to assert the
`/new-testfoo` edge-case is ignored: inside the testCases slice add an entry
with name like "ignores /new-testfoo suffix", body "/new-testfoo", action
github.IssueCommentActionCreated, isPR true, and expectComments 0 so that
handleIssueComment (and the fakeGithubClient used in the test) will fail the
test if the `/new-test` matcher wrongly triggers on `/new-testfoo`.
Comment on lines
+99
to
+100
| case strings.HasPrefix(body, newTestPrefix): | ||
| s.handleNewTest(l, ic) |
There was a problem hiding this comment.
Tighten /new-test command matching
At Line 99, HasPrefix("/new-test") also matches unintended inputs (e.g. /new-testfoo). Gate on exact command or /new-test with args.
Suggested diff
- case strings.HasPrefix(body, newTestPrefix):
+ case body == newTestPrefix || strings.HasPrefix(body, newTestPrefix+" "):
s.handleNewTest(l, ic)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case strings.HasPrefix(body, newTestPrefix): | |
| s.handleNewTest(l, ic) | |
| case body == newTestPrefix || strings.HasPrefix(body, newTestPrefix+" "): | |
| s.handleNewTest(l, ic) |
🤖 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/in-repo-config-plugin/server.go` around lines 99 - 100, The current
command match using strings.HasPrefix(body, newTestPrefix) is too loose and will
match inputs like "/new-testfoo"; update the match in the switch/case around
handleNewTest (referencing newTestPrefix and s.handleNewTest) to only trigger
when body == "/new-test" or when body starts with "/new-test " (slash-new-test
followed by a space) so arguments remain supported but accidental concatenations
are rejected; implement the conditional check accordingly and keep calling
s.handleNewTest(l, ic) only when that stricter match passes.
Comment on lines
+122
to
+123
| s.ghc.CreateComment(org, repo, number, fmt.Sprintf("@%s: you are not trusted to use `/onboard`.", user)) | ||
| return |
There was a problem hiding this comment.
Handle GitHub comment creation errors
These CreateComment(...) calls ignore returned errors, so user-facing status can be silently dropped. At minimum, log failures consistently (or route through a helper that logs on error).
As per coding guidelines, "Errors should be handled correctly - determine whether to ignore, log, wrap and raise up; use informative error messages".
Also applies to: 135-136, 141-142, 168-169, 190-191, 210-211, 292-293, 323-324
🤖 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/in-repo-config-plugin/server.go` around lines 122 - 123, The
CreateComment calls on s.ghc (s.ghc.CreateComment(...)) are ignoring returned
errors; update each call (including the other occurrences you flagged) to
capture the error return and log it (e.g., err := s.ghc.CreateComment(...); if
err != nil { s.logger.Errorf("CreateComment failed for %s/%s#%d: %v", org, repo,
number, err) }) or create a small helper like createCommentAndLog(org, repo,
number, body) that calls s.ghc.CreateComment, checks the error, and logs a
descriptive message; replace the raw calls with that helper to ensure consistent
error handling and logging across server.go.
| @@ -0,0 +1,3 @@ | |||
| FROM quay.io/centos/centos:stream8 | |||
There was a problem hiding this comment.
Base image must be UBI minimal or distroless, pinned by digest.
The Dockerfile uses quay.io/centos/centos:stream8, which violates two container security requirements:
- Base image must be UBI minimal or distroless from
catalog.redhat.com(not CentOS Stream) - Non-Red Hat images must be pinned by digest, not floating tags
As per coding guidelines: "Base image: UBI minimal or distroless from catalog.redhat.com" and "non-RH images: pin by digest".
🔒 Proposed fix
-FROM quay.io/centos/centos:stream8
+FROM registry.access.redhat.com/ubi8/ubi-minimal:latestNote: Red Hat images can use floating tags since Red Hat manages updates. If a non-RH base is required, pin by digest:
FROM quay.io/centos/centos@sha256:...🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@images/in-repo-config-plugin/Dockerfile` at line 1, Replace the non-compliant
base image on the Dockerfile's FROM line (currently
"quay.io/centos/centos:stream8") with a UBI minimal or distroless image sourced
from catalog.redhat.com (use the appropriate image name/tag provided by Red
Hat); if you must keep a non-Red Hat base, pin it by digest (e.g., change
"quay.io/centos/centos:stream8" to "quay.io/centos/centos@sha256:..."). Update
the FROM instruction accordingly and verify the new base meets the project's
security rules.
Comment on lines
+1
to
+3
| FROM quay.io/centos/centos:stream8 | ||
| ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin | ||
| ENTRYPOINT ["/usr/bin/in-repo-config-plugin"] |
There was a problem hiding this comment.
Add USER directive to run as non-root.
The container runs as root by default, violating the security requirement that containers must run as non-root. The plugin writes to EFS-mounted --job-config-dir and reads from the git-sync'd --release-repo-dir, so the USER directive must align with the UID/GID of those volume mounts.
As per coding guidelines: "USER non-root; never run as root".
🛡️ Proposed fix
FROM quay.io/centos/centos:stream8
ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin
+USER 1000:1000
ENTRYPOINT ["/usr/bin/in-repo-config-plugin"]Note: Verify the UID/GID (1000:1000 is an example) matches the permissions on the EFS and git-sync volume mounts in the deployment manifest. Common choices are 65532:65532 (nonroot) or a specific service account UID.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM quay.io/centos/centos:stream8 | |
| ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin | |
| ENTRYPOINT ["/usr/bin/in-repo-config-plugin"] | |
| FROM quay.io/centos/centos:stream8 | |
| ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin | |
| USER 1000:1000 | |
| ENTRYPOINT ["/usr/bin/in-repo-config-plugin"] |
🧰 Tools
🪛 Hadolint (2.14.0)
[error] 2-2: Use COPY instead of ADD for files and folders
(DL3020)
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 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 `@images/in-repo-config-plugin/Dockerfile` around lines 1 - 3, The Dockerfile
currently runs the plugin as root; add a USER directive after adding the binary
and before ENTRYPOINT to switch to a non-root UID:GID that matches the EFS and
git-sync mounts (e.g., USER 1000:1000 or USER 65532:65532). Update the
Dockerfile around the ADD in-repo-config-plugin and ENTRYPOINT
["/usr/bin/in-repo-config-plugin"] lines to include the chosen USER and ensure
file ownership/permissions on /usr/bin/in-repo-config-plugin and any expected
runtime dirs (like --job-config-dir and --release-repo-dir) are set to that
UID:GID so the container can read/write those mounts.
| @@ -0,0 +1,3 @@ | |||
| FROM quay.io/centos/centos:stream8 | |||
| ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin | |||
There was a problem hiding this comment.
Use COPY instead of ADD for files.
ADD has additional features (auto-extraction, URL fetching) that aren't needed for simple file copying. COPY is more explicit and preferred.
As per coding guidelines: "COPY specific files, not entire context".
📝 Proposed fix
-ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin
+COPY in-repo-config-plugin /usr/bin/in-repo-config-plugin🧰 Tools
🪛 Hadolint (2.14.0)
[error] 2-2: Use COPY instead of ADD for files and folders
(DL3020)
🤖 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 `@images/in-repo-config-plugin/Dockerfile` at line 2, Replace the Dockerfile
ADD instruction with COPY to avoid ADD's extra behavior; change the line that
currently uses "ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin" to use
"COPY" so the file in-repo-config-plugin is explicitly copied into
/usr/bin/in-repo-config-plugin without unintended extraction or URL fetching.
Add a Prow external plugin (`in-repo-config-plugin`) that enables repos to manage CI configuration in-repo via `.ci-operator.yaml` files instead of the centralized openshift/release repository. The plugin handles two slash commands: - `/onboard`: bootstraps a repo with config-checker presubmit and prowgen postsubmit jobs written to EFS - `/new-test`: generates ProwJob CRs directly from the PR's `.ci-operator/` configs for new tests not yet on EFS, with `--unresolved-config` so ci-operator uses the in-repo config Also adds `--from-file` mode to ci-operator-prowgen for generating jobs from a single ci-operator config file, `UnresolvedConfig` podspec mutator, and `WriteBranchToDir` for branch-scoped job writing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
in-repo-config-plugin
branch
from
2e3de1a to
a92eaa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cmd/ci-operator-prowgen/main.go`:
- Around line 135-137: The code currently assigns a host filesystem path
(o.fromFile) directly to configSpec.UnresolvedConfigPath which embeds an invalid
container path in generated jobs; change the assignment to a
repo/container-relative name by stripping host directories (e.g.,
configSpec.UnresolvedConfigPath = filepath.Base(o.fromFile) or otherwise compute
a repo-relative path) and ensure filepath import is added and empty/null cases
handled before calling prowgen.GenerateJobs(&configSpec, &info). This keeps the
generated job path valid inside the job container while still using the provided
file name.
In `@cmd/in-repo-config-plugin/server.go`:
- Around line 417-423: Change dirExists to return (bool, error) instead of
swallowing all stat errors: call os.Stat(path), if err == nil return
info.IsDir(), nil; if os.IsNotExist(err) return false, nil; otherwise return
false and propagate/wrap the error (e.g. fmt.Errorf("stat %s: %w", path, err)).
Update any callers of dirExists to handle the error path instead of assuming
false means "missing". This preserves permission/I/O errors while keeping
missing-file semantics.
- Around line 220-223: The loop that sets configSpec.UnresolvedConfigPath to the
constant cioperatorapi.CIOperatorInrepoConfigFileName causes every generated job
to point to ".ci-operator.yaml"; instead, carry the actual source path from
fetchConfigs into each configSpec (preserve the original filename or source path
returned by fetchConfigs), and set configSpec.UnresolvedConfigPath = <that
source path> inside the for filename, configSpec := range configs loop before
calling prowgen.GenerateJobs(info) so only the single-file fallback uses the
constant; update fetchConfigs to return the source path per config if it doesn't
already and use that value here (references: fetchConfigs, metadataFromFilename,
configSpec.UnresolvedConfigPath, prowgen.GenerateJobs).
- Around line 389-393: The commentError function currently posts the raw err
into a GitHub comment; change it to post a sanitized, generic user-facing
message instead (e.g., "@%s: command `%s` failed, please check plugin logs or
contact maintainers"), and send the full error details to the logger only.
Concretely, update commentError (and the call to s.ghc.CreateComment) to format
and post a redacted string without err, and call
l.WithError(err).Error("detailed error handling message") so the full error
stays in logrus; keep s.ghc.CreateComment and l references unchanged.
In `@pkg/jobconfig/files.go`:
- Around line 422-427: The current WriteBranchToDir path (loop over files ->
sortConfigFields(jc) -> WriteToFile(filepath.Join(jobDirForComponent, file),
jc)) blindly overwrites per-branch files and thus drops manually-managed fields;
update WriteBranchToDir to read the existing file (if present) for each target
path, merge/retain fields not managed by prowgen into the new jc (preserving
reporter_config and other unmanaged keys) before calling WriteToFile, using the
same file key from the files map and the jobDirForComponent path to locate the
existing content; implement the merge logic in a helper (e.g.,
mergePreservingUnmanaged) and call it prior to sortConfigFields/WriteToFile so
unmanaged fields remain intact.
In `@pkg/prowgen/jobbase.go`:
- Around line 42-44: The code is inserting only the basename of
UnresolvedConfigPath which truncates nested paths and causes sparse-checkout to
drop directories; update the insertion to use the full UnresolvedConfigPath
(i.e., pass configSpec.UnresolvedConfigPath to files.Insert rather than
path.Base(configSpec.UnresolvedConfigPath)) so the exact unresolved config path
is preserved; locate the occurrence around configSpec.UnresolvedConfigPath and
files.Insert to make this change.
In `@pkg/prowgen/podspec.go`:
- Around line 566-572: Add a Go doc comment immediately above the exported
UnresolvedConfig function describing its purpose, parameters, and return value:
explain that UnresolvedConfig(configPath string) returns a PodSpecMutator which
appends a command-line flag "--unresolved-config=<configPath>" to the first
container in a corev1.PodSpec; mention that it mutates the given PodSpec and
returns an error if mutation fails (or nil on success). Keep the comment concise
and follow Go doc style (start with "UnresolvedConfig").
🪄 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: 159822a4-064e-4352-8f22-4ae28f85f17f
📒 Files selected for processing (13)
cmd/ci-operator-prowgen/main.gocmd/in-repo-config-plugin/bootstrap.gocmd/in-repo-config-plugin/bootstrap_test.gocmd/in-repo-config-plugin/main.gocmd/in-repo-config-plugin/server.gocmd/in-repo-config-plugin/server_test.goimages/in-repo-config-plugin/Dockerfilepkg/api/types.gopkg/jobconfig/files.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/podspec.gopkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_unresolved_config__including_podspec.yaml
✅ Files skipped from review due to trivial changes (1)
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_unresolved_config__including_podspec.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/in-repo-config-plugin/bootstrap.go
- cmd/in-repo-config-plugin/bootstrap_test.go
- cmd/in-repo-config-plugin/server_test.go
- cmd/in-repo-config-plugin/main.go
Comment on lines
+135
to
+137
| configSpec.UnresolvedConfigPath = o.fromFile | ||
| generated, err := prowgen.GenerateJobs(&configSpec, &info) | ||
| if err != nil { |
There was a problem hiding this comment.
--unresolved-config is populated with a host path, not a repo path.
On Line 135, UnresolvedConfigPath is set to o.fromFile directly. In --from-file usage this can be an absolute/local path, which is then embedded in generated jobs and won’t exist inside the job container.
Suggested fix
- configSpec.UnresolvedConfigPath = o.fromFile
+ // ci-operator runs in the repo checkout; unresolved config path must be repo-relative.
+ configSpec.UnresolvedConfigPath = filepath.Base(o.fromFile)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| configSpec.UnresolvedConfigPath = o.fromFile | |
| generated, err := prowgen.GenerateJobs(&configSpec, &info) | |
| if err != nil { | |
| // ci-operator runs in the repo checkout; unresolved config path must be repo-relative. | |
| configSpec.UnresolvedConfigPath = filepath.Base(o.fromFile) | |
| generated, err := prowgen.GenerateJobs(&configSpec, &info) | |
| if err != nil { |
🤖 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/ci-operator-prowgen/main.go` around lines 135 - 137, The code currently
assigns a host filesystem path (o.fromFile) directly to
configSpec.UnresolvedConfigPath which embeds an invalid container path in
generated jobs; change the assignment to a repo/container-relative name by
stripping host directories (e.g., configSpec.UnresolvedConfigPath =
filepath.Base(o.fromFile) or otherwise compute a repo-relative path) and ensure
filepath import is added and empty/null cases handled before calling
prowgen.GenerateJobs(&configSpec, &info). This keeps the generated job path
valid inside the job container while still using the provided file name.
Comment on lines
+220
to
+223
| for filename, configSpec := range configs { | ||
| info := metadataFromFilename(filename, org, repo, branch) | ||
| configSpec.UnresolvedConfigPath = cioperatorapi.CIOperatorInrepoConfigFileName | ||
| generated, err := prowgen.GenerateJobs(configSpec, info) |
There was a problem hiding this comment.
Use the source config path here.
Every generated job gets UnresolvedConfigPath = ".ci-operator.yaml", even when it came from .ci-operator/<file>.yaml. Those ProwJobs will point ci-operator at the wrong file and fail on repos that only use split configs. Carry the actual source path through fetchConfigs and set it per config; only the single-file fallback should use .ci-operator.yaml.
🤖 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/in-repo-config-plugin/server.go` around lines 220 - 223, The loop that
sets configSpec.UnresolvedConfigPath to the constant
cioperatorapi.CIOperatorInrepoConfigFileName causes every generated job to point
to ".ci-operator.yaml"; instead, carry the actual source path from fetchConfigs
into each configSpec (preserve the original filename or source path returned by
fetchConfigs), and set configSpec.UnresolvedConfigPath = <that source path>
inside the for filename, configSpec := range configs loop before calling
prowgen.GenerateJobs(info) so only the single-file fallback uses the constant;
update fetchConfigs to return the source path per config if it doesn't already
and use that value here (references: fetchConfigs, metadataFromFilename,
configSpec.UnresolvedConfigPath, prowgen.GenerateJobs).
Comment on lines
+389
to
+393
| func (s *server) commentError(org, repo string, number int, user, command string, err error, l *logrus.Entry) { | ||
| comment := fmt.Sprintf("@%s: `%s` error:\n```\n%v\n```", user, command, err) | ||
| if commentErr := s.ghc.CreateComment(org, repo, number, comment); commentErr != nil { | ||
| l.WithError(commentErr).Error("failed to create error comment") | ||
| } |
There was a problem hiding this comment.
Don't publish raw backend errors to GitHub.
commentError renders err verbatim into a PR comment. Failures from filesystem, GitHub, or Kubernetes calls can include internal paths and cluster details that should stay in plugin logs, not on a public PR. Return a sanitized user message here and keep the full error in logrus. As per coding guidelines, "Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data."
🤖 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/in-repo-config-plugin/server.go` around lines 389 - 393, The commentError
function currently posts the raw err into a GitHub comment; change it to post a
sanitized, generic user-facing message instead (e.g., "@%s: command `%s` failed,
please check plugin logs or contact maintainers"), and send the full error
details to the logger only. Concretely, update commentError (and the call to
s.ghc.CreateComment) to format and post a redacted string without err, and call
l.WithError(err).Error("detailed error handling message") so the full error
stays in logrus; keep s.ghc.CreateComment and l references unchanged.
Comment on lines
+417
to
+423
| func dirExists(path string) bool { | ||
| info, err := os.Stat(path) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return info.IsDir() | ||
| } |
There was a problem hiding this comment.
Distinguish "missing" from "stat failed".
dirExists collapses permission errors, broken mounts, and transient I/O failures into false. In this plugin that means onboarding and /new-test can proceed as if no jobs exist when EFS or the release repo is actually unhealthy. Return (bool, error) and only treat os.IsNotExist as absence. As per coding guidelines, "Errors should be handled correctly - determine whether to ignore, log, wrap and raise up; use informative error messages".
🤖 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/in-repo-config-plugin/server.go` around lines 417 - 423, Change dirExists
to return (bool, error) instead of swallowing all stat errors: call
os.Stat(path), if err == nil return info.IsDir(), nil; if os.IsNotExist(err)
return false, nil; otherwise return false and propagate/wrap the error (e.g.
fmt.Errorf("stat %s: %w", path, err)). Update any callers of dirExists to handle
the error path instead of assuming false means "missing". This preserves
permission/I/O errors while keeping missing-file semantics.
Comment on lines
+422
to
+427
| for file, jc := range files { | ||
| sortConfigFields(jc) | ||
| if err := WriteToFile(filepath.Join(jobDirForComponent, file), jc); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
WriteBranchToDir overwrites branch files without preserving unmanaged fields.
The new write path replaces existing per-branch files directly, so manually maintained fields in those files can be lost (for example reporter config or cluster/max concurrency adjustments).
Suggested direction
- for file, jc := range files {
- sortConfigFields(jc)
- if err := WriteToFile(filepath.Join(jobDirForComponent, file), jc); err != nil {
- return err
- }
- }
+ for file, generated := range files {
+ target := filepath.Join(jobDirForComponent, file)
+ existing, err := readFromFile(target)
+ if err != nil && !os.IsNotExist(err) {
+ return err
+ }
+ if existing != nil {
+ allJobs := sets.New[string]()
+ for _, jobs := range generated.PresubmitsStatic {
+ for _, j := range jobs {
+ allJobs.Insert(j.Name)
+ }
+ }
+ for _, jobs := range generated.PostsubmitsStatic {
+ for _, j := range jobs {
+ allJobs.Insert(j.Name)
+ }
+ }
+ for _, j := range generated.Periodics {
+ allJobs.Insert(j.Name)
+ }
+ mergeJobConfig(existing, generated, allJobs)
+ generated = existing
+ }
+ sortConfigFields(generated)
+ if err := WriteToFile(target, generated); err != nil {
+ return err
+ }
+ }As per coding guidelines, "pkg/jobconfig/**: ... Must preserve fields that are not managed by prowgen (e.g. manually-set reporter_config)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for file, jc := range files { | |
| sortConfigFields(jc) | |
| if err := WriteToFile(filepath.Join(jobDirForComponent, file), jc); err != nil { | |
| return err | |
| } | |
| } | |
| for file, generated := range files { | |
| target := filepath.Join(jobDirForComponent, file) | |
| existing, err := readFromFile(target) | |
| if err != nil && !os.IsNotExist(err) { | |
| return err | |
| } | |
| if existing != nil { | |
| allJobs := sets.New[string]() | |
| for _, jobs := range generated.PresubmitsStatic { | |
| for _, j := range jobs { | |
| allJobs.Insert(j.Name) | |
| } | |
| } | |
| for _, jobs := range generated.PostsubmitsStatic { | |
| for _, j := range jobs { | |
| allJobs.Insert(j.Name) | |
| } | |
| } | |
| for _, j := range generated.Periodics { | |
| allJobs.Insert(j.Name) | |
| } | |
| mergeJobConfig(existing, generated, allJobs) | |
| generated = existing | |
| } | |
| sortConfigFields(generated) | |
| if err := WriteToFile(target, generated); err != nil { | |
| return err | |
| } | |
| } |
🤖 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/jobconfig/files.go` around lines 422 - 427, The current WriteBranchToDir
path (loop over files -> sortConfigFields(jc) ->
WriteToFile(filepath.Join(jobDirForComponent, file), jc)) blindly overwrites
per-branch files and thus drops manually-managed fields; update WriteBranchToDir
to read the existing file (if present) for each target path, merge/retain fields
not managed by prowgen into the new jc (preserving reporter_config and other
unmanaged keys) before calling WriteToFile, using the same file key from the
files map and the jobDirForComponent path to locate the existing content;
implement the merge logic in a helper (e.g., mergePreservingUnmanaged) and call
it prior to sortConfigFields/WriteToFile so unmanaged fields remain intact.
Comment on lines
+42
to
+44
| if configSpec.UnresolvedConfigPath != "" { | ||
| files.Insert(path.Base(configSpec.UnresolvedConfigPath)) | ||
| } |
There was a problem hiding this comment.
Sparse-checkout drops unresolved config directories.
Line 43 uses path.Base, so nested unresolved config paths are truncated. That can fetch the wrong file (or none) while --unresolved-config still references the full path.
Suggested fix
- if configSpec.UnresolvedConfigPath != "" {
- files.Insert(path.Base(configSpec.UnresolvedConfigPath))
- }
+ if configSpec.UnresolvedConfigPath != "" {
+ files.Insert(path.Clean(configSpec.UnresolvedConfigPath))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if configSpec.UnresolvedConfigPath != "" { | |
| files.Insert(path.Base(configSpec.UnresolvedConfigPath)) | |
| } | |
| if configSpec.UnresolvedConfigPath != "" { | |
| files.Insert(path.Clean(configSpec.UnresolvedConfigPath)) | |
| } |
🤖 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/prowgen/jobbase.go` around lines 42 - 44, The code is inserting only the
basename of UnresolvedConfigPath which truncates nested paths and causes
sparse-checkout to drop directories; update the insertion to use the full
UnresolvedConfigPath (i.e., pass configSpec.UnresolvedConfigPath to files.Insert
rather than path.Base(configSpec.UnresolvedConfigPath)) so the exact unresolved
config path is preserved; locate the occurrence around
configSpec.UnresolvedConfigPath and files.Insert to make this change.
Comment on lines
+566
to
+572
| func UnresolvedConfig(configPath string) PodSpecMutator { | ||
| return func(spec *corev1.PodSpec) error { | ||
| container := &spec.Containers[0] | ||
| addUniqueParameter(container, fmt.Sprintf("--unresolved-config=%s", configPath)) | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a doc comment for exported UnresolvedConfig.
This exported function is missing a Go doc comment, which makes public API behavior less discoverable.
As per coding guidelines, "Go documentation on Classes/Functions/Fields should be written properly" and "Comment important exported functions with their purpose, parameters, and return values."
🤖 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/prowgen/podspec.go` around lines 566 - 572, Add a Go doc comment
immediately above the exported UnresolvedConfig function describing its purpose,
parameters, and return value: explain that UnresolvedConfig(configPath string)
returns a PodSpecMutator which appends a command-line flag
"--unresolved-config=<configPath>" to the first container in a corev1.PodSpec;
mention that it mutates the given PodSpec and returns an error if mutation fails
(or nil on success). Keep the comment concise and follow Go doc style (start
with "UnresolvedConfig").
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
in-repo-config-plugin) that enables repos to manage CI configuration in-repo via.ci-operator.yamlfiles instead of the centralized openshift/release repository/onboardcommand bootstraps a repo with config-checker presubmit and prowgen postsubmit jobs on EFS/new-testcommand creates ProwJob CRs directly from the PR's.ci-operator/configs for new tests not yet on EFS, with--unresolved-configso ci-operator uses the in-repo config--from-filemode to ci-operator-prowgen for generating jobs from a single ci-operator config fileUnresolvedConfigpodspec mutator andWriteBranchToDirfor branch-scoped job writing🤖 Generated with Claude Code