enable in-repo ci-op config#5212
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughAdds an in-repo-config Prow plugin with webhook dispatch, pull request and push handling, bootstrap job generation, persistent branch shards, and ephemeral cleanup. It also extends prowgen to read single config files and preserve unresolved config paths in generated jobs. ChangesIn-repo configuration workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Prucek
changed the title
|
[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
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
| 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`.
| 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.
| 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.
| 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.
| 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.
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
| 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.
| 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.
| 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).
| 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.
| 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.
| 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.
| 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.
| 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.
| 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").
a92eaa5 to
c32c4e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/in-repo-config-plugin/server.go (1)
590-649: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGC mutates ephemeral dirs without the repo lock.
gcEphemeralDirscallsos.RemoveAllon a PR directory without acquiringrepoLock(org, repo), whilehandlePROpenedOrUpdated/handlePRClosedwrite and delete the same paths under that lock. A close-then-reopen (or sync racing GC) can delete a directory mid-write. Take the repo lock before removing.🤖 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 590 - 649, gcEphemeralDirs removes PR directories without synchronizing with the per-repo lock, which can race with handlePROpenedOrUpdated and handlePRClosed. Update gcEphemeralDirs to acquire the same repoLock(org, repo) before calling os.RemoveAll on a PR path, and hold it only around the delete/check for that repo. Keep the existing org/repo/PR traversal and logging, but ensure cleanup uses the same locking discipline as the write/delete paths.Source: Coding guidelines
🤖 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.go`:
- Around line 268-298: Handle the prowconfig.Load failure in server.go by
stopping the trigger flow instead of continuing with a nil prowCfg. In the
ProwJob defaulting logic inside the presubmit creation path, return immediately
after logging the load error so jobs are not created without namespace or
DecorationConfig defaults. Use the existing prowconfig.Load call, prowCfg
variable, and the surrounding job-processing loop to locate the fix.
---
Nitpick comments:
In `@cmd/in-repo-config-plugin/server.go`:
- Around line 590-649: gcEphemeralDirs removes PR directories without
synchronizing with the per-repo lock, which can race with
handlePROpenedOrUpdated and handlePRClosed. Update gcEphemeralDirs to acquire
the same repoLock(org, repo) before calling os.RemoveAll on a PR path, and hold
it only around the delete/check for that repo. Keep the existing org/repo/PR
traversal and logging, but ensure cleanup uses the same locking discipline as
the write/delete paths.
🪄 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: bb62c45e-bbe3-4966-8875-2786f7c95ffb
📒 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
🔗 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/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_unresolved_config__including_podspec.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/api/types.go
- cmd/in-repo-config-plugin/bootstrap_test.go
- pkg/prowgen/podspec.go
- pkg/prowgen/jobbase.go
- pkg/jobconfig/files.go
- cmd/in-repo-config-plugin/bootstrap.go
- cmd/ci-operator-prowgen/main.go
- pkg/prowgen/jobbase_test.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c32c4e8 to
116917d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
images/in-repo-config-plugin/Dockerfile (2)
2-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
COPYinstead ofADD.
ADD's extra behavior (extraction, URL fetch) isn't needed for a plain binary copy; Hadolint DL3020 flags this too.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🤖 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’s ADD instruction for in-repo-config-plugin with COPY, targeting only the required binary rather than the entire build context, while preserving the /usr/bin/in-repo-config-plugin destination.Source: Coding guidelines
1-3: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winStill runs as root.
No
USERdirective is present; the container runs as root by default, which Trivy also flags (DS-0002). This plugin writes to EFS-mounted--job-config-dir, so the UID/GID must align with those volume-mount permissions.As per coding guidelines, "USER non-root; never run as root".
🛡️ Proposed fix
FROM registry.access.redhat.com/ubi9/ubi-minimal:latest ADD in-repo-config-plugin /usr/bin/in-repo-config-plugin +USER 1000:1000 ENTRYPOINT ["/usr/bin/in-repo-config-plugin"]Confirm the UID/GID matches the EFS/git-sync mount permissions used in the deployment manifest.
🤖 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, Add a non-root USER directive to the in-repo-config-plugin image, using a UID/GID confirmed to match the EFS/git-sync volume permissions in the deployment manifest, and place it before ENTRYPOINT so the plugin never runs as root.Source: Coding guidelines
pkg/prowgen/jobbase.go (1)
42-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill truncating nested unresolved-config paths.
path.Base(configSpec.UnresolvedConfigPath)drops any directory components, so sparse-checkout can fetch the wrong file (or none) for nestedUnresolvedConfigPathvalues, while the actual--unresolved-configflag added inNewProwJobBaseBuilderstill references the full path.🐛 Proposed fix
- 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, Update the unresolved-config insertion logic to preserve the complete configSpec.UnresolvedConfigPath rather than applying path.Base, so sparse-checkout includes nested paths consistently with the --unresolved-config value configured by NewProwJobBaseBuilder.cmd/in-repo-config-plugin/server.go (2)
294-297: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrong
UnresolvedConfigPathfor split configs, at two sites. Every generated job getsUnresolvedConfigPath = ".ci-operator.yaml"regardless of whether the source was the single-file fallback or a file under.ci-operator/; for split-config repos the resulting ProwJobs will pointci-operatorat the wrong file and fail. Flagged previously and still present in both handlers.
cmd/in-repo-config-plugin/server.go#L294-L297: inhandlePush, setconfigSpec.UnresolvedConfigPathfrom the actual source path perfilename(e.g.filepath.Join(ciOperatorDir, filename)for directory-listing results), reserving the constant only for thefetchSingleConfigfallback.cmd/in-repo-config-plugin/server.go#L105-L108: apply the identical fix inhandlePROpenedOrUpdated, since it loops over the sameconfigsmap with the same bug.🤖 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 294 - 297, Update UnresolvedConfigPath assignment in handlePush at cmd/in-repo-config-plugin/server.go lines 294-297 and handlePROpenedOrUpdated at lines 105-108 to use the actual source path for each split config, such as filepath.Join(ciOperatorDir, filename); retain CIOperatorInrepoConfigFileName only for configs returned by fetchSingleConfig.
481-487: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
dirExistsstill collapses all stat failures to "missing".Permission errors, broken EFS mounts, and transient I/O failures are indistinguishable from "doesn't exist" here.
filterNewJobs(Line 346) andgcEphemeralDirs(Line 505) both rely on this, so an unhealthy mount would make onboarding/detection silently behave as if no jobs/ephemeral dirs exist. Same concern flagged on a prior commit, still unaddressed.As per coding guidelines, "Errors should be handled correctly - determine whether to ignore, log, wrap and raise up; use informative error messages."
🛡️ Proposed fix
-func dirExists(path string) bool { - info, err := os.Stat(path) - if err != nil { - return false - } - return info.IsDir() -} +func dirExists(path string) (bool, error) { + info, err := os.Stat(path) + if err == nil { + return info.IsDir(), nil + } + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("stat %s: %w", path, 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 `@cmd/in-repo-config-plugin/server.go` around lines 481 - 487, Update dirExists to distinguish a nonexistent path from other os.Stat failures instead of returning false for every error. Propagate or otherwise surface permission, mount, and I/O errors, and update filterNewJobs and gcEphemeralDirs to handle the resulting error explicitly while preserving false only for paths confirmed not to exist.Source: Coding guidelines
pkg/jobconfig/files.go (1)
374-430: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftStill overwrites branch files without preserving unmanaged fields.
WriteBranchToDirwrites generated shards straight to disk viaWriteToFileAtomicwith no read-and-merge of existing content, so manually-maintained fields (e.g.reporter_config, cluster/max-concurrency overrides) in those per-branch files are lost on every write. This was already flagged on a prior commit and remains unaddressed.As per path instructions, "
pkg/jobconfig/**: ... Must preserve fields that are not managed by prowgen (e.g. manually-set reporter_config)."🤖 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 374 - 430, The WriteBranchToDir function must preserve unmanaged fields in existing per-branch files instead of replacing them. Before each WriteToFileAtomic call, read the corresponding existing shard, merge its non-prowgen fields into the generated JobConfig, and then write the merged result; preserve generated jobs and managed-field updates while retaining fields such as reporter_config and cluster/max-concurrency overrides.Source: Path instructions
🧹 Nitpick comments (3)
images/in-repo-config-plugin/Dockerfile (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo
HEALTHCHECKdefined.Container security guidelines for this path call for a defined
HEALTHCHECK.As per coding guidelines, "HEALTHCHECK defined".
🤖 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 - 4, Add a Docker HEALTHCHECK instruction to the image defined by the Dockerfile, using the existing /usr/bin/in-repo-config-plugin entrypoint or an appropriate lightweight availability check, while preserving the current ADD and ENTRYPOINT behavior.Source: Coding guidelines
cmd/in-repo-config-plugin/server.go (1)
162-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated ProwJob defaulting logic.
The cluster/namespace/DecorationConfig defaulting block is repeated almost verbatim for the presubmit loop and the periodic-as-presubmit loop. Consider extracting a small helper (e.g.
applyProwJobDefaults(job *prowconfig.JobBase, prowCfg, orgrepo)) to keep the two loops in sync going forward.🤖 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 162 - 220, The presubmit and periodic-as-presubmit loops duplicate cluster, namespace, and DecorationConfig defaulting. Extract this logic into a shared helper accepting the relevant JobBase, prowCfg, and orgrepo values, then call it from both loops before constructing the ProwJob while preserving the current defaults and decoration behavior.cmd/in-repo-config-plugin/server_test.go (1)
115-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage for PR open/sync/close paths. Once the
filterNewJobspostsubmit-drop issue inserver.gois fixed, consider adding a case here with only a new postsubmit test to lock in 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 115 - 243, The TestHandlePullRequest table lacks coverage for a PR containing only a newly added postsubmit test. After fixing filterNewJobs in handlePullRequest’s server flow, add a test case with only a new postsubmit configuration and assert the expected job generation/comment behavior, preserving the existing open/synchronize/close coverage.
🤖 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.go`:
- Around line 118-127: Acquire the repository lock before calling filterNewJobs
so its jc.ReadFromDir state read is serialized with handlePush’s
WriteBranchToDir writes. Move the repoLock acquisition and deferred unlock ahead
of the filterNewJobs call, while preserving the existing no-new-tests early
return within the locked section.
- Around line 343-389: Update filterNewJobs to collect newly detected postsubmit
jobs in a newPostsubmits slice and return it alongside the existing results.
Thread that slice through handlePROpenedOrUpdated into
ephemeralJobs.PostsubmitsStatic[orgrepo], and include the postsubmit names in
the “New tests detected” comment listing so postsubmit-only changes are
preserved and reported.
- Around line 162-220: Add a bounded timeout context for ProwJob creation and
use it for both s.pjclient.Create calls in the newPresubmits and newPeriodics
loops. Replace each context.Background() passed to Create with the
deadline-bearing context, ensuring stalled API requests cannot block
indefinitely.
- Around line 391-395: Update fetchConfigs so it calls fetchSingleConfig only
when GetDirectory returns github.FileNotFound; return the original GetDirectory
error for authentication, rate-limit, and other failures. Preserve the existing
successful directory-processing path and use the repository’s existing GitHub
error symbols or matching mechanism.
---
Duplicate comments:
In `@cmd/in-repo-config-plugin/server.go`:
- Around line 294-297: Update UnresolvedConfigPath assignment in handlePush at
cmd/in-repo-config-plugin/server.go lines 294-297 and handlePROpenedOrUpdated at
lines 105-108 to use the actual source path for each split config, such as
filepath.Join(ciOperatorDir, filename); retain CIOperatorInrepoConfigFileName
only for configs returned by fetchSingleConfig.
- Around line 481-487: Update dirExists to distinguish a nonexistent path from
other os.Stat failures instead of returning false for every error. Propagate or
otherwise surface permission, mount, and I/O errors, and update filterNewJobs
and gcEphemeralDirs to handle the resulting error explicitly while preserving
false only for paths confirmed not to exist.
In `@images/in-repo-config-plugin/Dockerfile`:
- Line 2: Replace the Dockerfile’s ADD instruction for in-repo-config-plugin
with COPY, targeting only the required binary rather than the entire build
context, while preserving the /usr/bin/in-repo-config-plugin destination.
- Around line 1-3: Add a non-root USER directive to the in-repo-config-plugin
image, using a UID/GID confirmed to match the EFS/git-sync volume permissions in
the deployment manifest, and place it before ENTRYPOINT so the plugin never runs
as root.
In `@pkg/jobconfig/files.go`:
- Around line 374-430: The WriteBranchToDir function must preserve unmanaged
fields in existing per-branch files instead of replacing them. Before each
WriteToFileAtomic call, read the corresponding existing shard, merge its
non-prowgen fields into the generated JobConfig, and then write the merged
result; preserve generated jobs and managed-field updates while retaining fields
such as reporter_config and cluster/max-concurrency overrides.
In `@pkg/prowgen/jobbase.go`:
- Around line 42-44: Update the unresolved-config insertion logic to preserve
the complete configSpec.UnresolvedConfigPath rather than applying path.Base, so
sparse-checkout includes nested paths consistently with the --unresolved-config
value configured by NewProwJobBaseBuilder.
---
Nitpick comments:
In `@cmd/in-repo-config-plugin/server_test.go`:
- Around line 115-243: The TestHandlePullRequest table lacks coverage for a PR
containing only a newly added postsubmit test. After fixing filterNewJobs in
handlePullRequest’s server flow, add a test case with only a new postsubmit
configuration and assert the expected job generation/comment behavior,
preserving the existing open/synchronize/close coverage.
In `@cmd/in-repo-config-plugin/server.go`:
- Around line 162-220: The presubmit and periodic-as-presubmit loops duplicate
cluster, namespace, and DecorationConfig defaulting. Extract this logic into a
shared helper accepting the relevant JobBase, prowCfg, and orgrepo values, then
call it from both loops before constructing the ProwJob while preserving the
current defaults and decoration behavior.
In `@images/in-repo-config-plugin/Dockerfile`:
- Around line 1-4: Add a Docker HEALTHCHECK instruction to the image defined by
the Dockerfile, using the existing /usr/bin/in-repo-config-plugin entrypoint or
an appropriate lightweight availability check, while preserving the current ADD
and ENTRYPOINT behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6d101c87-91e1-468f-a892-caebbcf9a218
📒 Files selected for processing (14)
cmd/ci-operator-checkconfig/main.gocmd/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
🔗 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 (7)
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_unresolved_config__including_podspec.yaml
- pkg/prowgen/podspec.go
- cmd/in-repo-config-plugin/bootstrap.go
- pkg/api/types.go
- pkg/prowgen/jobbase_test.go
- cmd/in-repo-config-plugin/main.go
- cmd/ci-operator-prowgen/main.go
| newJobNames, newPresubmits, newPeriodics := filterNewJobs(allJobs, s.jobConfigDir, org, repo, logger) | ||
|
|
||
| if len(newJobNames) == 0 { | ||
| logger.Info("no new tests detected, skipping ephemeral write") | ||
| return | ||
| } | ||
|
|
||
| lock := s.repoLock(org, repo) | ||
| lock.Lock() | ||
| defer lock.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Existing-job read happens outside the repo lock.
filterNewJobs reads permanent job state via jc.ReadFromDir (Line 118) before s.repoLock is acquired (Lines 125-127). A concurrent handlePush writing via WriteBranchToDir under the same lock could race with this read, causing stale "already exists"/"new" decisions.
🤖 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 118 - 127, Acquire the
repository lock before calling filterNewJobs so its jc.ReadFromDir state read is
serialized with handlePush’s WriteBranchToDir writes. Move the repoLock
acquisition and deferred unlock ahead of the filterNewJobs call, while
preserving the existing no-new-tests early return within the locked section.
| for _, job := range newPresubmits { | ||
| if job.Cluster == "" { | ||
| job.Cluster = kube.DefaultClusterAlias | ||
| } | ||
| if job.Namespace == nil || *job.Namespace == "" { | ||
| ns := prowCfg.PodNamespace | ||
| job.Namespace = &ns | ||
| } | ||
| if dc := prowCfg.Plank.GuessDefaultDecorationConfig(orgrepo, job.Cluster); dc != nil { | ||
| if job.DecorationConfig != nil { | ||
| job.DecorationConfig = dc.ApplyDefault(job.DecorationConfig) | ||
| } else { | ||
| job.DecorationConfig = dc | ||
| } | ||
| } | ||
| pj := pjutil.NewProwJob(pjutil.PresubmitSpec(job, refs), job.Labels, job.Annotations) | ||
| pj.Namespace = s.namespace | ||
| if err := s.pjclient.Create(context.Background(), &pj); err != nil { | ||
| logger.WithError(err).WithField("job", job.Name).Error("could not create ProwJob") | ||
| continue | ||
| } | ||
| triggered = append(triggered, job.Name) | ||
| } | ||
| for _, job := range newPeriodics { | ||
| var extraRefs []pjapi.Refs | ||
| for _, ref := range job.ExtraRefs { | ||
| if ref.Org == org && ref.Repo == repo { | ||
| continue | ||
| } | ||
| extraRefs = append(extraRefs, ref) | ||
| } | ||
| job.ExtraRefs = extraRefs | ||
| testName := periodicTestName(job.Name, org, repo, branch) | ||
| presubmit := prowconfig.Presubmit{ | ||
| JobBase: job.JobBase, | ||
| Reporter: prowconfig.Reporter{Context: fmt.Sprintf("ci/prow/%s", testName)}, | ||
| } | ||
| if presubmit.Cluster == "" { | ||
| presubmit.Cluster = kube.DefaultClusterAlias | ||
| } | ||
| if presubmit.Namespace == nil || *presubmit.Namespace == "" { | ||
| ns := prowCfg.PodNamespace | ||
| presubmit.Namespace = &ns | ||
| } | ||
| if dc := prowCfg.Plank.GuessDefaultDecorationConfig(orgrepo, presubmit.Cluster); dc != nil { | ||
| if presubmit.DecorationConfig != nil { | ||
| presubmit.DecorationConfig = dc.ApplyDefault(presubmit.DecorationConfig) | ||
| } else { | ||
| presubmit.DecorationConfig = dc | ||
| } | ||
| } | ||
| pj := pjutil.NewProwJob(pjutil.PresubmitSpec(presubmit, refs), presubmit.Labels, presubmit.Annotations) | ||
| pj.Namespace = s.namespace | ||
| if err := s.pjclient.Create(context.Background(), &pj); err != nil { | ||
| logger.WithError(err).WithField("job", job.Name).Error("could not create ProwJob for periodic") | ||
| continue | ||
| } | ||
| triggered = append(triggered, job.Name) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the controller-runtime client is built with a timeout-bearing rest.Config
rg -n -A5 -B5 'ctrlruntimeclient.New|rest.Config' cmd/in-repo-config-plugin/main.goRepository: openshift/ci-tools
Length of output: 554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how clusterConfig is built and whether a timeout is set anywhere nearby.
sed -n '1,260p' cmd/in-repo-config-plugin/main.go
echo '---'
rg -n 'Timeout|timeout|rest\.Config|clusterConfig|NewForConfig|New\(' cmd/in-repo-config-plugin -g '*.go'Repository: openshift/ci-tools
Length of output: 10470
🌐 Web query:
kubernetes client-go rest.Config Timeout context.Background controller-runtime client.Create no deadline
💡 Result:
When using controller-runtime's client, passing context.Background—which has no deadline—does not inherently mean your Kubernetes request will wait indefinitely [1][2]. The effective timeout behavior is determined by the interaction between the context passed to the method (e.g., client.Create) and the configuration of the underlying rest.Config [3][4]. 1. Rest Config Timeout: The Timeout field in rest.Config is an optional setting for the underlying HTTP client [3][5]. If this value is greater than zero, it sets a hard limit on the duration of a request [3][4]. Even if you pass context.Background to a client call, the client-go library will often wrap the context using context.WithTimeout based on the value configured in rest.Config [4]. 2. Context Inheritance: When you invoke a method like client.Create(ctx, obj), the provided ctx is the primary driver for request cancellation [6][7]. If you pass context.Background, you are not providing a deadline, meaning the request will rely on other mechanisms for termination, such as: - The rest.Config.Timeout, if it is set to a non-zero value [4]. - The underlying network transport timeouts or server-side timeouts. 3. Best Practice: While passing context.Background is syntactically valid, it is generally recommended to pass a context with a timeout or cancellation signal (e.g., context.WithTimeout(context.Background, timeout)) to ensure that your application can recover from stalled requests or unresponsive API servers, rather than relying solely on the global rest.Config default [4]. In summary, passing context.Background does not guarantee an infinite wait if rest.Config.Timeout is configured; it simply delegates the timeout responsibility to the client configuration or network layer [4].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/example_test.go
- 2: How to correctly use context arguments is unclear kubernetes-sigs/controller-runtime#265
- 3: https://github.com/kubernetes/client-go/blob/bdfc4cfc125f556bfe46fff68207767a605b64ed/rest/config.go
- 4: https://github.com/kubernetes/client-go/blob/v0.35.3/rest/request.go
- 5: https://github.com/kubernetes/client-go/blob/v11.0.0/rest/config.go
- 6: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/client.go
- 7: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/interfaces.go
Add a request deadline to ProwJob creation
pjclient.Create(context.Background(), &pj) still has no deadline here, and the in-cluster client config does not set one. A stalled API server can hold the handler goroutine and concurrency slot open indefinitely; use a bounded context for both Create calls.
🤖 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 162 - 220, Add a bounded
timeout context for ProwJob creation and use it for both s.pjclient.Create calls
in the newPresubmits and newPeriodics loops. Replace each context.Background()
passed to Create with the deadline-bearing context, ensuring stalled API
requests cannot block indefinitely.
| func filterNewJobs(allJobs *prowconfig.JobConfig, jobConfigDir, org, repo string, logger *logrus.Entry) ([]string, []prowconfig.Presubmit, []prowconfig.Periodic) { | ||
| existingNames := map[string]bool{} | ||
| permanentPath := filepath.Join(jobConfigDir, org, repo) | ||
| if dirExists(permanentPath) { | ||
| existing, err := jc.ReadFromDir(permanentPath) | ||
| if err != nil { | ||
| logger.WithError(err).Warn("could not read existing jobs from EFS") | ||
| } else { | ||
| for _, jobs := range existing.PresubmitsStatic { | ||
| for _, j := range jobs { | ||
| existingNames[j.Name] = true | ||
| } | ||
| } | ||
| for _, jobs := range existing.PostsubmitsStatic { | ||
| for _, j := range jobs { | ||
| existingNames[j.Name] = true | ||
| } | ||
| } | ||
| for _, j := range existing.Periodics { | ||
| existingNames[j.Name] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var newJobNames []string | ||
| var newPresubmits []prowconfig.Presubmit | ||
| var newPeriodics []prowconfig.Periodic | ||
| orgrepo := fmt.Sprintf("%s/%s", org, repo) | ||
| for _, j := range allJobs.PresubmitsStatic[orgrepo] { | ||
| if !existingNames[j.Name] { | ||
| newJobNames = append(newJobNames, j.Name) | ||
| newPresubmits = append(newPresubmits, j) | ||
| } | ||
| } | ||
| for _, j := range allJobs.PostsubmitsStatic[orgrepo] { | ||
| if !existingNames[j.Name] { | ||
| newJobNames = append(newJobNames, j.Name) | ||
| } | ||
| } | ||
| for _, j := range allJobs.Periodics { | ||
| if !existingNames[j.Name] { | ||
| newJobNames = append(newJobNames, j.Name) | ||
| newPeriodics = append(newPeriodics, j) | ||
| } | ||
| } | ||
| return newJobNames, newPresubmits, newPeriodics | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Postsubmit jobs are silently dropped from ephemeral detection.
filterNewJobs counts new postsubmit names into newJobNames (Lines 377-381) but never returns them — there's no newPostsubmits slice. Downstream, handlePROpenedOrUpdated (Lines 118-140) builds ephemeralJobs.PostsubmitsStatic as an always-empty map, so a PR whose only new test is a postsubmit will pass the len(newJobNames) == 0 gate, write an ephemeral file with no presubmits/periodics, and post a "New tests detected" comment (Lines 222-229) that lists nothing.
🐛 Proposed fix
-func filterNewJobs(allJobs *prowconfig.JobConfig, jobConfigDir, org, repo string, logger *logrus.Entry) ([]string, []prowconfig.Presubmit, []prowconfig.Periodic) {
+func filterNewJobs(allJobs *prowconfig.JobConfig, jobConfigDir, org, repo string, logger *logrus.Entry) ([]string, []prowconfig.Presubmit, []prowconfig.Postsubmit, []prowconfig.Periodic) {
...
+ var newPostsubmits []prowconfig.Postsubmit
for _, j := range allJobs.PostsubmitsStatic[orgrepo] {
if !existingNames[j.Name] {
newJobNames = append(newJobNames, j.Name)
+ newPostsubmits = append(newPostsubmits, j)
}
}
...
- return newJobNames, newPresubmits, newPeriodics
+ return newJobNames, newPresubmits, newPostsubmits, newPeriodics
}Then thread newPostsubmits into ephemeralJobs.PostsubmitsStatic[orgrepo] and into the comment listing at the call site.
🤖 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 343 - 389, Update
filterNewJobs to collect newly detected postsubmit jobs in a newPostsubmits
slice and return it alongside the existing results. Thread that slice through
handlePROpenedOrUpdated into ephemeralJobs.PostsubmitsStatic[orgrepo], and
include the postsubmit names in the “New tests detected” comment listing so
postsubmit-only changes are preserved and reported.
| func (s *server) fetchConfigs(org, repo, sha string, l *logrus.Entry) (map[string]*cioperatorapi.ReleaseBuildConfiguration, error) { | ||
| entries, err := s.ghc.GetDirectory(org, repo, ciOperatorDir, sha) | ||
| if err != nil { | ||
| return s.fetchSingleConfig(org, repo, sha, l) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- server.go around fetchConfigs ---\n'
sed -n '360,430p' cmd/in-repo-config-plugin/server.go
printf '\n--- search for fetchSingleConfig and FileNotFound ---\n'
rg -n "fetchSingleConfig|FileNotFound|GetDirectory\\(" cmd/in-repo-config-plugin -S
printf '\n--- tests in cmd/in-repo-config-plugin ---\n'
find cmd/in-repo-config-plugin -maxdepth 2 -type f | sortRepository: openshift/ci-tools
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '380,420p' cmd/in-repo-config-plugin/server.go
printf '\n---\n'
rg -n "type FileNotFound|FileNotFound struct|GetDirectory\\(|fetchSingleConfig" cmd/in-repo-config-plugin -S
printf '\n--- tests ---\n'
rg -n "GetDirectory|FileNotFound|fetchConfigs|fetchSingleConfig" cmd/in-repo-config-plugin -g '*_test.go' -SRepository: openshift/ci-tools
Length of output: 2297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('cmd/in-repo-config-plugin/server.go')
text = p.read_text()
for needle in ['fetchConfigs', 'fetchSingleConfig', 'GetDirectory(', 'FileNotFound']:
idx = text.find(needle)
print(f'needle={needle!r} idx={idx}')
if idx != -1:
start = max(0, text.rfind('\n', 0, idx-200))
end = text.find('\n', idx+300)
print(text[start:end])
print('---')
PYRepository: openshift/ci-tools
Length of output: 1979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '427,520p' cmd/in-repo-config-plugin/server.go
printf '\n--- server_test.go relevant section ---\n'
sed -n '1,140p' cmd/in-repo-config-plugin/server_test.goRepository: openshift/ci-tools
Length of output: 6040
Only fall back on missing .ci-operator/
fetchConfigs should return the GetDirectory error unless it is github.FileNotFound; otherwise auth, rate-limit, and other API failures get masked by the .ci-operator.yaml fallback.
🤖 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 391 - 395, Update
fetchConfigs so it calls fetchSingleConfig only when GetDirectory returns
github.FileNotFound; return the original GetDirectory error for authentication,
rate-limit, and other failures. Preserve the existing successful
directory-processing path and use the repository’s existing GitHub error symbols
or matching mechanism.
1 participant
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.ci-operator/configs and creates ephemeral ProwJob definitions + triggers them automatically.ci-operator/configs, auto-onboards the repo with bootstrap jobs and initial test definitions--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
Summary
Adds the
in-repo-config-pluginto manage OpenShift CI configuration directly from repository.ci-operator/files.ci-operator-prowgen --from-filesupport for single configuration files.ci-operator-checkconfigbehavior when cluster-claim configuration is not provided.