-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
⚠️ Minimal Apply Support, Fix Up Client Options #435
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 |
---|---|---|
|
@@ -276,7 +276,7 @@ var _ = Describe("Client", func() { | |
Expect(cl).NotTo(BeNil()) | ||
|
||
By("creating the object (with DryRun)") | ||
err = cl.Create(context.TODO(), dep, client.CreateDryRunAll()) | ||
err = cl.Create(context.TODO(), dep, client.CreateDryRunAll) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{}) | ||
|
@@ -415,7 +415,7 @@ var _ = Describe("Client", func() { | |
}) | ||
|
||
By("creating the object") | ||
err = cl.Create(context.TODO(), u, client.CreateDryRunAll()) | ||
err = cl.Create(context.TODO(), u, client.CreateDryRunAll) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
actual, err := clientset.AppsV1().Deployments(ns).Get(dep.Name, metav1.GetOptions{}) | ||
|
@@ -1074,7 +1074,7 @@ var _ = Describe("Client", func() { | |
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("patching the Deployment with dry-run") | ||
err = cl.Patch(context.TODO(), dep, client.ConstantPatch(types.MergePatchType, mergePatch), client.PatchDryRunAll()) | ||
err = cl.Patch(context.TODO(), dep, client.ConstantPatch(types.MergePatchType, mergePatch), client.PatchDryRunAll) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("validating patched Deployment doesn't have the new annotation") | ||
|
@@ -1183,7 +1183,7 @@ var _ = Describe("Client", func() { | |
Kind: "Deployment", | ||
Version: "v1", | ||
}) | ||
err = cl.Patch(context.TODO(), u, client.ConstantPatch(types.MergePatchType, mergePatch), client.PatchDryRunAll()) | ||
err = cl.Patch(context.TODO(), u, client.ConstantPatch(types.MergePatchType, mergePatch), client.PatchDryRunAll) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("validating patched Deployment does not have the new annotation") | ||
|
@@ -2000,7 +2000,7 @@ var _ = Describe("Client", func() { | |
Describe("CreateOptions", func() { | ||
It("should allow setting DryRun to 'all'", func() { | ||
co := &client.CreateOptions{} | ||
client.CreateDryRunAll()(co) | ||
client.CreateDryRunAll(co) | ||
all := []string{metav1.DryRunAll} | ||
Expect(co.AsCreateOptions().DryRun).To(Equal(all)) | ||
}) | ||
|
@@ -2141,7 +2141,7 @@ var _ = Describe("Client", func() { | |
Describe("UpdateOptions", func() { | ||
It("should allow setting DryRun to 'all'", func() { | ||
uo := &client.UpdateOptions{} | ||
client.UpdateDryRunAll()(uo) | ||
client.UpdateDryRunAll(uo) | ||
all := []string{metav1.DryRunAll} | ||
Expect(uo.AsUpdateOptions().DryRun).To(Equal(all)) | ||
}) | ||
|
@@ -2157,19 +2157,25 @@ var _ = Describe("Client", func() { | |
Describe("PatchOptions", func() { | ||
It("should allow setting DryRun to 'all'", func() { | ||
po := &client.PatchOptions{} | ||
client.PatchDryRunAll()(po) | ||
client.PatchDryRunAll(po) | ||
all := []string{metav1.DryRunAll} | ||
Expect(po.AsPatchOptions().DryRun).To(Equal(all)) | ||
}) | ||
|
||
It("should allow setting Force to 'true'", func() { | ||
po := &client.PatchOptions{} | ||
client.PatchWithForce()(po) | ||
client.ForceOwnership(po) | ||
mpo := po.AsPatchOptions() | ||
Expect(mpo.Force).NotTo(BeNil()) | ||
Expect(*mpo.Force).To(BeTrue()) | ||
}) | ||
|
||
It("should allow setting the field manager", func() { | ||
po := &client.PatchOptions{} | ||
client.FieldOwner("some-owner")(po) | ||
Expect(po.AsPatchOptions().FieldManager).To(Equal("some-owner")) | ||
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. Looks like we are mixing owner/manager terms here. would be nice if we can be consistent here. 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. for posterity: manager is the term used by the kubernetes API, but I feel it's non-intuitive, so I picked fieldowner for the option name. We do that a couple of places. 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. We've had a few discussion, and "official" term is |
||
}) | ||
|
||
It("should produce empty metav1.PatchOptions if nil", func() { | ||
var po *client.PatchOptions | ||
Expect(po.AsPatchOptions()).To(Equal(&metav1.PatchOptions{})) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,8 +244,14 @@ type fakeStatusWriter struct { | |
client *fakeClient | ||
} | ||
|
||
func (sw *fakeStatusWriter) Update(ctx context.Context, obj runtime.Object) error { | ||
func (sw *fakeStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOptionFunc) error { | ||
// TODO(droot): This results in full update of the obj (spec + status). Need | ||
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. We can remove this TODO, since we have status client. |
||
// a way to update status field only. | ||
return sw.client.Update(ctx, obj) | ||
return sw.client.Update(ctx, obj, opts...) | ||
} | ||
|
||
func (sw *fakeStatusWriter) Patch(ctx context.Context, obj runtime.Object, patch client.Patch, opts ...client.PatchOptionFunc) error { | ||
// TODO(droot): This results in full update of the obj (spec + status). Need | ||
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. Same here. |
||
// a way to update status field only. | ||
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. Looks like I am inheriting something here :) |
||
return sw.client.Patch(ctx, obj, patch, opts...) | ||
} |
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 am a bit confused about -- does it patches the object or just the status of the object ?