return nil if observed is nil from a successful delete #182
Conversation
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, surajkota, vijtrip2 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 |
| if ackcompare.IsNil(r) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Shouldn't this work just with a simple nil comparison? if r != nil { ... }
/cc @vijtrip2 @surajkota
There was a problem hiding this comment.
Yes, @a-hilaly is correct. We do not need the ackcompare.IsNil here. r is a *resource, which is a pointer type, not an interface.
There was a problem hiding this comment.
isNil method handles this case as well
There was a problem hiding this comment.
@surajkota no, the ackcompare.IsNil should not be used here.
|
@a-hilaly: GitHub didn't allow me to request PR reviews from the following users: surajkota. Note that only aws-controllers-k8s members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issue #, if available: Addresses #182 (comment) Testing: mode e2e test using sagemaker controller By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
5 participants
Issue #, if available:
aws-controllers-k8s/community#931
Description of changes:
Solution 2 in the issue linked above. If
rm.Deletedoes not return an error, and observed is also nil, returnnil, nilto the caller instead of the resource passed to the method.Testing:
make testManually using sagemaker controller
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.