CORENET-7267: Add pre-merge and custom checks to CodeRabbit configuration#3037
Open
tssurya wants to merge 1 commit into
Open
CORENET-7267: Add pre-merge and custom checks to CodeRabbit configuration#3037tssurya wants to merge 1 commit into
tssurya wants to merge 1 commit into
Conversation
Enable request_changes_workflow and configure both built-in pre-merge checks (title, description, docstrings, issue_assessment) and 12 custom checks covering: PR description quality, commit message quality, unit test and e2e test coverage, sensitive data in logs, RBAC least privilege, documentation for feature changes, stale docs detection, CNO-specific test quality, Go code quality, large PR detection, and AI-generated code smell detection. These checks enforce consistent PR hygiene, ensure test and documentation coverage for behavioral changes, and catch common Go code quality issues specific to the CNO codebase. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
WalkthroughUpdated ChangesCodeRabbit pre-merge policy updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tssurya
changed the title
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@tssurya: This pull request references CORENET-7267 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tssurya 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.coderabbit.yaml (3)
142-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
@coderabbitai ignore pre-merge checksoverrides every check, not just this one.The override guidance here (and in the E2E/Docs checks) is reasonably guarded with "once all other pre-merge checks are also addressed," but it's worth being explicit that this command suppresses all pre-merge checks for the PR — it is not a per-check escape hatch. Authors who run it to bypass a single false-positive will also disable the RBAC, sensitive-data, and AI-smell gates. Consider noting this caveat so the override isn't used prematurely.
🤖 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 @.coderabbit.yaml around lines 142 - 161, The override guidance for the Unit Tests for Go Changes check should explicitly say that `@coderabbitai` ignore pre-merge checks disables all pre-merge checks, not just this one. Update the instructions in the .coderabbit.yaml entry for this check to warn that the command is a global override and should only be used after all other checks are addressed, so authors don’t treat it as a per-check bypass.
62-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOverlapping
error-mode checks will fail redundantly on the same root cause.
PR Quality(description/"How to verify it" + size), the built-indescriptioncheck,E2E Tests for Feature Changes, andDocs for Feature and Behavior Changesall independently require/inspect the "Testing/How to verify it" section. A PR missing that section will trip several checks at once, producing noisy duplicate failures for one underlying gap. Consider consolidating the description-section requirements into a single source of truth (e.g., let the built-indescriptioncheck own section presence, and keep custom checks focused on their unique concern) to reduce friction.🤖 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 @.coderabbit.yaml around lines 62 - 107, The PR Quality check overlaps with other description-related checks and will duplicate failures for the same missing “How to verify it/Testing” section. Update the PR Quality rule in the configuration so it does not re-validate section presence already covered by the built-in description check; keep PR Quality focused on size and any unique quality criteria, and reference the existing description-based checks to avoid redundant error-mode failures.
55-57: 📐 Maintainability & Code Quality | 🔵 TrivialConsider turning off
docstringsfor this Go repoThis check is supported, but it often adds recurring noise on Go PRs. If the coverage line isn’t useful here,
mode: "off"is a better fit.♻️ Optional
docstrings: - mode: "warning" - threshold: 80 + mode: "off"🤖 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 @.coderabbit.yaml around lines 55 - 57, The `docstrings` check in the `.coderabbit.yaml` config is currently enabled in warning mode, which is causing unnecessary noise for this Go repo. Update the configuration around the `docstrings` setting to turn it off entirely by changing its mode from warning to off, keeping the existing threshold section only if it is still needed for future use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.coderabbit.yaml:
- Around line 142-161: The override guidance for the Unit Tests for Go Changes
check should explicitly say that `@coderabbitai` ignore pre-merge checks disables
all pre-merge checks, not just this one. Update the instructions in the
.coderabbit.yaml entry for this check to warn that the command is a global
override and should only be used after all other checks are addressed, so
authors don’t treat it as a per-check bypass.
- Around line 62-107: The PR Quality check overlaps with other
description-related checks and will duplicate failures for the same missing “How
to verify it/Testing” section. Update the PR Quality rule in the configuration
so it does not re-validate section presence already covered by the built-in
description check; keep PR Quality focused on size and any unique quality
criteria, and reference the existing description-based checks to avoid redundant
error-mode failures.
- Around line 55-57: The `docstrings` check in the `.coderabbit.yaml` config is
currently enabled in warning mode, which is causing unnecessary noise for this
Go repo. Update the configuration around the `docstrings` setting to turn it off
entirely by changing its mode from warning to off, keeping the existing
threshold section only if it is still needed for future use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3ab78192-836e-49d7-9413-07943fdd8c8a
📒 Files selected for processing (1)
.coderabbit.yaml
Contributor
Author
|
@coderabbitai: I am confused though - your docs said you will fail the review and ask for requested changes if the pre-merge checks fail right? My title is ugly and long and overflowing - isn't that grounds to ask to change that? But I can see you happily approved my PR! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query:
Length of output: 15507
What I found:
So the most likely explanation is:
There’s also an important nuance in the docs: pre-merge So in short: your expectation was reasonable, but the config being introduced by this same PR is likely why you still saw an approval. If you want, I can do a fresh full review of |
Contributor
Author
|
@coderabbitai I don't know what you mean by: how do you block the PR merge w/o asking for requested changes in red, right now your review just made the the pre-merge check pass - how else to enforce folks to read the pre-merge checks table in your walkthrough comment? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query:
Length of output: 16436
A PR can be blocked by failing CodeRabbit/GitHub checks even if there is no separate red GitHub “Request changes” review event on the conversation thread. The docs distinguish between two things:
So the “block” is the failing pre-merge/check status, not necessarily a standalone red review card in the timeline. The second important piece — and the part that matters for this PR — is that the docs also say:
So for
Which means your complaint is valid: for this PR, the new title rule did not end up enforcing what you expected. To answer your specific point:
If the config is already active before a PR is opened/reviewed, enforcement should come from the failing pre-merge check status on the PR, not from hoping people read the walkthrough table. The walkthrough explains why it failed; the actual enforcement is the blocked status. So the practical takeaway is:
In short: you were expecting |
|
Tip For best results, initiate chat on the files or code changes.
The direct answer is:
So why did it pass? Because
So your summary is basically correct:
Yes — on this PR, that is what happened. The enforcement you expected should show up on the next PR after this config lands:
In short: my earlier “it can still block merge” explanation is true in general, but it does not explain the behavior of |
Contributor
Author
|
@coderabbitai: So are you saying after this PR merges rabbit will start asking for requested changes if pre-merge checks fail? isn't true right? i.e the request_changes_workflow: true is being set only in this PR so coderabbitai hasn't been approving PRs with GUI before right? this is the first PR? |
Contributor
Author
|
@coderabbitai: configuration |
|
Tip For best results, initiate chat on the files or code changes.
What it confirms from the effective repo configuration:
So yes: once this config is on the default branch, future PRs should be evaluated with the request-changes workflow enabled and those checks treated as errors. The other important line is:
That means I should be careful not to overstate this as “the CodeRabbit status will always go red and fail the PR” based on config alone. The repo is clearly configured to use the request-changes workflow, but this setting suggests the plain commit/check status behavior is not the strongest signal here. So the clean summary is:
The config also disproves one possible misunderstanding from earlier: this is not a repo where One caveat: the returned schema was empty, so I can validate the active settings, but not derive more precise product semantics from the schema itself. If you want, the most practical next step is:
That would answer the UI/enforcement question definitively for this repo. |
Contributor
Author
|
@coderabbitai: configuration |
|
Tip For best results, initiate chat on the files or code changes.
The more precise answer is:
What I can say confidently:
What I should not have said so definitively:
So the corrected summary is:
If you want, after this lands I can help validate it on a tiny follow-up PR with an intentionally bad title. |
Contributor
Author
|
@coderabbitai configuration |
|
Configuration used: Repository YAML (base), Central YAML (inherited) CodeRabbit configuration# Source: Central YAML (inherited)
language: en-US
# Source: defaults
tone_instructions: ''
# Source: Central YAML (inherited)
early_access: false
# Source: Central YAML (inherited)
enable_free_tier: false
# Source: Repository YAML (base)
inheritance: true
reviews:
# Source: Central YAML (inherited)
profile: chill
# Source: Repository YAML (base)
request_changes_workflow: true
# Source: Repository YAML (base)
high_level_summary: true
# Source: defaults
high_level_summary_instructions: ''
# Source: defaults
high_level_summary_placeholder: '`@coderabbitai` summary'
# Source: Repository YAML (base)
high_level_summary_in_walkthrough: true
# Source: defaults
auto_title_placeholder: '`@coderabbitai`'
# Source: defaults
auto_title_instructions: ''
# Source: Central YAML (inherited)
review_status: true
# Source: defaults
review_details: false
# Source: defaults
commit_status: true
# Source: defaults
fail_commit_status: false
# Source: Central YAML (inherited)
collapse_walkthrough: false
# Source: defaults
changed_files_summary: true
# Source: defaults
sequence_diagrams: true
# Source: defaults
estimate_code_review_effort: true
# Source: defaults
assess_linked_issues: true
# Source: defaults
related_issues: true
# Source: defaults
related_prs: true
# Source: defaults
suggested_labels: true
# Source: defaults
labeling_instructions: []
# Source: defaults
mutually_exclusive_groups: {}
# Source: defaults
auto_apply_labels: false
# Source: defaults
suggested_reviewers: true
# Source: defaults
auto_assign_reviewers: false
# Source: defaults
suggested_reviewers_instructions: []
# Source: Central YAML (inherited)
in_progress_fortune: false
# Source: Repository YAML (base)
poem: false
# Source: defaults
enable_prompt_for_ai_agents: true
# Source: Repository YAML (base), Central YAML (inherited)
path_filters:
- '!vendor/**'
- '!_output/**'
- '!**/vendor/**'
- '!**/zz_generated*'
- '!boilerplate/**'
- '!node_modules/**'
- '!dist/**'
- '!*.min.js'
- '!yarn.lock'
# Source: Repository YAML (base), Central YAML (inherited)
path_instructions:
- path: '**'
instructions: |
This is the OpenShift Cluster Network Operator (CNO). It manages the lifecycle
of cluster networking components in OpenShift including OVN-Kubernetes, Multus,
kube-proxy, network-diagnostics, egress-router, cloud-network-config-controller,
network-metrics, node-identity, FRR-K8s, iptables-alerter, MTU-prober,
container-networking-plugins, bond CNI, whereabouts CNI, route-override CNI,
multi-network-policy, and the networking console plugin.
- Focus on major issues impacting performance, readability, maintainability and security.
- Consider backward compatibility, upgrade safety, and multi-platform support.
- Avoid nitpicks and avoid verbosity.
- path: '**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}'
instructions: |
Injection prevention (prodsec-skills):
- SQL: parameterized queries only; no string concatenation
- Command: no shell=True, os.system, or backtick exec with user input
- LDAP/XPath: escape special characters in filters
- Path traversal: canonicalize paths, reject ../
- Deserialization: no pickle/yaml.load()/eval on untrusted data
- Prototype pollution: no recursive merge of untrusted objects
- Validate at trust boundaries with allow-lists, not deny-lists
- Normalize Unicode and anchor regexes (^$); watch for ReDoS
- path: '**/*.{html,jsx,tsx,vue,svelte}'
instructions: |
Web security (prodsec-skills):
- No dangerouslySetInnerHTML or v-html with user data
- CSP: no unsafe-inline, no unsafe-eval
- CSRF tokens on state-changing requests
- Cookies: Secure, HttpOnly, SameSite=Strict
- No document.write, eval, new Function with user input
- GraphQL: depth/complexity limits, disable introspection in prod
- File uploads: validate by content magic, cap size, server-generate names
- XML: disable external entities (XXE), reject DTDs from untrusted sources
- path: '**/*{crypt,cipher,sign,hash,tls,ssl,cert,key,token}*'
instructions: |
Cryptographic security (prodsec-skills):
- Banned: MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode
- Symmetric: AES-256-GCM or ChaCha20-Poly1305
- Passwords: Argon2id (not bcrypt/scrypt for new code)
- Signing: Ed25519 or ECDSA P-256+
- Key exchange: X25519 or ECDH P-256+
- Constant-time comparison for all secret/token data
- Zeroize key material after use (no garbage-collector reliance)
- No custom crypto; use vetted libraries only
- Post-quantum: flag if protecting long-lived secrets
- path: '**/{Dockerfile,Containerfile}*'
instructions: |
Container security (prodsec-skills):
- Base image: UBI minimal or distroless from catalog.redhat.com
- Red Hat images: use floating tags (Red Hat manages updates);
non-RH images: pin by digest
- Multi-stage builds; no build tools in final image
- USER non-root; never run as root
- COPY specific files, not entire context
- No secrets in ENV, ARG, or COPY
- Read-only rootfs where possible
- No package manager cache in final layer
- HEALTHCHECK defined
- path: '**/*.{yaml,yml}'
instructions: |
If this is a Kubernetes/OpenShift manifest or Helm template:
- securityContext: runAsNonRoot, readOnlyRootFilesystem,
allowPrivilegeEscalation: false
- Drop ALL capabilities, add only what is required
- Resource limits (cpu, memory) on every container
- No hostPID, hostNetwork, hostIPC, privileged: true
- NetworkPolicy defined for the namespace
- OpenShift: SCC must be restricted or custom-scoped
- Liveness + readiness probes defined
- automountServiceAccountToken: false unless needed
- RBAC: least privilege; no cluster-admin for workloads
- Helm: no .Values interpolation in shell commands
- path: '**/{requirements*.txt,Pipfile*,pyproject.toml,package*.json,go.mod,go.sum,Cargo.toml,Gemfile*,pom.xml,build.gradle*}'
instructions: |
Supply chain security (prodsec-skills):
- New deps: justify need, check license compatibility
- Pin exact versions; verify hashes where supported
- Flag known CVEs (cross-ref osv.dev)
- No pre-release or yanked versions in production
- SBOM: ensure build produces provenance attestations
- Signing: artifacts signed with Sigstore/cosign
- path: .github/workflows/**/*
instructions: |
CI/CD security (prodsec-skills):
- Pin actions by full SHA, not tag
- No secrets in logs; mask sensitive outputs
- Least privilege: minimize GITHUB_TOKEN permissions
- No pull_request_target with checkout of PR head
- SAST/SCA steps in pipeline
- Sign artifacts with Sigstore/cosign
- Agentic CI actions: audit for prompt injection via
issue/PR title/body flowing into LLM prompts
- path: '**/{.claude,.vscode}/**/*'
instructions: |
HIGH RISK — IDE and AI tool configuration (prodsec-skills):
These directories can be supply chain attack vectors.
Review every change with a security and malware lens.
Flag the following for close scrutiny:
- .claude: hook commands, MCP server definitions, or
permissive tool-use policies in settings.json
- .claude: any scripts or binaries (e.g., .mjs, .sh)
- .vscode: tasks.json or launch.json entries that execute
code on folder open, build, or save
- .vscode: settings.json entries that auto-run formatters,
linters, or extensions with broad permissions
- Any change that grants broad filesystem or network access
- Obfuscated content or suspiciously large files
- Changes without a clear, legitimate purpose
- path: '**/*.go'
instructions: |
Go security (prodsec-skills):
- Never ignore error returns
- database/sql with placeholders; no fmt.Sprintf in queries
- Use stdlib crypto/* and golang.org/x/crypto (Go team maintained);
avoid third-party crypto libraries
- Integer overflow: bounds-check user-supplied sizes
- context.Context for cancellation and timeouts
- path: '**/*.{c,cpp,cc,h,hpp}'
instructions: |
C/C++ security (prodsec-skills):
- Banned: gets, sprintf, strcpy, strcat, strtok
- Use strlcpy, snprintf, bounded APIs
- Compile: -fstack-protector-strong -fPIE -pie
-D_FORTIFY_SOURCE=2 -Wformat-security
- Nullify pointers after free; no use-after-free
- Integer overflow: check arithmetic on untrusted sizes
- path: '**/{db,database,redis,cache,storage}/**/*'
instructions: |
Data store security (prodsec-skills):
- Auth: no default credentials; use IAM or IdP tokens
- Encryption: TLS in transit, encryption at rest
- Least privilege: app user has minimal grants
- Redis/ElastiCache: AUTH required, no KEYS in prod,
rename dangerous commands (FLUSHALL, CONFIG)
- Connection strings: no embedded credentials
# Source: defaults
abort_on_close: true
# Source: defaults
disable_cache: false
slop_detection:
# Source: Repository YAML (base)
enabled: true
# Source: Repository YAML (base)
label: coderabbit/ai-slop
auto_review:
# Source: Central YAML (inherited)
enabled: true
# Source: defaults
description_keyword: ''
# Source: defaults
auto_incremental_review: true
# Source: defaults
auto_pause_after_reviewed_commits: 5
# Source: Repository YAML (base)
ignore_title_keywords:
- WIP
- DNM
# Source: Repository YAML (base)
labels:
- '!work-in-progress'
# Source: Repository YAML (base)
drafts: false
# Source: defaults
base_branches: []
# Source: defaults
ignore_usernames: []
finishing_touches:
docstrings:
# Source: defaults
enabled: true
unit_tests:
# Source: defaults
enabled: true
simplify:
# Source: defaults
enabled: false
# Source: defaults
custom: []
pre_merge_checks:
# Source: defaults
override_requested_reviewers_only: false
docstrings:
# Source: Repository YAML (base)
mode: warning
# Source: Repository YAML (base)
threshold: 80
title:
# Source: Repository YAML (base)
mode: error
# Source: Repository YAML (base)
requirements: 'Use the imperative mood (e.g. "Add", "Fix", "Update"). Keep under 72 characters. Prefix with the affected component when scoped (e.g. "ovn-kubernetes: Fix EgressIP race condition").'
description:
# Source: Repository YAML (base)
mode: error
issue_assessment:
# Source: Repository YAML (base)
mode: error
# Source: Repository YAML (base), Central YAML (inherited)
custom_checks:
- mode: error
name: PR Quality
instructions: |-
Check the PR for overall quality in both description and size:
Description — must include the following sections:
1. "Why": A clear explanation of why this change is needed, the motivation or problem being solved. Must go beyond restating the title.
2. "What": A description of what the PR does, the approach taken, and any key design decisions.
3. "How to verify it" or "Testing": Which automated CI lanes or jobs in the CNO repo are running tests for this PR, and on what platforms (e.g. AWS, GCP, Azure, bare-metal, vSphere, SNO, HyperShift). Manual testing alone is not acceptable; tests must run in CI in an automated fashion.
4. For bug fixes: a description of the root cause and how the fix addresses it. Ideally include a link to the bug or issue.
5. For new features or behavioral changes: a description of the user-facing impact and any upgrade/rollback considerations.
Size — the PR must be reasonably scoped:
Fail if: - The description is empty, only contains the title, or is
missing any of the required sections (Why, What, How to
verify it) for non-trivial changes.
- The PR changes more than 7000 lines (additions +
deletions), or implements an entire large feature
end-to-end in a single PR touching many unrelated
components. Large PRs lead to shallow reviews and increase
the risk of bugs slipping through. Advise the author to
split into smaller, focused PRs scoped to individual
components or logical steps. Each PR should be
independently reviewable and mergeable.
Pass only if none of the above fail conditions are triggered, and either: the PR is a trivial change (typo fix, comment update, CI config only, vendor bump) with a self-explanatory title, or the PR is under 7000 lines with all required description sections present and all changes tightly related.
- mode: error
name: Commit Message Quality
instructions: |-
Check commit messages and structure in this PR for quality:
1. Each commit must be a logical, self-contained unit of change. Flag commits that bundle unrelated changes together.
2. Subject line should be concise and descriptive.
3. Subject should be prefixed with the affected component when scoped (e.g. "ovn-kubernetes: Fix EgressIP race").
4. Body must be present and explain why the change is needed, not just what was changed. The code diff already shows what.
5. Flag vague, uninformative, or generic messages like "fix", "update", "wip", "changes", "misc", or "address review comments" that provide no meaningful context. Review feedback should be squashed into the relevant logical commit, not added as a separate "address review comments" commit.
6. Flag overly verbose commit messages that read like AI-generated changelogs — listing every function name, variable, or line changed. Commit messages should explain intent at a high level, not narrate the diff.
7. No merge commits (e.g. "Merge branch 'master' into ..." or "Merge remote-tracking branch ..."). The author should rebase onto the target branch instead.
Fail if any commit violates any of the above points. Pass only if all commits satisfy all 7 criteria.
- mode: error
name: Unit Tests for Go Changes
instructions: |-
If Go source files (*.go) under pkg/ or cmd/ are added or modified (excluding vendor/ and *_test.go files), check whether corresponding *_test.go files are also added or modified in this PR. Exclude documentation-only, CI, or script changes.
Fail if production Go code changed with no test file changes. If failing, advise the author: if there is a valid reason for not including tests (e.g. existing tests already cover the change, the change is trivial, or testing is infeasible), document that justification in the PR description under "How to verify it" and then, once all other pre-merge checks are also addressed, use "`@coderabbitai` ignore pre-merge checks" to override.
Pass only if no production Go files changed, or if test files are included alongside the production changes.
- mode: error
name: E2E Tests for Feature Changes
instructions: |-
If Go source files (*.go) under pkg/ or cmd/ are added or modified in a way that adds new user-facing behavior, changes existing user-facing behavior, or fixes a bug (excluding vendor/, *_test.go, pure refactors with no behavioral change, and internal-only changes):
1. Check whether files under test/e2e/ are also added or modified in this PR.
2. The PR description must include a "Testing" or "How to verify it" section that describes which CI lanes or jobs are running tests for this PR, what platforms have coverage (e.g. AWS, GCP, Azure, bare-metal, vSphere, SNO, HyperShift), and whether the tests passed.
Fail if user-facing behavior changed or a bug was fixed and either criterion 1 or 2 (or both) is not satisfied. If failing, advise the author: if E2E tests are genuinely not feasible for this change, document the justification in the PR description under "How to verify it" and then, once all other pre-merge checks are also addressed, use "`@coderabbitai` ignore pre-merge checks" to override.
Pass only if no user-facing behavior changed and no bug was fixed, or if both E2E test files are included and the PR description contains the required testing context.
- mode: error
name: AI Config Authorship Restriction
instructions: |-
If the PR modifies .coderabbit.yaml, any **/AGENTS.md, or any **/ARCHITECTURE.md file, check whether the PR author's GitHub handle is listed in OWNERS_ALIASES as a core-approver or core-reviewer. These files control AI review behavior, agent instructions, and architectural documentation — changes from external contributors or non-SMEs risk degrading review quality or introducing security issues.
Fail if the PR author is not in OWNERS_ALIASES and the PR modifies .coderabbit.yaml, any **/AGENTS.md, or any **/ARCHITECTURE.md file. Advise the author that these files may only be changed by recognized reviewers or approvers listed in OWNERS_ALIASES.
Pass only if none of those files are modified, or the PR author is listed in OWNERS_ALIASES under core-approvers or core-reviewers.
- mode: warning
name: RBAC Least Privilege
instructions: |-
In YAML files under bindata/ and manifests/, if ClusterRole or Role rules are added or modified:
1. Flag any rule that uses a wildcard verb ("*") or wildcard resource ("*") unless explicitly justified.
2. Flag rules granting mutation verbs (create, update, patch, delete, deletecollection) and ask the author to justify why mutation access is needed. Read-only verbs (get, list, watch) are fine without justification.
Fail if any new RBAC rule uses wildcards without justification or grants mutation access without explanation.
Pass only if no RBAC rules are changed, or all new rules use specific verbs and resources with appropriate justification for any mutation access.
- mode: error
name: Docs for Feature and Behavior Changes
instructions: |-
If a PR introduces a new feature, changes user-facing behavior, fixes a significant bug that alters expected behavior, or modifies architecture or control flows, check whether files under docs/ are also added or modified.
DPU/DPF-related changes require elaborate documentation covering: the architectural impact (which components run on DPU vs host, what changes in the data path), new or changed configuration options (ConfigMap keys, environment variables, CLI flags), operational considerations (health monitoring, failure modes, troubleshooting), platform requirements, and any interaction with other features (egress IP, multicast, multi-network). Similarly, changes to IPsec must update docs/enabling_ns_ipsec.md, and changes to operand lifecycle or component boundaries must update docs/operands.md or docs/architecture.md.
Fail if the PR adds or changes user-facing behavior, architecture, or flows without any docs/ changes. For new features, a new markdown file must be added under docs/ describing the feature. The absence of existing documentation is not an excuse for not adding it — new features must create new docs. If failing, advise the author: if docs are genuinely not required for this change, document the reasoning in the PR description and then use "`@coderabbitai` ignore pre-merge checks" to override.
Pass only if the change is a minor bug fix, pure refactor, test-only change, CI/infra change, or internal-only change that does not affect user-facing behavior or architecture, or if relevant docs/ files are included.
- mode: error
name: Stale Project Docs and Config
instructions: |-
Check whether changes in this PR make any of the following project files stale or inaccurate: - .coderabbit.yaml (path instructions, pre-merge checks,
global review instructions)
- docs/** - **/AGENTS.md - **/ARCHITECTURE.md
Examples of staleness: 1. A file or directory is renamed, moved, or deleted but is still referenced by path in .coderabbit.yaml. 2. A code entity (CRD, flag, binary, config key) is renamed or removed but still referenced in docs. 3. Architecture or component boundaries change but docs/architecture.md is not updated. 4. A new operand is added but docs/operands.md is not updated. 5. DPU/DPF mode behavior changes but docs/ovn_node_mode.md is not updated.
Fail if any project docs or config become stale due to the changes. If failing, advise the author: update the affected docs/config files in this PR, or if the flagged staleness is a false positive, explain why in a PR comment and use "`@coderabbitai` ignore pre-merge checks" to override.
Pass only if the PR does not introduce any such staleness, or if the affected docs/config files are also updated in this PR.
- mode: warning
name: Go and Test Code Quality
instructions: |-
Check new or modified Go code (excluding vendor/) for these objective issues:
Production code (excluding *_test.go):
1. Flag use of fmt.Print, fmt.Println, log.Print, log.Fatal, or log.* for logging. The project uses klog exclusively.
2. Flag bare error returns (return err) without adding context. Errors should be wrapped with fmt.Errorf to add context. Use %w when the caller should be able to inspect the underlying error with errors.Is/errors.As. Use %v when the underlying error is an implementation detail that should not be exposed as part of the API.
3. Flag bare integers passed as time.Duration without explicit units. Must use e.g. 10*time.Second, not 10.
4. Flag err := inside nested blocks that shadow an outer err variable, silently losing errors.
5. Flag changes that only handle IPv4 without considering IPv6. Dual-stack (IPv4+IPv6) must always be considered.
6. Flag unprotected concurrent map or slice access. Shared state between goroutines must be guarded by locks.
Test code (pkg/**/*_test.go, cmd/**/*_test.go, test/e2e/**):
7. Flag bare t.Fatal(err) or t.Fatalf calls that only pass the error without describing what operation failed. Must include context, e.g. t.Fatalf("failed to render ovn: %v", err).
8. Flag time.Sleep in tests: prefer retry/poll loops or gomega Eventually/Consistently for async assertions.
9. Flag os.Setenv in tests: use t.Setenv which auto-restores on cleanup. This is already the established pattern in the CNO codebase.
Fail if any of the above 9 issues are found in new or modified code.
Pass only if none of the above issues are present.
- mode: error
name: AI-Generated Code Smell
instructions: |-
Flag signs of AI-generated code that was not properly reviewed by the author before submission:
1. Obvious comment slop: comments that merely restate the code they annotate (e.g. "// set a to b" above "a = b", "// return the error" above "return err", "// create the pod" above "CreatePod()"). These add no value and clutter the codebase.
2. Large blocks of unit tests that appear auto-generated and are unrelated to the functional changes in the PR. Flag if the PR adds hundreds of lines of tests for code that was not modified in this PR.
3. Overly verbose or boilerplate-heavy code that follows repetitive patterns suggesting template expansion rather than thoughtful implementation.
4. Comments or variable names that reference AI tools or prompts (e.g. "as suggested by", "generated by", "TODO: verify this AI suggestion").
Fail if any of the above 4 patterns are found in new or modified code.
Pass only if the code is clean, comments add genuine value, and test changes are proportional to the functional changes.
- mode: error
name: Stable and Deterministic Test Names
instructions: |
Ginkgo test names MUST be stable and deterministic. They must never contain dynamic
information that changes between runs.
Flag any test title (It(), Describe(), Context(), When(), etc.) that includes:
- Pod names with generated suffixes (e.g., "test-pod-abc123")
- Timestamps or dates
- Random UUIDs or generated identifiers
- Node names
- Namespace names with random suffixes
- IP addresses
- Any value that could change between test runs
Additionally, flag overly-specific test titles likely to change in
later development iterations, to avoid the need to specify mappings
after changing the name.
Test names should use descriptive, static strings that clearly indicate what
the test validates.
❌ Bad examples:
- `It("should create pod test-pod-xyz123 with custom security context")`
- `It(fmt.Sprintf("should run on node %s", nodeName))`
- `It("should create namespace " + ns.Name)`
- `It("should complete initialization within 30s")`
✅ Good examples:
- `It("should create a pod with custom security context")`
- `It("should schedule workloads to labeled nodes")`
- `It("should enforce network policy between namespaces")`
- `It("should complete initialization quickly")`
Dynamic values belong in test BODIES (assertions, setup), never in test TITLES.
- mode: warning
name: Test Structure and Quality
instructions: |
Review Ginkgo test code for these quality requirements:
1. **Single responsibility**: Each test (It block) should test one specific behavior.
Flag tests that assert multiple unrelated behaviors.
2. **Setup and cleanup**: Tests should use BeforeEach/AfterEach for setup and cleanup.
Flag tests that create resources without cleanup, especially cluster-scoped resources.
3. **Timeouts**: Operations that interact with the cluster (pod creation, deployments,
waiting for conditions) must include appropriate timeouts. Flag indefinite waits
or missing timeouts on Eventually/Consistently calls.
4. **Assertion messages**: Assertions should include meaningful failure messages
that help diagnose what went wrong.
❌ `Expect(err).NotTo(HaveOccurred())`
✅ `Expect(err).NotTo(HaveOccurred(), "failed to create test pod")`
5. **Consistency with codebase**: Tests should follow existing patterns in the
repository for how fixtures are created, how clients are obtained, and how
waits are structured.
- mode: warning
name: MicroShift Test Compatibility
instructions: |
When new Ginkgo e2e tests are added (It(), Describe(), Context(), When(), etc.),
check whether they use any APIs or features that are NOT available on MicroShift.
MicroShift is a single-node, minimal OpenShift distribution and does not support
all standard OpenShift APIs and features.
Note: The only OpenShift kube APIs available on MicroShift are Route and
SecurityContextConstraints. All other OpenShift-specific APIs are unavailable.
IMPORTANT: Do NOT flag a test if it is already protected from running on
MicroShift by any of these mechanisms:
- The test name includes a `[Skipped:MicroShift]` label
- The test name includes an `[apigroup:...]` tag for an API group not available
on MicroShift (e.g., `[apigroup:config.openshift.io]`,
`[apigroup:machine.openshift.io]`). The MicroShift CI jobs automatically skip
tests whose apigroup tag references an API group not served by MicroShift.
- The test body contains an `exutil.IsMicroShiftCluster()` check with `g.Skip()`
- The test is wrapped in a Describe/Context that already has one of the above
Flag the test if it references ANY of the following unavailable APIs or resources:
- Project / ProjectRequest (project.openshift.io) — use plain Namespaces instead
- Build / BuildConfig (build.openshift.io)
- DeploymentConfig (apps.openshift.io) — use Deployments instead
- ClusterOperator / ClusterOperators (config.openshift.io/v1)
- ClusterVersion / ClusterVersions (config.openshift.io/v1)
- Etcd operator or etcd pods (etcd.operator.openshift.io, openshift-etcd namespace)
- ClusterServiceVersion (CSV) / OLM resources (operators.coreos.com)
- MachineSet / Machine / MachineHealthCheck (machine.openshift.io)
- ClusterAutoscaler / MachineAutoscaler
- Console (console.openshift.io, openshift-console namespace)
- Monitoring stack components (prometheus-k8s, alertmanager, thanos-querier in openshift-monitoring)
- ImageRegistry operator (imageregistry.operator.openshift.io, openshift-image-registry namespace)
- Samples operator (samples.operator.openshift.io, openshift-cluster-samples-operator namespace)
- OperatorHub / CatalogSource / PackageManifest (operators.coreos.com, marketplace.redhat.com)
- CloudCredential / CredentialsRequest (cloudcredential.openshift.io)
- Storage operator (operator.openshift.io/v1 storage, openshift-cluster-storage-operator namespace)
- Network operator CRDs (operator.openshift.io/v1 network, openshift-network-operator namespace)
- Any other OpenShift API group besides Route (route.openshift.io) and
SecurityContextConstraints (security.openshift.io)
Flag the test if it references ANY of the following namespaces that do not exist on MicroShift:
- openshift-kube-apiserver
- openshift-kube-controller-manager
- openshift-kube-scheduler
Flag the test if it makes ANY of the following unsupported assumptions:
- Multi-node or HA assumptions (e.g., expecting multiple master/control-plane nodes,
pod anti-affinity across nodes, leader election across replicas)
- FeatureGate resources or TechPreviewNoUpgrade / CustomNoUpgrade feature sets
- Upgrade or update workflows (ClusterVersion-based upgrades)
- Node scaling (expecting nodes to be added or removed)
- Multi-replica deployments of control-plane components
If a test is flagged, recommend the following:
> **MicroShift compatibility notice:** This test uses APIs or features that are
> not available on MicroShift. If this repository's presubmit CI does not already
> include MicroShift jobs, please verify your test works on MicroShift by running
> an additional CI job:
>
> For parallel tests:
> `/payload-job periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance`
>
> For serial tests (test name contains `[Serial]`):
> `/payload-job periodic-ci-openshift-microshift-release-4.22-periodics-e2e-aws-ovn-ocp-conformance-serial`
>
> If the test is intentionally not applicable to MicroShift, there are
> several options:
>
> **Option 1 (preferred for API-specific tests):** Add an `[apigroup:...]` tag
> to the test name for the OpenShift API group being used. MicroShift CI jobs
> automatically skip tests whose apigroup is not served by MicroShift:
>
> ```go
> g.It("should report cluster operator status [apigroup:config.openshift.io]", func() { ... })
> ```
>
> **Option 2:** Add a `[Skipped:MicroShift]` label to the test name:
>
> ```go
> g.It("should do something [Skipped:MicroShift]", func() { ... })
> ```
>
> **Option 3:** Guard the test with a runtime platform check. In the
> `openshift/origin` repository, the common pattern is:
>
> ```go
> isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient())
> o.Expect(err).NotTo(o.HaveOccurred())
> if isMicroShift {
> g.Skip("Not supported on MicroShift")
> }
> ```
- mode: warning
name: Single Node OpenShift (SNO) Test Compatibility
instructions: |
When new Ginkgo e2e tests are added (It(), Describe(), Context(), When(), etc.),
check whether they make assumptions about multi-node or HA clusters. Single Node
OpenShift (SNO) runs the full OpenShift control plane and worker components on a
single node. Unlike MicroShift, SNO includes all standard OpenShift operators and
APIs, but any test that assumes multiple nodes will fail.
IMPORTANT: Do NOT flag a test if it is already protected from running on
SNO by any of these mechanisms:
- The test name includes a `[Skipped:SingleReplicaTopology]` label
- The test body contains an `exutil.IsSingleNode()` check with `g.Skip()`
- The test body contains a `skipOnSingleNodeTopology()` call
- The test body checks `infrastructure.Status.ControlPlaneTopology == configv1.SingleReplicaTopologyMode`
and skips accordingly
- The test is wrapped in a Describe/Context that already has one of the above
Flag the test if it makes ANY of the following multi-node or HA assumptions:
- Expects multiple control-plane/master nodes (e.g., counting master nodes > 1)
- Expects multiple worker nodes or schedules pods across distinct nodes
- Uses pod anti-affinity or topology spread constraints requiring multiple nodes
- Tests node-to-node communication patterns that require separate hosts
- Assumes leader election failover across multiple replicas on different nodes
- Expects pod rescheduling to a different node after node drain or failure
- Tests node scaling operations (adding or removing nodes)
- Assumes separate infra/worker/master node roles on different hosts
- Validates rolling updates that require scheduling to other nodes
- Tests ingress or load balancing behavior that depends on multiple endpoints
on different nodes
Do NOT flag tests that:
- Use OpenShift APIs and operators (these are all available on SNO)
- Run multiple pods on the same node
- Test single-pod behavior, even with multiple replicas (replicas can coexist on one node)
If a test is flagged, recommend the following:
> **Single Node OpenShift (SNO) compatibility notice:** This test assumes a
> multi-node cluster and may fail on Single Node OpenShift deployments. Please
> verify your test works on SNO by running an additional CI job:
>
> For parallel tests:
> `/payload-job periodic-ci-openshift-release-master-ci-4.22-e2e-aws-upgrade-ovn-single-node`
>
> For serial tests (test name contains `[Serial]`):
> `/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-single-node-serial`
>
> If the test is intentionally not applicable to SNO, there are several
> options:
>
> **Option 1:** Add a `[Skipped:SingleReplicaTopology]` label to the test name.
> SNO CI jobs automatically skip tests with this label:
>
> ```go
> g.It("should schedule pods across nodes [Skipped:SingleReplicaTopology]", func() { ... })
> ```
>
> **Option 2:** Guard the test with a runtime topology check. In the
> `openshift/origin` repo, use the `exutil.IsSingleNode()` utility:
>
> ```go
> isSingleNode, err := exutil.IsSingleNode(context.Background(), oc.AdminConfigClient())
> o.Expect(err).NotTo(o.HaveOccurred())
> if isSingleNode {
> g.Skip("Test requires multiple nodes and does not apply to single-node topologies")
> }
> ```
>
> This checks `infrastructure.Status.ControlPlaneTopology == configv1.SingleReplicaTopologyMode`
> which is the canonical way to detect SNO clusters.
>
> **Option 3:** Some test packages define a local `skipOnSingleNodeTopology()` helper:
>
> ```go
> func skipOnSingleNodeTopology(oc *exutil.CLI) {
> infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(
> context.Background(), "cluster", metav1.GetOptions{})
> o.Expect(err).NotTo(o.HaveOccurred())
> if infra.Status.ControlPlaneTopology == configv1.SingleReplicaTopologyMode {
> e2eskipper.Skipf("This test does not apply to single-node topologies")
> }
> }
> ```
>
> You can also use the `single_node.GetTopologies()` helper from
> `test/extended/single_node/topology.go` to get both control plane and
> infrastructure topology modes.
- mode: warning
name: Topology-Aware Scheduling Compatibility
instructions: |
When deployment manifests, operator code, or controllers are added or modified,
check whether they introduce scheduling constraints that assume a standard
highly-available (HA) topology with 3+ control-plane nodes and dedicated worker
nodes. OpenShift supports several topologies where these assumptions break:
- **Single Node OpenShift (SNO):** One node runs everything.
`ControlPlaneTopology = SingleReplica`, 1 schedulable node.
- **Two-Node Fixed (TNF):** Two control-plane nodes, no dedicated workers.
`ControlPlaneTopology = DualReplica`, 2 schedulable nodes.
Operators should run 2 replicas (not 1 as on SNO) to maintain HA.
- **Two-Node with Arbiter (TNA):** Two control-plane nodes plus a
resource-constrained arbiter node. The arbiter is NOT a master or worker
— it is a distinct role with label `node-role.kubernetes.io/arbiter` and
taint `node-role.kubernetes.io/arbiter:NoSchedule`. It only runs etcd
for quorum plus essential infrastructure (kubelet, SDN/OVN, MCD), and
may be as small as 2 cores / 8 GiB RAM. `ControlPlaneTopology =
HighlyAvailableArbiter`, but only 2 fully schedulable nodes.
- **External Control Plane (HyperShift):** Control plane runs outside the
hosted cluster. `ControlPlaneTopology = External`. There are NO nodes with
`node-role.kubernetes.io/master` or `node-role.kubernetes.io/control-plane`
labels in the hosted cluster.
The `ControlPlaneTopology` values are defined in `openshift/api` as the
`TopologyMode` type. Availability of each value depends on the release
version and enabled feature gates — check `openshift/api/features/features.go`
and the `FeatureGateAwareEnum` annotations on the `ControlPlaneTopology`
field for the target release:
- `HighlyAvailable` (default)
- `SingleReplica`
- `DualReplica` (feature gate: `DualReplica`)
- `HighlyAvailableArbiter`
- `External`
Do NOT flag changes that already check `ControlPlaneTopology`, node counts,
or topology labels before applying scheduling constraints.
Flag changes that introduce ANY of the following without topology-awareness:
- **Required pod anti-affinity with `maxUnavailable: 0`.**
(`requiredDuringSchedulingIgnoredDuringExecution` with
`topologyKey: kubernetes.io/hostname` combined with
`maxUnavailable: 0` in the rolling update strategy).
This combination deadlocks on ANY topology where replicas == schedulable
nodes (SNO, TNF, TNA, and even HA with 3 replicas on 3 nodes): the surge
pod cannot schedule (anti-affinity blocks it) and no old pod can be deleted
(maxUnavailable: 0 prevents it). Required anti-affinity is SAFE when paired
with `maxUnavailable >= 1` — this is the pattern used by most HA operators
(oauth-openshift, openshift-apiserver, image-registry, monitoring).
Preferred anti-affinity is NOT a safe alternative — it allows pods to
co-locate on the same node, defeating HA (see OCPBUGS-65984).
- **Pod topology spread constraints** with `whenUnsatisfiable: DoNotSchedule`
and a hostname topology key. Breaks on SNO and is problematic when the spread
`maxSkew` exceeds the number of schedulable nodes (TNF, TNA).
- **Replica count derived from node count** (e.g., counting control-plane nodes
and setting replicas to match) without considering that SNO and TNF have
fewer than 3 control-plane nodes, and TNA's arbiter node should not run
general workloads.
- **nodeSelector or node affinity targeting control-plane nodes**
(`node-role.kubernetes.io/master` or `node-role.kubernetes.io/control-plane`).
On HyperShift, no nodes carry these labels — pods will remain Pending
indefinitely.
- **Scheduling workloads to all control-plane nodes equally** without excluding
arbiter nodes. On TNA, the arbiter node has a separate taint
(`node-role.kubernetes.io/arbiter:NoSchedule`) that blocks general
workloads. However, operators that use **broad or wildcard tolerations**
(e.g., tolerating all `NoSchedule` taints) will inadvertently schedule to
the resource-constrained arbiter. Only etcd and essential infrastructure
pods (kubelet, SDN/OVN, machine-config-daemon) should run on the arbiter.
- **Assuming dedicated worker nodes exist.** On SNO and TNF, all workloads
run on control-plane nodes. Code that targets only worker nodes via
`node-role.kubernetes.io/worker` nodeSelector may need to also consider
nodes that carry both control-plane and worker roles.
- **PodDisruptionBudgets designed for 3+ nodes.** On TNF and TNA, only 2
nodes are fully schedulable. PDBs with `minAvailable: 2` on a 2-replica
deployment will block all voluntary disruptions (drains, upgrades).
PDBs should be reviewed for topology-appropriate values. Note: PDBs only
protect against the eviction API (`kubectl drain`). They do NOT protect
against `TaintManagerEviction` (node fencing, unreachable taints), which
directly deletes pods regardless of PDB settings.
If a change is flagged, recommend the following:
> **Topology-aware scheduling notice:** This change introduces scheduling
> constraints that may not work on all supported OpenShift topologies
> (SNO, Two-Node, HyperShift).
>
> OpenShift clusters vary in topology:
> | Topology | `ControlPlaneTopology` | Schedulable nodes |
> |---|---|---|
> | HA | `HighlyAvailable` | 3+ CP + N workers |
> | SNO | `SingleReplica` | 1 (all roles) |
> | Two-Node Fixed | `DualReplica` | 2 CP (no workers) |
> | Two-Node + Arbiter | `HighlyAvailableArbiter` | 2 CP + 1 arbiter (resource-limited) |
> | HyperShift | `External` | N workers (no CP nodes) |
>
> Please consider:
> - Checking `infrastructure.Status.ControlPlaneTopology` to detect
> `SingleReplica`, `DualReplica`, `HighlyAvailableArbiter`, or
> `External` topologies
> - Using required anti-affinity with `maxUnavailable >= 1` (not
> `maxUnavailable: 0`, which deadlocks). Preferred anti-affinity
> allows pods to co-locate, defeating HA.
> - Capping replica counts to the number of schedulable nodes
> - Excluding arbiter nodes (`node-role.kubernetes.io/arbiter`) from
> workload scheduling on TNA clusters
> - Avoiding `node-role.kubernetes.io/master` nodeSelectors on HyperShift,
> where no control-plane nodes exist in-cluster
> - For operators using the library-go `DeploymentController`, consider
> `WithTopologyAwareReplicasHook`, `WithTopologyAwareSchedulingHook`,
> and `WithControlPlaneNodeSelectorHook` from
> `library-go/pkg/operator/deploymentcontroller/`
>
> Verify with topology-specific CI jobs before merging:
> ```
> /payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-aws-upgrade-ovn-single-node
> /payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-ovn-hypershift
> /payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-arbiter
> /payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-techpreview
> ```
- mode: error
name: OTE Binary Stdout Contract
instructions: |
OpenShift Tests Extension (OTE) binaries communicate with `openshift-tests` via
JSON on stdout. Any non-JSON stdout from the main binary process corrupts the
test listing and breaks CI. Individual test cases (It, BeforeEach, AfterEach) are
fine — their stdout is intercepted by the framework.
Only flag stdout writes in process-level code: main(), init(), TestMain(),
BeforeSuite(), AfterSuite(), SynchronizedBeforeSuite(), RunSpecs() setup, or
top-level var/const initializers.
Common violations:
- klog writing to stdout (default behavior — must redirect to stderr via
`klog.SetOutput(os.Stderr)` or `klog.LogToStderr(true)`)
- fmt.Print/Println/Printf to stdout in main or suite setup
- Ginkgo v2 suite configuration that emits warnings to stdout
- log.SetOutput not set to stderr before any logging
Do NOT flag:
- Writes inside It(), BeforeEach(), AfterEach(), JustBeforeEach() blocks
- Writes to explicitly-opened files or buffers
- Writes to os.Stderr or GinkgoWriter
- mode: warning
name: IPv6 and Disconnected Network Test Compatibility
instructions: |
When new Ginkgo e2e tests are added (It(), Describe(), Context(), When(), etc.),
check whether they make assumptions about IPv4 networking or require connectivity
to external/public internet services. IPv6-only CI jobs run in disconnected
environments with no public internet access.
Flag the test if it contains ANY of the following IPv4 assumptions:
- Hardcoded IPv4 addresses (e.g., "10.0.0.1", "192.168.1.1", "172.16.0.0/12")
- Hardcoded IPv4 localhost ("127.0.0.1") where "::1" would also be needed
- Parsing or validating IPs assuming IPv4 format only (e.g., splitting on "." to parse octets)
- Creating Service or Endpoint objects with hardcoded IPv4 CIDRs or addresses
- Using net.ParseIP() or net.ParseCIDR() with hardcoded IPv4 values only
- Assuming pod or node IPs will be IPv4 (e.g., checking ip.To4() != nil without fallback)
- Hardcoded IPv4-only network policies (ipBlock with IPv4 CIDRs only)
- Building URLs by interpolating a variable host/IP without brackets for IPv6
(e.g., `fmt.Sprintf("http://%s:%d/path", host, port)` or `"http://" + host + ":" + port`).
Use `net.JoinHostPort(host, port)` instead, which adds brackets automatically for IPv6.
Flag the test if it requires ANY of the following external connectivity:
- Connections to public internet hosts (e.g., google.com, github.com, quay.io, registry.redhat.io)
- Pulling images from public registries without using a mirror or internal registry
- Downloading content from external URLs (curl, wget to public endpoints)
- DNS resolution of public hostnames
- Connections to external APIs or services outside the cluster
Do NOT flag tests that:
- Use cluster-internal service DNS names (e.g., service.namespace.svc.cluster.local)
- Use the cluster's own registry or image streams
- Dynamically detect IP family and adapt accordingly
If a test is flagged, recommend the following:
> **IPv6 and disconnected network compatibility notice:** This test may contain
> IPv4 assumptions or external connectivity requirements that will fail in IPv6-only
> disconnected environments. Please verify your test works on IPv6 by running
> an additional CI job:
>
> For parallel tests:
> `/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-ovn-ipv6`
>
> For serial tests (test name contains `[Serial]`):
> `/payload-job periodic-ci-openshift-release-master-nightly-4.22-e2e-metal-ipi-serial-ovn-ipv6`
>
> In the `openshift/origin` repo, use `GetIPAddressFamily()` to detect the
> cluster's IP family and adapt accordingly:
>
> ```go
> hasIPv4, hasIPv6, err := GetIPAddressFamily(oc)
> o.Expect(err).NotTo(o.HaveOccurred())
> if !hasIPv4 {
> // Use IPv6 addresses and CIDRs instead
> }
> ```
>
> Or use `GetIPFamilyForCluster()` to check the pod network IP family:
>
> ```go
> ipFamily := GetIPFamilyForCluster(oc.KubeFramework())
> if ipFamily == IPv6 {
> g.Skip("Test requires IPv4 connectivity")
> }
> ```
>
> You can also use the `InIPv4ClusterContext()` wrapper to automatically skip
> tests that only apply to IPv4 clusters:
>
> ```go
> InIPv4ClusterContext(oc, func() {
> // Test body - only runs on IPv4 clusters
> })
> ```
>
> For CIDRs, use `correctCIDRFamily()` to select the right CIDR for the cluster:
>
> ```go
> cidr := correctCIDRFamily(oc, "10.128.0.0/14", "fd01::/48")
> ```
>
> If the test requires external internet connectivity and cannot be adapted for
> disconnected environments, add `[Skipped:Disconnected]` to the test name to
> automatically skip it on disconnected clusters:
>
> ```go
> g.It("should fetch external content [Skipped:Disconnected]", func() { ... })
> ```
- mode: error
name: no-weak-crypto
instructions: |
Flag MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode usage.
Flag custom crypto implementations. Flag non-constant-time
comparison of secrets or tokens.
- mode: error
name: container-privileges
instructions: |
Flag privileged: true, hostPID, hostNetwork, hostIPC,
SYS_ADMIN capability, running as root without justification,
allowPrivilegeEscalation: true in container/K8s manifests.
- mode: error
name: no-sensitive-data-in-logs
instructions: |
Flag logging that may expose passwords, tokens, API keys,
PII (email, SSN, credit card), session IDs, internal
hostnames, or customer data.
tools:
ast-grep:
# Source: defaults
rule_dirs: []
# Source: defaults
util_dirs: []
# Source: Central YAML (inherited)
essential_rules: true
# Source: defaults
packages: []
shellcheck:
# Source: defaults
enabled: true
ruff:
# Source: defaults
enabled: true
markdownlint:
# Source: defaults
enabled: true
github-checks:
# Source: defaults
enabled: true
# Source: defaults
timeout_ms: 90000
languagetool:
# Source: defaults
enabled: true
# Source: defaults
enabled_rules: []
# Source: defaults
disabled_rules: []
# Source: defaults
enabled_categories: []
# Source: defaults
disabled_categories: []
# Source: defaults
enabled_only: false
# Source: defaults
level: default
biome:
# Source: defaults
enabled: true
hadolint:
# Source: Central YAML (inherited)
enabled: true
swiftlint:
# Source: defaults
enabled: true
phpstan:
# Source: defaults
enabled: true
# Source: defaults
level: default
phpmd:
# Source: defaults
enabled: true
phpcs:
# Source: defaults
enabled: true
golangci-lint:
# Source: defaults
enabled: true
yamllint:
# Source: defaults
enabled: true
gitleaks:
# Source: Central YAML (inherited)
enabled: true
trufflehog:
# Source: defaults
enabled: true
checkov:
# Source: Central YAML (inherited)
enabled: true
tflint:
# Source: defaults
enabled: true
detekt:
# Source: defaults
enabled: true
eslint:
# Source: defaults
enabled: true
flake8:
# Source: defaults
enabled: true
fbinfer:
# Source: defaults
enabled: true
# Source: defaults
enable_java: false
fortitudeLint:
# Source: defaults
enabled: true
rubocop:
# Source: defaults
enabled: true
buf:
# Source: defaults
enabled: true
regal:
# Source: defaults
enabled: true
actionlint:
# Source: Central YAML (inherited)
enabled: true
zizmor:
# Source: defaults
enabled: true
pmd:
# Source: defaults
enabled: true
clang:
# Source: defaults
enabled: true
cppcheck:
# Source: defaults
enabled: true
opengrep:
# Source: defaults
enabled: true
semgrep:
# Source: Central YAML (inherited)
enabled: true
circleci:
# Source: defaults
enabled: true
clippy:
# Source: defaults
enabled: true
sqlfluff:
# Source: defaults
enabled: true
trivy:
# Source: Central YAML (inherited)
enabled: true
prismaLint:
# Source: defaults
enabled: true
pylint:
# Source: defaults
enabled: true
oxc:
# Source: defaults
enabled: true
shopifyThemeCheck:
# Source: defaults
enabled: true
luacheck:
# Source: defaults
enabled: true
brakeman:
# Source: defaults
enabled: true
dotenvLint:
# Source: defaults
enabled: true
htmlhint:
# Source: defaults
enabled: true
stylelint:
# Source: defaults
enabled: true
checkmake:
# Source: defaults
enabled: true
osvScanner:
# Source: Central YAML (inherited)
enabled: true
oasdiff:
# Source: defaults
enabled: true
reactDoctor:
# Source: defaults
enabled: true
presidio:
# Source: defaults
enabled: true
blinter:
# Source: defaults
enabled: true
smartyLint:
# Source: defaults
enabled: true
emberTemplateLint:
# Source: defaults
enabled: true
skillspector:
# Source: defaults
enabled: true
psscriptanalyzer:
# Source: defaults
enabled: true
chat:
# Source: Central YAML (inherited)
art: false
# Source: defaults
allow_non_org_members: true
# Source: Central YAML (inherited)
auto_reply: true
integrations:
jira:
# Source: defaults
usage: auto
linear:
# Source: defaults
usage: auto
knowledge_base:
# Source: defaults
opt_out: false
web_search:
# Source: defaults
enabled: true
code_guidelines:
# Source: Central YAML (inherited)
enabled: true
# Source: Central YAML (inherited)
filePatterns:
- '**/AGENTS.md'
- '**/REDHAT.md'
- '**/CLAUDE.md'
- '**/CONTRIBUTING.md'
learnings:
# Source: defaults
scope: auto
# Source: defaults
approval_delay: 0
issues:
# Source: Central YAML (inherited)
scope: auto
jira:
# Source: defaults
usage: auto
# Source: defaults
project_keys: []
linear:
# Source: defaults
usage: auto
# Source: defaults
team_keys: []
pull_requests:
# Source: Central YAML (inherited)
scope: auto
mcp:
# Source: defaults
usage: auto
# Source: defaults
disabled_servers: []
# Source: defaults
automatic_repository_linking: false
# Source: defaults
linked_repositories: []
code_generation:
docstrings:
# Source: defaults
path_instructions: []
unit_tests:
# Source: defaults
path_instructions: []
issue_enrichment:
auto_enrich:
# Source: defaults
enabled: false
planning:
# Source: defaults
enabled: true
auto_planning:
# Source: defaults
enabled: true
# Source: defaults
labels: []
labeling:
# Source: defaults
labeling_instructions: []
# Source: defaults
auto_apply_labels: false
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 57481
Workflow-related settings
Built-in pre-merge checks
Notable implication
So the precise config-backed summary is:
There are also many repo-specific custom checks. The ones set to error include:
And these are warning only:
One caveat: the returned schema is empty, so I can confirm the active settings, but not use the schema to prove the exact GitHub UI behavior beyond those settings. So if I stick strictly to the fetched config:
If you want, I can also condense this into a one-paragraph answer specifically to the question: |
Contributor
|
@tssurya: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
Please see what custom checks are here: https://docs.coderabbit.ai/pr-reviews/custom-checks
See org level custom checks - 10 of them here: #3023 (comment)
We are allowed to override some of those but we chose to add 10 more for this specific repo. Total max for enterprise is 20 checks. So let's give this a try
Enable request_changes_workflow and configure both built-in pre-merge checks (title, description, docstrings, issue_assessment) and 12 custom checks covering: PR description quality, commit message quality, unit test and e2e test coverage, sensitive data in logs, RBAC least privilege, documentation for feature changes, stale docs detection, CNO-specific test quality, Go code quality, large PR detection, and AI-generated code smell detection.
These checks enforce consistent PR hygiene, ensure test and documentation coverage for behavioral changes, and catch common Go code quality issues specific to the CNO codebase.