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

#2296 aws_s3_bucket-replication-config-update-fix #3349

Merged

Conversation

Chhed13
Copy link
Contributor

@Chhed13 Chhed13 commented Feb 13, 2018

Always update replication_configuration fields.
Fix for #2296

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 13, 2018
@bflad bflad added bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. labels Feb 13, 2018
@bflad bflad added this to the v1.10.0 milestone Feb 13, 2018
@bflad bflad self-requested a review February 13, 2018 13:10
@@ -337,6 +337,7 @@ func resourceAwsS3Bucket() *schema.Resource {
"replication_configuration": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey quick question -- what was the motivation for adding this?

Copy link
Contributor Author

@Chhed13 Chhed13 Feb 14, 2018

Choose a reason for hiding this comment

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

Good question. The reason was - failed test TestAccAWSS3Bucket_Replication part 2

	testing.go:513: Step 1 error: Check failed: Check 2/7 error: aws_s3_bucket.bucket: Attribute 'replication_configuration.#' expected "1", got "0"

Process finished with exit code 1

Maybe it's just flacky. I re-run it many times - some times without Computed it pass, but mostly fail.
With Computed it works stable.
I know it's not a good answer, maybe root cause not here - but for now I didn't go deeper

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, we don't want to change this and the test failure you mention is unfortunately a red herring. S3 eventual consistency is a beast (we get failures regularly in our daily acceptance testing 😬 ) so you're not alone. I believe setting Computed: true here basically means that we will ignore out of band changes to the replication configuration in AWS if its not defined in the Terraform configuration, which is a change in behavior for the resource.

Removing it and running the tests worked fine for me 👍

make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_Replication'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSS3Bucket_Replication -timeout 120m
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (87.72s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (57.96s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (32.61s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	178.355s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the great explanation, now I revert it.
But for me the red flag was - that this test always pass before I made other changes. I explained it 2 step bucket creation process (first Create without replication, next Update and than we Read it several times while it come to state). On the first Read replication can absent, but what's changed - I added default line for replication to state and it can be read.
While debug output shows that all is ok, looks like test read first version of state if the section is present

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

I think this is good to go once the Computed: true change is reverted 👍

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 15, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

LGTM!

Tests passed: 24
=== RUN   TestAccAWSS3Bucket_shouldFailNotFound
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (10.57s)
=== RUN   TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (10.74s)
=== RUN   TestAccAWSS3Bucket_generatedName
--- PASS: TestAccAWSS3Bucket_generatedName (10.94s)
=== RUN   TestAccAWSS3Bucket_importBasic
--- PASS: TestAccAWSS3Bucket_importBasic (11.93s)
=== RUN   TestAccAWSS3Bucket_namePrefix
--- PASS: TestAccAWSS3Bucket_namePrefix (12.72s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (25.27s)
=== RUN   TestAccAWSS3Bucket_importWithPolicy
--- PASS: TestAccAWSS3Bucket_importWithPolicy (26.94s)
=== RUN   TestAccAWSS3Bucket_RequestPayer
--- PASS: TestAccAWSS3Bucket_RequestPayer (29.08s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (31.02s)
=== RUN   TestAccAWSS3Bucket_WebsiteRoutingRules
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (30.98s)
=== RUN   TestAccAWSS3Bucket_UpdateAcl
--- PASS: TestAccAWSS3Bucket_UpdateAcl (32.23s)
=== RUN   TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (33.31s)
=== RUN   TestAccAWSS3Bucket_Website_Simple
--- PASS: TestAccAWSS3Bucket_Website_Simple (35.49s)
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (37.33s)
=== RUN   TestAccAWSS3Bucket_region
--- PASS: TestAccAWSS3Bucket_region (37.44s)
=== RUN   TestAccAWSS3Bucket_Versioning
--- PASS: TestAccAWSS3Bucket_Versioning (38.97s)
=== RUN   TestAccAWSS3Bucket_Logging
--- PASS: TestAccAWSS3Bucket_Logging (40.53s)
=== RUN   TestAccAWSS3Bucket_Cors
--- PASS: TestAccAWSS3Bucket_Cors (40.53s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (32.19s)
=== RUN   TestAccAWSS3Bucket_Lifecycle
--- PASS: TestAccAWSS3Bucket_Lifecycle (33.53s)
=== RUN   TestAccAWSS3Bucket_Policy
--- PASS: TestAccAWSS3Bucket_Policy (45.40s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (56.99s)
=== RUN   TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (79.40s)
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (82.74s)

@bflad bflad merged commit f9719ad into hashicorp:master Feb 15, 2018
bflad added a commit that referenced this pull request Feb 15, 2018
@bflad
Copy link
Contributor

bflad commented Feb 27, 2018

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 7, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants