provider/aws: Support MFA delete for s3 bucket versioning #10020

stack72 · 2016-11-10T13:12:29Z

catsby · 2016-12-09T18:00:02Z

Is this still WIP?

Fixes #7902 ``` % make testacc TEST=./builtin/providers/aws % TESTARGS='-run=TestAccAWSS3Bucket_' % ✹ ==> Checking that code complies with gofmt requirements... go generate $(go list ./... | grep -v /terraform/vendor/) 2016/12/12 12:11:45 Generated command/internal_plugin_list.go TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSS3Bucket_ -timeout 120m === RUN TestAccAWSS3Bucket_importBasic --- PASS: TestAccAWSS3Bucket_importBasic (55.74s) === RUN TestAccAWSS3Bucket_importWithPolicy --- PASS: TestAccAWSS3Bucket_importWithPolicy (63.34s) === RUN TestAccAWSS3Bucket_Notification --- PASS: TestAccAWSS3Bucket_Notification (165.15s) === RUN TestAccAWSS3Bucket_NotificationWithoutFilter --- PASS: TestAccAWSS3Bucket_NotificationWithoutFilter (63.22s) === RUN TestAccAWSS3Bucket_basic --- PASS: TestAccAWSS3Bucket_basic (47.82s) === RUN TestAccAWSS3Bucket_region --- PASS: TestAccAWSS3Bucket_region (18.88s) === RUN TestAccAWSS3Bucket_acceleration --- PASS: TestAccAWSS3Bucket_acceleration (34.56s) === RUN TestAccAWSS3Bucket_RequestPayer --- PASS: TestAccAWSS3Bucket_RequestPayer (90.26s) === RUN TestAccAWSS3Bucket_Policy --- PASS: TestAccAWSS3Bucket_Policy (120.25s) === RUN TestAccAWSS3Bucket_UpdateAcl --- PASS: TestAccAWSS3Bucket_UpdateAcl (87.51s) === RUN TestAccAWSS3Bucket_Website_Simple --- PASS: TestAccAWSS3Bucket_Website_Simple (138.38s) === RUN TestAccAWSS3Bucket_WebsiteRedirect --- PASS: TestAccAWSS3Bucket_WebsiteRedirect (139.44s) === RUN TestAccAWSS3Bucket_WebsiteRoutingRules --- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (97.82s) === RUN TestAccAWSS3Bucket_shouldFailNotFound --- PASS: TestAccAWSS3Bucket_shouldFailNotFound (26.84s) === RUN TestAccAWSS3Bucket_Versioning --- PASS: TestAccAWSS3Bucket_Versioning (131.89s) === RUN TestAccAWSS3Bucket_Cors --- PASS: TestAccAWSS3Bucket_Cors (92.71s) === RUN TestAccAWSS3Bucket_Logging --- PASS: TestAccAWSS3Bucket_Logging (86.46s) === RUN TestAccAWSS3Bucket_Lifecycle --- PASS: TestAccAWSS3Bucket_Lifecycle (132.70s) === RUN TestAccAWSS3Bucket_Replication --- PASS: TestAccAWSS3Bucket_Replication (122.70s) === RUN TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError --- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (39.04s) ```

stack72 · 2016-12-12T12:52:47Z

Hi @catsby

Thanks for the ping here - finally got this working now :)

P.

Ninir · 2016-12-12T22:14:03Z

Awesome @stack72 ! thanks :)

paddycarver

Looks sane to me, just a question about BC.

Would also love to see a test that exercises the new property, personally, but that may just be me.

paddycarver · 2016-12-12T22:15:45Z

builtin/providers/aws/resource_aws_s3_bucket.go

