Returning the latest resource from the Delete() call #114
Conversation
There was a problem hiding this comment.
See inline... we will need a change to ACK runtime first (the interface for AWSResourceManager needs to be updated before the templates here).
Also, please change the PR summary to reflect what this is doing (returning the latest resource from the Delete() call, not "Provide custom resource delete progress for long-running delete")
Thanks!
-jay
| // service API, returning an AWSResource representing the | ||
| // resource being deleted (if delete is asynchronous and takes time) | ||
| func (rm *resourceManager) Delete( | ||
| ctx context.Context, | ||
| res acktypes.AWSResource, | ||
| ) error { | ||
| ) (acktypes.AWSResource, error) { |
There was a problem hiding this comment.
you cannot change this method signature before changing the interface in ACK runtime:
There was a problem hiding this comment.
PR: aws-controllers-k8s/runtime#21 contained changes in runtime.
There was a problem hiding this comment.
Updated the PR summary as: "Returning the latest resource from the Delete() call"
jaypipes
added
the
do-not-merge/hold
kumargauravsharma
changed the title
Related issue: aws-controllers-k8s/community#836 This PR contains code changes to patch custom resource with latest details during long running delete. It does so if resource manager's Delete function return requeue related errors. The expectation is that resource manager's ReadOne method would set appropriate conditions (example: `ACK.ResourceSynced` to false with message) and provide latest details about the resource which is being deleted. Conditions can also be set at some common place where common conditions logic should reside. Testing: `make test` passed for runtime. **Alternate approach** that was considered, but was not taken up as it involved non-intrusive changes to resource manager's APIs. Following are the details: Currently, [resource manager's `Delete` API](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/manager.go.tpl#L136) returns only one data field of type `error`, it does not return `resource`. Also, the [`sdk.go::sdkDelete()` logic](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/sdk.go.tpl#L141) does not parse the return result output of service delete API. It can be updated such that `sdk.go::sdkDelete()` logic parses the output response, and allows a `sdk_delete_post_set_output` hook for service controllers to have custom logic (as needed). The resultant resource reference would then be returned from this method. In case, requeue is required then the return values from this method will have both resource, error (requeue type) as not nil. And the [reconciler:cleanup() logic here](https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L327), would patch the current resource with the returned resource from `rm.Delete()` call and return the error (if any). Though this change is similar to the approach taken for [resource manager ReadOne here](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/manager.go.tpl#L76-L78), this is more intrusive as it changes the Delete API's return type. However, it does have advantage that there is no need for another ReadOne call from reconciler, and also provides flexibility to service controller's to implement custom hook specific to delete scenario. [EDIT]: the alternate approach that is mentioned above, is the final approach taken per the discussion/comments in this PR. Related PR in code-generator: aws-controllers-k8s/code-generator#114
There was a problem hiding this comment.
OK, I cut https://github.com/aws-controllers-k8s/runtime/releases/tag/v0.5.0 with the changes to the AWSResourceManager.Delete interface method. Please update code-gen's go.mod file accordingly in this PR.
kumargauravsharma
force-pushed
the
delete-api-change
branch
from
5f27cdc to
cd470ce
Compare
|
Updated |
kumargauravsharma
force-pushed
the
delete-api-change
branch
from
cd470ce to
debab62
Compare
|
/lgtm |
|
@kumargauravsharma @jaypipes we also need to set the version to 0.5.0 in the Makefile https://github.com/aws-controllers-k8s/code-generator/blob/main/Makefile#L9 |
kumargauravsharma
force-pushed
the
delete-api-change
branch
from
debab62 to
f29b01f
Compare
|
@a-hilaly thanks. I have updated the Makefile. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, kumargauravsharma, surajkota 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 |
jaypipes
removed
the
do-not-merge/hold
5 participants
Issue #, if available:
aws-controllers-k8s/community#836
Description of changes:
This PR is to related to aws-controllers-k8s/runtime#21
It makes the code-generator changes for delete api as discussed under: aws-controllers-k8s/runtime#21 (comment)
Testing:
make testpassed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.