OCPBUGS-36246: Improve resource merge diffs#1409
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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 (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughA new exported ChangesManifestDiff helper introduction and adoption
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 2uasimojo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci-robot
added
jira/valid-bug
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@2uasimojo: This pull request references Jira Issue OCPBUGS-36246, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![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 `@lib/resourceapply/interface.go`:
- Around line 33-36: The allow-list filter in the switch statement (checking
parts[1] against "Name", "Namespace", "Labels", "Annotations",
"OwnerReferences") fails when paths contain bracket notation or array indices
like `Labels["key"]` or `OwnerReferences[0].Name`. Extract the base field name
from parts[1] by removing any content after the first bracket character (such as
"[") before performing the switch case comparison, so that paths with bracket
notation or array indices still match the allow-listed field names and are not
incorrectly filtered out.
In `@lib/resourceapply/security.go`:
- Around line 39-41: The ManifestDiff comparison on line 40 is comparing
`original` vs `existing`, but the actual Update call on line 47 is updating with
`reconcile`. This mismatch can hide real changes in logs. Change the
ManifestDiff call to compare `&required` vs `existing` or the appropriate
objects that reflect what is actually being updated with the reconcile object,
ensuring the diff accurately shows what changes will be applied.
🪄 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: c20f1549-1bc8-44a1-ae3f-7b75c3047348
📒 Files selected for processing (13)
lib/resourceapply/admissionregistration.golib/resourceapply/apiext.golib/resourceapply/apps.golib/resourceapply/batch.golib/resourceapply/core.golib/resourceapply/cv.golib/resourceapply/imagestream.golib/resourceapply/interface.golib/resourceapply/operators.golib/resourceapply/rbac.golib/resourceapply/security.gopkg/cvo/internal/generic.gopkg/cvo/internal/operatorstatus.go
Resource merges -- where CVO asserts the shape of "owned" resources based on manifests from the release payload -- were trying to display useful information when an update was triggered. However: - In some places, we were comparing objects *after* syncing, so the output would never show the discrepancy that actually triggered the update. - We were using `cmp.Diff()` without any filters, so the diff would always show mismatches in Metadata like UID, CreationTimestamp, etc. Here we fix the instances of the former; and address the latter via a helper that filters (ignores): - All TypeMeta, since by the time we get to this code, we're already sure the GVKs are correct (but sometimes the values from the manifest side can be blank?). - Status - ObjectMeta fields not explicitly synced by EnsureObjectMeta() Signed-off-by: Eric Fried <efried@redhat.com>
2uasimojo
force-pushed
the
OCPBUGS-36246/ffdc-resourcemerge-diff
branch
from
c916190 to
4dbfdec
Compare
| } | ||
| } | ||
| // Any other ObjectMeta (IDs, versions, timestamps, ...) we don't care about | ||
| return true |
There was a problem hiding this comment.
I'm slightly concerend about the things-we-care-about list approach. If the list goes stale, we could fail to log a change we do care about. If instead we had a things-we-don't-care-about list, the risk of the list going stale is just distracting output when we're actually updating a resource, and that seems like a lesser problem (the logs are there, complaining about some field we don't care about changing, and we can use that to extend our things-we-don't-care-about list).
There was a problem hiding this comment.
Ack, that makes sense. I chose this list because it’s shorter and already explicitly enumerated (via EnsureObjectMeta) but I agree it’s better to err such that later changes produce false positives vs (opposite kind of changes) mask true ones. Will fix.
|
@2uasimojo: 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
Resource merges -- where CVO asserts the shape of "owned" resources based on manifests from the release payload -- were trying to display useful information when an update was triggered. However:
cmp.Diff()without any filters, so the diff would always show mismatches in Metadata like UID, CreationTimestamp, etc.Here we fix the instances of the former; and address the latter via a helper that filters (ignores):
Summary by CodeRabbit