Skip to content
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.

Sign up for GitHub

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

Jump to bottom

⚠️ Minimal Apply Support, Fix Up Client Options #435

Merged
merged 4 commits into from
May 17, 2019
Merged

⚠️ Minimal Apply Support, Fix Up Client Options #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc64d91
Server Side Apply Patch Type
DirectXMan12 May 17, 2019
5e7ec24
Add server-side apply options
DirectXMan12 May 17, 2019
f4eaa7b
Switch functional options with no args to vars
DirectXMan12 May 17, 2019
36db6ad
Add PatchStatus, options to UpdateStatus
DirectXMan12 May 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,19 @@ type statusWriter struct {
var _ StatusWriter = &statusWriter{}

// Update implements client.StatusWriter
func (sw *statusWriter) Update(ctx context.Context, obj runtime.Object) error {
func (sw *statusWriter) Update(ctx context.Context, obj runtime.Object, opts ...UpdateOptionFunc) error {
_, ok := obj.(*unstructured.Unstructured)
if ok {
return sw.client.unstructuredClient.UpdateStatus(ctx, obj)
return sw.client.unstructuredClient.UpdateStatus(ctx, obj, opts...)
}
return sw.client.typedClient.UpdateStatus(ctx, obj)
return sw.client.typedClient.UpdateStatus(ctx, obj, opts...)
}

// Patch implements client.Client
Copy link
Contributor

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 ?

func (sw *statusWriter) Patch(ctx context.Context, obj runtime.Object, patch Patch, opts ...PatchOptionFunc) error {
_, ok := obj.(*unstructured.Unstructured)
if ok {
return sw.client.unstructuredClient.PatchStatus(ctx, obj, patch, opts...)
}
return sw.client.typedClient.PatchStatus(ctx, obj, patch, opts...)
}
22 changes: 14 additions & 8 deletions pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down Expand Up @@ -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{})
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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))
})
Expand Down Expand Up @@ -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))
})
Expand 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"))
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link

@apelisse apelisse Jun 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've had a few discussion, and "official" term is FieldManager, ManagedFields, etc ...

})

It("should produce empty metav1.PatchOptions if nil", func() {
var po *client.PatchOptions
Expect(po.AsPatchOptions()).To(Equal(&metav1.PatchOptions{}))
Expand Down
10 changes: 8 additions & 2 deletions pkg/client/fake/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

// a way to update status field only.
Copy link
Contributor

Choose a reason for hiding this comment

The 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...)
}
4 changes: 2 additions & 2 deletions pkg/client/fake/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ var _ = Describe("Fake client", func() {
Namespace: "ns2",
},
}
err := cl.Create(nil, newcm, client.CreateDryRunAll())
err := cl.Create(nil, newcm, client.CreateDryRunAll)
Expect(err).To(BeNil())

By("Getting the new configmap")
Expand All @@ -193,7 +193,7 @@ var _ = Describe("Fake client", func() {
"test-key": "new-value",
},
}
err := cl.Update(nil, newcm, client.UpdateDryRunAll())
err := cl.Update(nil, newcm, client.UpdateDryRunAll)
Expect(err).To(BeNil())

By("Getting the new configmap")
Expand Down
Loading