-
Notifications
You must be signed in to change notification settings - Fork 280
chore: Handle deletion when the underlying GCP resource does not exist #3826
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
chore: Handle deletion when the underlying GCP resource does not exist #3826
Conversation
// and (false, nil) if the object was not found but should be presumed deleted. | ||
// In an error, the state is not fully determined - a delete might be in progress. | ||
// It returns (true, nil) if the object was deleted without error. | ||
// In an error (false, err), the state is not fully determined - a delete might be in progress. | ||
Delete(ctx context.Context, op *DeleteOperation) (deleted bool, err 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.
I feel the returned deleted bool
might be unnecessary, but I'm open to discussion:
- it's not used anywhere for now:
if _, err := adapter.Delete(ctx, deleteOp); err != nil { - the situation
return (false, nil) if the object was not found but should be presumed deleted
, we could callFind
to determine the existence of the GCP obj, to avoid unnecessary Delete calls. Also We don't have specific handling for the case of (false, nil); instead, we treat it the same as (true, nil).
This can be called without calling Find.
Unsure if there's a use case of it I haven't considered...
080046e
to
26bab12
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
...re/testdata/basic/secretmanager/v1beta1/secretmanagersecretversion/_vcr_cassettes/nontf.yaml
Outdated
Show resolved
Hide resolved
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.
/lgtm
38746e9
to
b8f0e35
Compare
New changes are detected. LGTM label has been removed. |
b8f0e35
to
ae14ea7
Compare
Discussed with Joyce offline, I'll handle this issue in #4621 |
Change description
Fixes #3727
Tests you have done
make ready-pr
to ensure this PR is ready for review.