-
Notifications
You must be signed in to change notification settings - Fork 74
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
✨ implement patch function in ManifestWorkAgentClient #271
✨ implement patch function in ManifestWorkAgentClient #271
Conversation
90349c1
to
237f2ac
Compare
/assign @qiujian16 |
func (c *ManifestWorksAgentClient) Create(ctx context.Context, manifestWork *workv1.ManifestWork, opts metav1.CreateOptions) (*workv1.ManifestWork, error) { | ||
klog.Fatal("Create function for ManifestWorksAgentClient is unsupported") | ||
func (c *ManifestWorkAgentClient) Create(ctx context.Context, manifestWork *workv1.ManifestWork, opts metav1.CreateOptions) (*workv1.ManifestWork, error) { | ||
klog.Fatal("Create function for ManifestWorkAgentClient is unsupported") |
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.
fatal seems not appropriate, should we just return not supported error here/.
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.
return MethodNotSupported error
cloudevents/work/utils/utils.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
case types.StrategicMergePatchType: |
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.
StrategicMergePatchType is not supported in CRD.
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.
do we need handle the crd here? we patch a work here, if a work has a crd, we will stop here? or the caller should not use this type when they patch their works that contain crds?
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.
and it seems to be difficult for us to distinguish pacth a work or patch the crds in works
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.
no I mean resource served by crd, in this case ManifestWork does not support StrategicMergePatchType
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.
OK, remvoe the StrategicMergePatchType support
eventDataType, err := types.ParseCloudEventsDataType(patchedWork.Annotations[codec.CloudEventsDataTypeAnnotationKey]) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
eventType := types.CloudEventsType{ | ||
CloudEventsDataType: *eventDataType, | ||
SubResource: types.SubResourceStatus, |
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.
should we set SubResource after checking the sub 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.
we don't need, we just send the work status to source here (deletion is also a status update for a source)
// send resync request to fetch manifestworks from source when the ManifestWorkInformer status | ||
if err := c.cloudEventsClient.Resync(ctx); err != nil { | ||
return nil, err | ||
if IsStatusUpdate(subresources) { |
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.
if it is not status subresource we should return error
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.
return the error at the end
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 mean it could be either patch the finalizer (patch spec) or patch status. We can not patch other sub resources.
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.
return the error if the patch is not for status or finalizers and allow update finalizers(add/remvoe)
c6f518c
to
0943e5f
Compare
Signed-off-by: Wei Liu <liuweixa@redhat.com>
0943e5f
to
b18538a
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey 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 |
14fa3e0
into
open-cluster-management-io:main
Summary
implement path function in ManifestWorkAgentClient