@@ -147,24 +147,24 @@ func resourceAwsS3Bucket() *schema.Resource {
 			},

 			"versioning": {
-				Type:     schema.TypeSet,
+				Type:     schema.TypeList,


Gonna put my ignorance on display here for a moment: would this break configs/require a migration?

Technically yes, as we are moving from the hashcode to an int - but I figured as it was a 0.8 release and we only ever allow 1 set of versioning, that we wouldn't cause any issue here

stack72 · 2016-12-12T22:29:38Z

Pre this PR I had a bucket that had the following state:

% terraform show                                                                                                                ✭
aws_s3_bucket.bucket:
  id = tf-test-bucket-paddy-test
  acceleration_status =
  acl = public-read
  arn = arn:aws:s3:::tf-test-bucket-paddy-test
  bucket = tf-test-bucket-paddy-test
  force_destroy = false
  hosted_zone_id = Z3BJ6K6RIION7M
  region = us-west-2
  request_payer = BucketOwner
  tags.% = 0
  versioning.# = 1
  versioning.2972667452.enabled = false
  website.# = 0

stack72 · 2016-12-12T22:31:14Z

Post this PR, changing the set to a list has no effect:

% terraform plan                                                                                                                ✭
[WARN] /Users/stacko/Code/go/bin/terraform-provider-aws overrides an internal plugin for aws-provider.
  If you did not expect to see this message you will need to remove the old plugin.
  See https://www.terraform.io/docs/internals/internal-plugins.html
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but
will not be persisted to local or remote state storage.

aws_s3_bucket.bucket: Refreshing state... (ID: tf-test-bucket-paddy-test)

No changes. Infrastructure is up-to-date. This means that Terraform
could not detect any differences between your configuration and
the real physical resources that exist. As a result, Terraform
doesn't need to do anything.

% terraform show                                                                                                                ✭
[WARN] /Users/stacko/Code/go/bin/terraform-provider-aws overrides an internal plugin for aws-provider.
 If you did not expect to see this message you will need to remove the old plugin.
 See https://www.terraform.io/docs/internals/internal-plugins.html
aws_s3_bucket.bucket:
 id = tf-test-bucket-paddy-test
 acceleration_status =
 acl = public-read
 arn = arn:aws:s3:::tf-test-bucket-paddy-test
 bucket = tf-test-bucket-paddy-test
 force_destroy = false
 hosted_zone_id = Z3BJ6K6RIION7M
 region = us-west-2
 request_payer = BucketOwner
 tags.% = 0
 versioning.# = 1
 versioning.2972667452.enabled = false
 website.# = 0

stack72 · 2016-12-12T22:34:01Z

Took @paddyforan through this to clear up his question :)

paddycarver · 2016-12-12T22:34:59Z

Yup, questions cleared up, post-merge LGTM :)

(Test explanation is: MFA means the tests wouldn't run right because it involves manual interaction. Oops.)

ghost · 2020-04-18T02:31:36Z

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.

stack72 changed the title ~~provider/aws: Support MFA delete for s3 bucket versioning~~ [WIP] provider/aws: Support MFA delete for s3 bucket versioning Nov 10, 2016

stack72 added enhancement provider/aws labels Nov 10, 2016

catsby added the waiting-response An issue/pull request is waiting for a response from the community label Dec 9, 2016

stack72 force-pushed the aws-s3-mfa-delete branch from 506d31a to 25518f9 Compare December 12, 2016 11:37

stack72 changed the title ~~[WIP] provider/aws: Support MFA delete for s3 bucket versioning~~ provider/aws: Support MFA delete for s3 bucket versioning Dec 12, 2016

stack72 force-pushed the aws-s3-mfa-delete branch from 25518f9 to 44c4fc2 Compare December 12, 2016 12:14

stack72 force-pushed the aws-s3-mfa-delete branch from 44c4fc2 to e519758 Compare December 12, 2016 12:48

stack72 removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 12, 2016

paddycarver reviewed Dec 12, 2016

View reviewed changes

stack72 merged commit 1113ed5 into master Dec 12, 2016

stack72 deleted the aws-s3-mfa-delete branch December 12, 2016 22:34

ghost locked and limited conversation to collaborators Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Support MFA delete for s3 bucket versioning #10020

provider/aws: Support MFA delete for s3 bucket versioning #10020

stack72 commented Nov 10, 2016

catsby commented Dec 9, 2016

stack72 commented Dec 12, 2016

Ninir commented Dec 12, 2016

paddycarver left a comment

paddycarver Dec 12, 2016

stack72 Dec 12, 2016

stack72 commented Dec 12, 2016

stack72 commented Dec 12, 2016

stack72 commented Dec 12, 2016

paddycarver commented Dec 12, 2016 •

edited

Loading

ghost commented Apr 18, 2020

provider/aws: Support MFA delete for s3 bucket versioning #10020

provider/aws: Support MFA delete for s3 bucket versioning #10020

Conversation

stack72 commented Nov 10, 2016

catsby commented Dec 9, 2016

stack72 commented Dec 12, 2016

Ninir commented Dec 12, 2016

paddycarver left a comment

Choose a reason for hiding this comment

paddycarver Dec 12, 2016

Choose a reason for hiding this comment

stack72 Dec 12, 2016

Choose a reason for hiding this comment

stack72 commented Dec 12, 2016

stack72 commented Dec 12, 2016

stack72 commented Dec 12, 2016

paddycarver commented Dec 12, 2016 • edited Loading

ghost commented Apr 18, 2020

paddycarver commented Dec 12, 2016 •

edited

Loading