-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support UpdateStrategy and DeleteOption #99
Conversation
Signed-off-by: clyang82 <[email protected]>
Signed-off-by: clyang82 <[email protected]>
@@ -70,7 +70,7 @@ func (h resourceHandler) Patch(w http.ResponseWriter, r *http.Request) { | |||
func() (interface{}, *errors.ServiceError) { | |||
ctx := r.Context() | |||
id := mux.Vars(r)["id"] | |||
manifest, err := presenters.ConvertResourceManifest(patch.Manifest) | |||
manifest, err := presenters.ConvertResourceManifest(patch.Manifest, patch.DeleteOption, patch.UpdateStrategy) |
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 think we should support update these options.
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 you mean PATCH
these options? if so, it was covered. I am working on adding test cases for that.
@@ -406,6 +472,12 @@ func (o Resource) ToMap() (map[string]interface{}, error) { | |||
if !IsNil(o.Manifest) { | |||
toSerialize["manifest"] = o.Manifest | |||
} | |||
if !IsNil(o.DeleteOption) { | |||
toSerialize["delete_option"] = o.DeleteOption |
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.
delete_option or deleteOption?
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.
in restful api, it seems we should use delete_option
. for example: consumer_name
create_at
etc https://github.com/openshift-online/maestro?tab=readme-ov-file#get-your-resource
PropagationPolicy: workv1.DeletePropagationPolicyTypeForeground, | ||
}, | ||
Manifest: unstructured.Unstructured{Object: manifest}, | ||
DeleteOption: delOption, |
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.
@skeeey would you confirm deleteOption here works in agent side?
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.
what kinds of cases are you concerned about?
we already have some test in our integration test https://github.com/open-cluster-management-io/ocm/blob/main/test/integration/cloudevents/deleteoption_test.go
fixed: #69
Introduce to configure the
delete_option
andupdate_strategy
when creating or patching resource.delete_option defines the option to delete the resource. It is optional when creating a resource. The propagationPolicy of
delete_option
can be:Foreground
represents that the resource should be fourground deleted. This is a default value.Orphan
represents that the resource is orphaned when deleting the resource.update_strategy defines the strategy to update the resource. It is optional when creating a resource. The type of
update_strategy
can be:ServerSideApply
means to update resource using server side apply with work-controller as the field manager. This is a default value.Update
means to update resource by an update call.CreateOnly
means do not update resource based on current manifest.ReadOnly
means only check the existence of the resource based on the resource's metadata.