docs: add comprehensive code review standards from 10 PRs#7397
docs: add comprehensive code review standards from 10 PRs#7397jhjaggars wants to merge 1 commit intoopenshift:mainfrom
Conversation
Add extensive coding standards to CONTRIBUTING.md based on maintainer feedback from 10 highly-reviewed PRs (6975, 6745, 7260, 7285, 6236, 7278, 5458, 6703, 5931, 4538). Standards captured from 8+ maintainers including csrwng, enxebre, JoelSpeed, muraee, jparrill, bryan-cox, celebdor, and others. New sections added to CONTRIBUTING.md: - Controller Code Review Standards (error handling, filtering, logging, code hygiene) - API Design Standards (enums vs bools, duration units, godoc, MinItems, immutability) - Status Update Patterns (Patch vs Update, meta.SetStatusCondition) - Code Organization (extract complex logic, constants together) - Breaking Changes (selector migration, required field rules, behavior changes) - Naming and Documentation (API vs Api, godoc prose, day 1/day 2 docs) - Condition Patterns (informative not blocking, Unknown state, consistency) - Testing (unit test validation, deterministic tests) - Performance (client reuse, guest cluster requests) Updated .coderabbit.yaml with path-specific instructions: - **/controllers/**/*.go - controller patterns, status updates, conditions - api/**/*.go - API design, naming, documentation, breaking change rules - cmd/**/*.go - feature flags, variable naming 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Please specify an area label 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhjaggars, muraee 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 configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
inheritance: false
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_instructions: ''
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
enable_prompt_for_ai_agents: true
path_filters: []
path_instructions:
- path: '**/controllers/**/*.go'
instructions: >
Follow .github/CONTRIBUTING.md "Controller Code Review Standards"
section.
Key checks:
- Non-retryable conditions (missing labels, wrong resource type) should
return nil, not error
- Check namespace labels
(hypershift.openshift.io/hosted-control-plane=true) early
- Validate references (non-nil, correct Kind) before fetching
- Only patch when there's an actual difference
- Use ctrl.LoggerFrom(ctx), not klog
- Use Events for user-visible state changes
- Use Patch instead of Update for status updates
- Use meta.SetStatusCondition() return value instead of DeepEqual
- Extract complex anonymous functions to helper functions
- Conditions should be informative, not blocking reconciliation
- Use Unknown for error states, not removal
- Reuse existing clients, don't recreate
- path: api/**/*.go
instructions: >
Follow .github/CONTRIBUTING.md "API Design Standards" section.
Key checks:
- Use enums instead of bools (bools don't evolve well)
- For durations, include unit in field name (e.g., delayAfterAddSeconds)
- Document limits and defaults in godoc (users can't see validations)
- Default in code, not API level
- Set MinItems=1 to prevent empty list issues
- Mark immutable fields with XValidation
- Use pointers for optional fields where empty string is valid
- Define condition constants in same file as condition types
- Use API not Api in field names (kubeAPICustomName not
kubeApiCustomName)
- Write godoc in prose, not bullet points
- Document day 1 and day 2 behavior
- Cannot add required fields to shipped APIs (breaking change)
- path: cmd/**/*.go
instructions: >
Follow .github/CONTRIBUTING.md "Controller Code Review Standards"
section.
Key checks:
- Feature flags should require all necessary config
- Clear variable names that reflect purpose
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
fortitudeLint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
issue_enrichment:
auto_enrich:
enabled: true
planning:
enabled: true
auto_planning:
enabled: true
labels: []
labeling:
labeling_instructions: []
auto_apply_labels: false
|
| - Always verify resource ownership before acting | ||
| - Check for expected annotations/labels before processing | ||
|
|
||
| ## API Design Standards |
There was a problem hiding this comment.
Rather than this section, shouldn't we just link to the OpenShift API Standards?
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
@jhjaggars: 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. |
|
I now have all the evidence. Here is the complete analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe job failed during the git clone/merge phase, before any CI steps or tests could execute. Prow attempted to merge PR #7397's head commit ( Root CausePR #7397 is a stale branch that has fallen behind
This is purely a branch-staleness problem — the PR branch has not been rebased onto current Recommendations
Evidence
|
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-bot: Closed this PR. 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 kubernetes-sigs/prow repository. |
5 participants
Summary
Add comprehensive coding standards to CONTRIBUTING.md based on maintainer feedback from 10 highly-reviewed PRs, capturing patterns from 8+ maintainers.
Changes
New Sections in CONTRIBUTING.md
Controller Code Review Standards (existing, enhanced)
API Design Standards (NEW)
Status Update Patterns (NEW)
Code Organization (NEW)
Breaking Changes (NEW)
Naming and Documentation (NEW)
Condition Patterns (NEW)
Testing (NEW)
Performance (NEW)
Updated .coderabbit.yaml
Added path-specific review instructions:
**/controllers/**/*.go- Enhanced with status update, condition, and performance patternsapi/**/*.go- NEW: API design standards, naming, documentation, breaking changescmd/**/*.go- Feature flags and variable namingSource PRs Analyzed
Benefits
For Contributors
For Maintainers
For CodeRabbit
Test Plan
Stats
🤖 Generated with Claude Code