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: Add aws_s3_bucket_policy resource #8615

Merged
merged 2 commits into from
Sep 2, 2016
Merged

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Sep 1, 2016

This commit adds a new "attachment" style resource for setting the policy of an AWS S3 bucket. This is desirable such that the ARN of the bucket can be referenced in an IAM Policy Document.

In addition, we now suppress diffs on the (now-computed) policy in the S3 bucket for structurally equivalent policies, which prevents flapping because of whitespace and map ordering changes made by the S3 endpoint.

@jen20
Copy link
Contributor Author

jen20 commented Sep 1, 2016

Just realised I forgot the docs, will add.

package aws

import (
//"encoding/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth removing?

@mitchellh
Copy link
Contributor

A few reviews, and a meta question: I have no problem with that lib being under your account, but I also wouldn't be against it just being a sub-library within the Terraform repo if it is fairly specialized to us. Either way.

This commit adds a new "attachment" style resource for setting the
policy of an AWS S3 bucket. This is desirable such that the ARN of the
bucket can be referenced in an IAM Policy Document.

In addition, we now suppress diffs on the (now-computed) policy in the
S3 bucket for structurally equivalent policies, which prevents flapping
because of whitespace and map ordering changes made by the S3 endpoint.
@jen20
Copy link
Contributor Author

jen20 commented Sep 2, 2016

I think over time (once we have more confidence in effectiveness across a wider range of policy types) we should bring it to be a helper library in the AWS provider. For now I think it's better to keep it vendored so it can be worked on separately without risking regression in Terraform?

@mitchellh
Copy link
Contributor

LGTM!

@kwilczynski
Copy link
Contributor

@mitchellh the change here that introduces Computed: true to the policy in S3 bucket seem to be causing this (on master):

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSS3Bucket_Policy'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/14 14:47:14 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_Policy -timeout 120m
=== RUN   TestAccAWSS3Bucket_Policy
--- FAIL: TestAccAWSS3Bucket_Policy (52.19s)
        testing.go:265: Step 1 error: Check failed: Check 2/2 error: unexpected end of JSON input
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    52.206s
make: *** [testacc] Error 1

Either the test or the ReadFunc needs to be updated, I haven't looked into this yet.

@ghost
Copy link

ghost commented Apr 11, 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 11, 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.

3 participants