Add CodeRabbit review configuration#5132
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a single Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.coderabbit.yaml (2)
171-184: Make custom check criteria more deterministic.Lines 174-184 use subjective wording (
should,trivial,pure), which can produce inconsistent warnings. Prefer explicit pass/fail criteria.Proposed tightening
- name: "Test Coverage for New Features" mode: warning instructions: | - New or modified functionality should include test coverage: - - New Go functions and methods should have corresponding unit tests - - Bug fixes should include a regression test that fails without the fix - - Pure functions should always be tested - - Prefer table-driven tests + Flag only when all are true: + 1) A non-test `*.go` file is added or modified, and + 2) exported functions/methods are added or changed, and + 3) no corresponding `*_test.go` file in the same package is changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 171 - 184, Update the "Test Coverage for New Features" custom check to use deterministic pass/fail language: replace subjective words like "should", "trivial", and "pure" with explicit rules (e.g., "must include unit tests for any new or modified Go function or method; bug fixes must include a regression test that fails without the fix; pure/deterministic functions are defined as functions with no external I/O or global-state side effects and therefore also must have tests"), enumerate exact exceptions (e.g., "renames, formatting-only, comment-only, generated code, configuration-only changes"), and change the check outcome wording to explicit actions (e.g., fail or allow) instead of vague guidance; update the "instructions" text for the "Test Coverage for New Features" entry to reflect these concrete criteria so the check yields consistent, repeatable results.
46-46: Update rule to reflect mixed logging practices in the codebase.The repository uses logrus in 376 files but also uses klog in 5 actual code files (ci-operator, ci-scheduling-webhook, jobrunaggregator). The hard-coded rule "Use logrus consistently for logging" conflicts with existing klog usage and would create misleading review feedback.
Reword to "Follow existing logger conventions per package/component" to respect the established patterns without prescribing a single logger across a mixed codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml at line 46, Update the .coderabbit.yaml rule text to reflect the mixed logging practices: replace the hard-coded rule string "Use logrus consistently for logging" with a neutral guideline like "Follow existing logger conventions per package/component" so the linter/review tool won't flag valid klog usage; ensure the rule key/name that currently holds that value is updated in the same declaration so references remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.coderabbit.yaml:
- Around line 171-184: Update the "Test Coverage for New Features" custom check
to use deterministic pass/fail language: replace subjective words like "should",
"trivial", and "pure" with explicit rules (e.g., "must include unit tests for
any new or modified Go function or method; bug fixes must include a regression
test that fails without the fix; pure/deterministic functions are defined as
functions with no external I/O or global-state side effects and therefore also
must have tests"), enumerate exact exceptions (e.g., "renames, formatting-only,
comment-only, generated code, configuration-only changes"), and change the check
outcome wording to explicit actions (e.g., fail or allow) instead of vague
guidance; update the "instructions" text for the "Test Coverage for New
Features" entry to reflect these concrete criteria so the check yields
consistent, repeatable results.
- Line 46: Update the .coderabbit.yaml rule text to reflect the mixed logging
practices: replace the hard-coded rule string "Use logrus consistently for
logging" with a neutral guideline like "Follow existing logger conventions per
package/component" so the linter/review tool won't flag valid klog usage; ensure
the rule key/name that currently holds that value is updated in the same
declaration so references remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ad98f6dc-4364-4fb0-bd22-f5997bdb32f4
📒 Files selected for processing (1)
.coderabbit.yaml
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
| needs to be mutated | ||
| - Prefer table-driven tests | ||
| - Use logrus consistently for logging | ||
| - No comments explaining what code does; only why when non-obvious |
There was a problem hiding this comment.
Maybe comment important functions only:
// foobar does this and that. Accepts `a` and `b`, returns ...
func foobar(a string, b int) (...) {
...
}|
|
||
| auto_review: | ||
| enabled: true | ||
| drafts: false |
There was a problem hiding this comment.
I have spotted this pattern:
- Open a PR
- Wait for coderabbitai to review
- Address any issue
- Possibly repeat from (1)
Through this cycle we keep force-pushing and triggering a lot of tests.
What if we allow coderabbitai to review draft PRs and the trigger the tests only when we are happy with the code changes?
I didn't investigate the possibility to run coderabbitai locally though. In that case this comment is no longer valid.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
coderabbit-config
branch
from
df5bc17 to
c7c8d03
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, 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 |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
@Prucek: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit def1c62
into
openshift:main
2 participants
Summary
.coderabbit.yamlwith Go-idiomatic review guidelines and codebase-specific path instructions🤖 Generated with Claude Code
Summary by CodeRabbit