-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 DeepCopy unstructured objects in helper before patching #1464
🐛 DeepCopy unstructured objects in helper before patching #1464
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @ncdc |
Would you mind opening an issue? |
/hold |
This might not be the issue we're seeing with @CecileRobertMichon |
c509ce8
to
22a6fe9
Compare
243b93a
to
e70e058
Compare
@@ -138,6 +140,36 @@ func TestHelperPatch(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
{ | |||
name: "Only add ownerref update to unstructured object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes even without your code change. Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new one that actually tests this behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Both should be valid tests though)
resource = resource.DeepCopyObject() | ||
} | ||
|
||
// Convert the resource to unstructured for easier comparison later. | ||
before, err := runtime.DefaultUnstructuredConverter.ToUnstructured(resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a point in doing the unstructured conversion here if it is already Unstructured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ToUnstructured
gets the inner .Object
map of the actual object, I kept this to avoid breaking the logic
e70e058
to
d50b35c
Compare
Signed-off-by: Vince Prignano <vincepri@vmware.com>
d50b35c
to
ea23897
Compare
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@vincepri I'm not sure this will fix patching the owner ref on the infra resource (e.g. AWSCluster)? |
@ncdc This should fix the issue that external objects don't get an OwnerReference. The external objects are unstructured, which is how we found the bug in the first place. Is there anything I missed there? 🙂 |
The external object ownerRef is set with |
Nevermind. old code locally |
/lgtm |
Signed-off-by: Vince Prignano vincepri@vmware.com
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1465