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
🐛Remove owner passed in to RemoveControllerReference #2595
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -181,6 +181,7 @@ var _ = Describe("Controllerutil", func() { | |||||||
Expect(controllerutil.RemoveOwnerReference(obj, rs, scheme.Scheme)).To(HaveOccurred()) | ||||||||
Expect(rs.GetOwnerReferences()).To(HaveLen(1)) | ||||||||
}) | ||||||||
|
||||||||
It("should error when trying to remove an owner that doesn't exist", func() { | ||||||||
rs := &appsv1.ReplicaSet{ | ||||||||
ObjectMeta: metav1.ObjectMeta{}, | ||||||||
|
@@ -229,6 +230,22 @@ var _ = Describe("Controllerutil", func() { | |||||||
Expect(controllerutil.RemoveControllerReference(dep, rs, scheme.Scheme)).To(HaveOccurred()) | ||||||||
}) | ||||||||
|
||||||||
It("should error when RemoveControllerReference passed in owner is not the owner", func() { | ||||||||
rs := &appsv1.ReplicaSet{ | ||||||||
ObjectMeta: metav1.ObjectMeta{}, | ||||||||
} | ||||||||
dep := &extensionsv1beta1.Deployment{ | ||||||||
ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "foo-uid-2"}, | ||||||||
} | ||||||||
dep2 := &extensionsv1beta1.Deployment{ | ||||||||
ObjectMeta: metav1.ObjectMeta{Name: "foo-2", UID: "foo-uid-42"}, | ||||||||
} | ||||||||
Expect(controllerutil.SetControllerReference(dep, rs, scheme.Scheme)).ToNot(HaveOccurred()) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Wondering if that would add useful coverage. Basically we error out because the owner we want to remove is only owner but not the controller (which is exactly the case we missed before, I think) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it exists, but the owner we passed in would still error because it doesn't have the controller == true. Makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thx! |
||||||||
Expect(controllerutil.SetOwnerReference(dep2, rs, scheme.Scheme)).ToNot(HaveOccurred()) | ||||||||
Expect(controllerutil.RemoveControllerReference(dep2, rs, scheme.Scheme)).To(HaveOccurred()) | ||||||||
Expect(rs.GetOwnerReferences()).To(HaveLen(2)) | ||||||||
}) | ||||||||
|
||||||||
It("should not error when RemoveControllerReference owner's controller is set to true", func() { | ||||||||
rs := &appsv1.ReplicaSet{ | ||||||||
ObjectMeta: metav1.ObjectMeta{}, | ||||||||
|
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.
WDYT about adding additional test cases for the cases which weren't captured by the existing tests?
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 can do that. My work here still doesn't show that the owner reference that exists is in fact the actual controller Reference that is being removed.
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 test I added was to show that this errors when passing in the wrong owner. @sbueringer