Skip to content
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

provider/aws: Make it possible to remove S3 bucket policy #8915

Merged
merged 4 commits into from
Sep 20, 2016

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 18, 2016

Due to a bug introduced in #8615 it is impossible to remove a policy as the policy field is marked as Computed and "existing-policy" => "" is always a no-op if the field is Computed.

There's actually already an acceptance test covering this and it was failing for the past 15 days after merging #8615 , just returning a slightly misleading error message - so I fixed that too in this PR.

I understand the reason for making this field Computed was:

  1. to expose policy which would be added/managed via aws_s3_bucket_policy after refresh
  2. to prevent aws_s3_bucket from deleting the policy which was set in the context of aws_s3_bucket_policy - i.e. prevent those resources from fighting with each other
  3. to allow import of bucket with policy

(2) can be still achieved without Computed - see my third commit.
(3) can be still achieved without Computed - see my last commit of this PR

I remember @stack72 and I discussed recently that there may be some plans in regards to overlapping/nested resources like these - but I don't know what these are - if there are any.

Either way this is now broken, so I think it needs fixing.

Test plan

make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSS3Bucket'

@jen20
Copy link
Contributor

jen20 commented Sep 20, 2016

Hi @radeksimko! At first blush this looks like a good set of fixes, I'll review the this morning and get them merged.

Copy link
Contributor

@jen20 jen20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me. There are currently acceptance test failures on:

  • TestAccAWSS3BucketNotification_importBasic
  • TestAccAWSS3Bucket_Notification
  • TestAccAWSS3Bucket_WebsiteRoutingRules
  • TestAccAWSS3Bucket_Lifecycle

However, none of these seem related, and fail in the same manner without these commits.

@jen20 jen20 merged commit c777827 into hashicorp:master Sep 20, 2016
@radeksimko radeksimko deleted the b-aws-s3-policy-fix branch September 20, 2016 15:52
@ghost
Copy link

ghost commented Apr 22, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants