⚠️ Minimal Apply Support, Fix Up Client Options #435
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12 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 |
k8s-ci-robot
added
approved
DirectXMan12
force-pushed
the
feature/minimal-apply
branch
2 times, most recently
from
21c61b5
to
c6745c7
Compare
| return sw.client.typedClient.UpdateStatus(ctx, obj, opts...) | ||
| } | ||
|
|
||
| // Patch implements client.Client |
There was a problem hiding this comment.
I am a bit confused about -- does it patches the object or just the status of the object ?
| 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.
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.
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.
We've had a few discussion, and "official" term is FieldManager, ManagedFields, etc ...
|
|
||
| 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 | ||
| // a way to update status field only. |
There was a problem hiding this comment.
Looks like I am inheriting something here :)
pkg/client/patch.go
Outdated
|
|
||
| // Type implements Patch. | ||
| func (p applyPatch) Type() types.PatchType { | ||
| // TODO(directxman12): when we update to 1.14, just use types.ApplyPatch |
There was a problem hiding this comment.
We are on 1.14 now right ?, so may be can update this.
| NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()). | ||
| Resource(o.resource()). | ||
| Name(o.GetName()). | ||
| SubResource("status"). |
There was a problem hiding this comment.
ok (I get it) server-side apply is implemented using patch on status sub-resource, little intuitive but I am sure they must have some reasons for it.
So that means, CRD needs to have status sub-resource enabled (I wonder if we should generate marker for status subresource by default or we already do ?)
There was a problem hiding this comment.
ok (I get it) server-side apply is implemented using patch on status sub-resource, little intuitive [clipped]
Not just status, all resources/sub-resources.
pkg/client/client_test.go
Outdated
| @@ -2262,7 +2268,7 @@ var _ = Describe("Patch", func() { | |||
| patch := client.MergeFrom(cm.DeepCopy()) | |||
|
|
|||
| By("returning a patch with type MergePatch") | |||
| Expect(patch.Type()).To(Equal(types.MergePatchType)) | |||
| Expect(patch.Type()).To(Equal(types.StrategicMergePatchType)) | |||
|
|
|||
| By("retrieving modifying the config map") | |||
| metav1.SetMetaDataAnnotation(&cm.ObjectMeta, annotationKey, annotationValue) | |||
There was a problem hiding this comment.
Do we need to add any tests for applyPatch ?
| @@ -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.
We can remove this TODO, since we have status client.
| } | ||
|
|
||
| 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 |
|
|
||
| // FieldManager is the name of the user or component submitting | ||
| // this request. It must be set with server-side apply. | ||
| FieldManager string |
There was a problem hiding this comment.
All of Create|Update|PatchOptions in upstream has FieldManager.
In CR, only PatchOptions has explicit FieldManager field. Should we make it consistent for all of Create|Update|PatchOptions?
DirectXMan12
force-pushed
the
feature/minimal-apply
branch
2 times, most recently
from
1d65338
to
b90dec0
Compare
This introduces a patch type for server-side apply, and the associated patch options.
This adds patch options related to server-side apply, and moves options to their own file.
This switches functional arguments with no variables to not taking arguments, meaning that they can be called like ```go r.Update(ctx, obj, client.UpdateDryRunAll) ``` instead of ```go r.Update(ctx, obj, client.UpdateDryRunAll()) ``` Options with arguments still get called as functions.
This adds .Status().Patch to the client for patching status, and makes sure that .Status().Update takes update options like normal update does.
DirectXMan12
force-pushed
the
feature/minimal-apply
branch
from
b90dec0
to
36db6ad
Compare
droot
added
the
lgtm
drop scaffold pkg from vendor
This fixes up client options (collapsing nillary functions to not require parens), makes sure that status update takes options, adds status patch, and adds a patch type and relevant options for server-side apply.