CORENET-7266: Add path-specific review instructions to CodeRabbit config#3039
CORENET-7266: Add path-specific review instructions to CodeRabbit config#3039kyrtapz wants to merge 1 commit into
Conversation
WalkthroughExpands ChangesCodeRabbit Review Instruction Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kyrtapz 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.
Actionable comments posted: 2
🤖 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 @.coderabbit.yaml:
- Around line 58-71: The
`networkoperator.openshift.io/generates-operator-status` entry is documented
under the wrong metadata type in `.coderabbit.yaml`; it should be described as a
label, not an annotation. Update the guidance to match `pkg/names/names.go`, and
call out that `StatusController` only recognizes it when the label has a
non-empty value, so reviewers know to verify both the label placement and its
value.
- Around line 190-195: The guidance in the configuration comment is reversed for
non-pointer booleans: `false` only becomes ambiguous when `omitempty` can drop
the zero value, not when the field is always serialized. Update the wording near
the SSA/merge explanation to say that non-pointer booleans need special handling
when `omitempty` is present, and keep `managementState` called out separately as
a special case that is always serialized but still requires preserving the live
value via merge logic.
🪄 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: 0e8043da-19f1-402b-a7ab-4b88792c185e
📒 Files selected for processing (1)
.coderabbit.yaml
Teach CodeRabbit about CNO-specific pitfalls drawn from past bugs and reviewer experience: managed/self-hosted manifest drift, missing release.openshift.io/version annotations, cluster-name annotation for HyperShift, OVN-K rollout sequencing, SSA merge gotchas, missing controller watches, and namespace node-selector requirements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kyrtapz
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@kyrtapz: This pull request references CORENET-7266 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. |
| and `ibm-cloud-managed` where applicable). Missing annotations cause the | ||
| resource to be skipped on certain profiles. | ||
| - Do NOT put NetworkPolicy or resources that depend on CNO being running | ||
| into `manifests/`. CVO applies these before CNO is ready, which can cause |
There was a problem hiding this comment.
This isn't quite correct; we deploy the CNO default-deny NetworkPolicy from manifests/.
A better rule would be that you need to be very careful when deploying things to the openshift-network-operator namespace from bindata/, or to anything except the openshift-network-operator namespace from manifests/
| - `network.operator.openshift.io/cluster-name` — specifies which cluster a | ||
| resource belongs to in HyperShift (management vs guest). CNO uses this to | ||
| select the correct API client for applying. Wrong value = resource applied | ||
| to wrong cluster. Must be set correctly on all HyperShift-aware resources. |
There was a problem hiding this comment.
I don't know whether coderabbit will understand what "all HyperShift-aware resources" means, but I don't.
| - `networkoperator.openshift.io/non-critical` — marks non-critical resources | ||
| that don't block upgrades. |
There was a problem hiding this comment.
| - `networkoperator.openshift.io/non-critical` — marks non-critical resources | |
| that don't block upgrades. | |
| - `networkoperator.openshift.io/non-critical` — marks resources that should | |
| not block the operator from becoming `Available` during cluster install, | |
| because they depend on other operators. |
| hash computation or use a different restart mechanism. | ||
|
|
||
| **CLI flags:** | ||
| - After upstream OVN-K syncs or rebases, audit for duplicated or obsolete |
There was a problem hiding this comment.
There are no "upstream OVN-K rebases" in this repo
| `merge.go` has special-case merge functions (e.g. for | ||
| `disableNetworkDiagnostics`, `managementState`). If adding a new | ||
| user-settable field without `omitempty` or that is not a pointer, | ||
| a merge function is likely needed. |
There was a problem hiding this comment.
No, we should just not add any more such fields. If coderabbit sees us trying to work around this problem in a new API, it should tell us that the API is wrong and we need to fix it.
|
@kyrtapz: 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. |
3 participants
Teach CodeRabbit about CNO-specific pitfalls drawn from past bugs and reviewer experience: managed/self-hosted manifest drift, missing release.openshift.io/version annotations, cluster-name annotation for HyperShift, OVN-K rollout sequencing, SSA merge gotchas, missing controller watches, and namespace node-selector requirements.