-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Refactor s3 acceptance tests to run in multiple partitions. #4466
Conversation
9e34f6c
to
7204d26
Compare
7204d26
to
38f40c6
Compare
38f40c6
to
e4a57a8
Compare
e4a57a8
to
24646e8
Compare
24646e8
to
c475586
Compare
c475586
to
e80abe8
Compare
@bflad: most s3 tests are passing in govcloud with this patch. Have time to take a look? |
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.
Great work so far! Left some initial comments below -- let me know what you think.
aws/resource_aws_s3_bucket_test.go
Outdated
partition := testAccGetPartition() | ||
|
||
if partition == "aws-us-gov" { | ||
t.Skip("skipping replication tests; govcloud only includes a single region") |
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.
We should probably implement this as a PreCheck
function (testAccMultipleRegionsPreCheck(t)
?) rather than duplicating the logic multiple places. This is especially relevant as it will pop up in other services where we have replication testing as well, like RDS.
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.
In fact further down the road, coupled with a new aws_regions
data source that pulls regions from the endpoints
SDK package, we could have these automatically start running if/when the endpoints
SDK package gets updated with additional regions in the GovCloud partition. 😉
@@ -364,7 +366,7 @@ resource "aws_sns_topic" "topic" { | |||
"Effect": "Allow", | |||
"Principal": {"AWS":"*"}, | |||
"Action": "SNS:Publish", | |||
"Resource": "arn:aws:sns:*:*:%s", | |||
"Resource": "arn:%s:sns:*:*:%s", |
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.
Rather than requiring this new partition
string to be passed around as function inputs, which gets messy, I think we should just add a consistently named aws_partition
data source anywhere its required, e.g. ${data.aws_partition.current.partition}
. This applies to all the testing configurations that have these manually constructed ARNs. 👍
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.
Good call--I'll use the aws_partition
data source where I can. There are a few places where it looks like we'll need to look up partition directly, like in https:/terraform-providers/terraform-provider-aws/pull/4466/files/e80abe873bb68f23cce3ae545e44db2f6687ad57#diff-2f90975e6a0864163009708350852a9aR19, where we're making assertions about a bucket policy and need to build the expected document.
@@ -1484,15 +1517,8 @@ EOF | |||
|
|||
func testAccAWSS3BucketConfigWithAcceleration(randInt int) string { | |||
return fmt.Sprintf(` | |||
provider "aws" { | |||
alias = "west" | |||
region = "eu-west-1" |
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.
It looks like testAccAWSS3BucketConfigWithoutAcceleration
needs to be updated as well
=== RUN TestAccAWSS3Bucket_acceleration
--- FAIL: TestAccAWSS3Bucket_acceleration (243.29s)
testing.go:518: Step 1 error: Error refreshing: 1 error(s) occurred:
* aws_s3_bucket.bucket: 1 error(s) occurred:
* aws_s3_bucket.bucket: aws_s3_bucket.bucket: error reading S3 bucket "tf-test-bucket-9115369584085629874": BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region
ac64c06
to
4d873ba
Compare
Updated. FWIW, the bucket acceleration tests are failing for me on master and on this branch, so not sure what's going on there. |
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.
This looks great in both Commercial and GovCloud (US) -- thanks @jmcarp! 🚀
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Changes proposed in this pull request: