Storage bucket: Surface non-retryable object delete error

[stepanstipl]

Release Note Template for Downstream PRs (will be copied)

storage: fixed bug where deleting a `google_storage_bucket` that contained non-deletable objects would retry indefinitely 

This PR fixes a bug when Storage Bucket delete operation is stuck in a never-ending loop when it can't delete objects - fixes hashicorp/terraform-provider-google#6034.

Please see the above-mentioned ticket on how to reproduce the issue.

With this PR in place, the terraform will give up when hitting a non-retryable error during objects delete (we will still try to delete the bucket in case we can):

google_storage_bucket.static-site: Destroying... [id=stepan-playground-tfdelete]

Error: could not delete non-empty bucket due to error when deleting contents: googleapi: Error 403: Object 'stepan-playground-tfdelete/test.txt' is under active Temporary hold and cannot be deleted, overwritten or archived until hold is removed., forbidden

I tried to add a test to cover this issue, but didn't find a way how to test Error on destroy with current Test() plumbing. If this is desirable I would appreciate help on how to best implement the test.

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@danawillow, please review this PR or find an appropriate assignee.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 6 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 1 file changed, 6 insertions(+), 6 deletions(-))

[modular-magician]

Oops! It looks like you're using an unknown release-note type in your changelog entries:

• REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

[danawillow]

Code LGTM. For a test, you could have one TestStep to get the config into the state such that deleting the bucket would error, and then a second TestStep that uses the same config but sets Destroy: true and ExpectError. If you do that, you'll want to have additional steps after that can "repair" it so that at the end of the test, the bucket is deleted (so we don't have dangling resources).

[stepanstipl]

Thanks for the review and your suggestion - it helped me to write the test, (I totally misunderstood what Destroy: true does 🤦‍♂). Test added.

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 57 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 2 files changed, 57 insertions(+), 6 deletions(-))

[danawillow]

Looks great, thanks!