New Resource: aws_ssm_resource_data_sync #1895

atsushi-ishibashi · 2017-10-14T20:02:30Z

#1259
In my local environment, I got error like below. But all checks have passed in travis-ci...

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSsmResourceDataSync'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSsmResourceDataSync -timeout 120m
=== RUN   TestAccAWSSsmResourceDataSync
--- FAIL: TestAccAWSSsmResourceDataSync (44.23s)
	testing.go:434: Step 0 error: Check failed: Check 1/1 error: Not found: aws_ssm_resource_data_sync.foo
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	44.270s
make: *** [testacc] Error 1

atsushi-ishibashi · 2017-10-23T08:08:39Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSsmResourceDataSync'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSsmResourceDataSync -timeout 120m
=== RUN   TestAccAWSSsmResourceDataSync
--- PASS: TestAccAWSSsmResourceDataSync (60.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	60.457s

radeksimko

Thanks for the PR @atsushi-ishibashi
I left you some comments in the code. Let me know if anything's unclear.

radeksimko · 2017-11-09T17:37:38Z

aws/resource_aws_ssm_resource_data_sync.go

+		if aerr, ok := err.(awserr.Error); ok {
+			switch aerr.Code() {
+			case ssm.ErrCodeResourceDataSyncAlreadyExistsException:
+				if err := resourceAwsSsmResourceDataSyncDeleteAndCreate(meta, input); err != nil {


Can you please explain why we should ever recreate the resource? The common convention (and user's expectations) is to just error out in case there's a conflict.

radeksimko · 2017-11-09T17:55:50Z

aws/resource_aws_ssm_resource_data_sync.go

+				Required: true,
+				ForceNew: true,
+			},
+			"destination": {


Is there any reason we should drift away from the API here? i.e. how do you feel about renaming this to s3_destination?

radeksimko · 2017-11-09T17:56:12Z

aws/resource_aws_ssm_resource_data_sync.go

+							Type:     schema.TypeString,
+							Optional: true,
+						},
+						"bucket": {


As above - bucket_name?

radeksimko · 2017-11-09T17:56:40Z

aws/resource_aws_ssm_resource_data_sync.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"key": {


As above - kms_key_arn?

radeksimko · 2017-11-09T17:58:48Z

aws/resource_aws_ssm_resource_data_sync.go

+
+	input := &ssm.CreateResourceDataSyncInput{
+		S3Destination: &ssm.ResourceDataSyncS3Destination{
+			SyncFormat: aws.String(ssm.ResourceDataSyncS3FormatJsonSerDe),


Perhaps a nitpick, but how do you feel about exposing this argument via schema too? It would help us to be prepared for time when AWS introduces new sync format(s).

radeksimko · 2017-11-09T18:07:28Z

aws/resource_aws_ssm_resource_data_sync_test.go

+			if found || nextToken == "" {
+				break
+			}
+		}


Once you have decoupled the logic for finding the right data sync, it can also be reused here 😉

radeksimko · 2017-11-09T18:07:56Z

aws/resource_aws_ssm_resource_data_sync_test.go

+			}
+		}
+		if found {
+			return fmt.Errorf("[DELETE ERROR] Resource Data Sync found for SyncName: %s", rs.Primary.Attributes["name"])


Nitpick, but there's no reason to prefix the message with [DELETE ERROR] - it's already clear that it's error.

radeksimko · 2017-11-09T18:08:35Z

aws/resource_aws_ssm_resource_data_sync_test.go

+    }
+
+    resource "aws_ssm_resource_data_sync" "foo" {
+      name = "foo"


Do you mind randomizing the name here?

radeksimko · 2017-11-09T18:09:37Z

aws/resource_aws_ssm_resource_data_sync.go

+					if val, ok := m["prefix"]; ok {
+						buf.WriteString(fmt.Sprintf("%s-", strings.ToLower(val.(string))))
+					}
+					return hashcode.String(buf.String())


Can you please explain why do we need to lowercase all the values here? It seems unnecessary to me. 🤔

radeksimko · 2017-11-09T18:12:21Z

aws/resource_aws_ssm_resource_data_sync.go

+				ForceNew: true,
+			},
+			"destination": {
+				Type:     schema.TypeSet,


Since this is a singleton I think we can just make it TypeList as there's no value in computing the hash and it would also complicate referencing. TypeList can be referenced as s3_destination.0.key etc., whereas TypeSet is non-trivial to reference because the address is s3_destination.<computed-idx>.key.

atsushi-ishibashi · 2017-11-10T02:24:18Z

@radeksimko I absolutely agree with your feedback. I'm sorry for bothering you with trivial mistakes because I couldn't understand the behaivour when I submitted this PR 🙇
I added sync_format with default value.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSsmResourceDataSync_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSsmResourceDataSync_basic -timeout 120m
=== RUN   TestAccAWSSsmResourceDataSync_basic
--- PASS: TestAccAWSSsmResourceDataSync_basic (63.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	63.119s

radeksimko

Hi @atsushi-ishibashi
thanks for implementing the feedback promptly.
There's no need to apologise! 😃

I just left you a few last comments - then I think we're ready to ship it 🚀

radeksimko · 2017-11-10T06:38:46Z

aws/resource_aws_ssm_resource_data_sync.go

+		} else {
+			d.SetId("")
+			return nil
+		}


I'm slightly worried about this conditional here, because we may add more errors to findResourceDataSyncItem in the future which aren't awserr and those may have nothing to do with "resource not found".
How do you feel about tweaking findResourceDataSyncItem to return nil, nil if no resource was found and then checking the nil here, instead of checking the error isn't awserr.Error?

Sure👍 I'll apply that idea!

radeksimko · 2017-11-10T06:41:51Z

aws/resource_aws_ssm_resource_data_sync_test.go

+				return err
+			} else {
+				return nil
+			}


^ I share the same concerns here as above

radeksimko · 2017-11-10T06:42:52Z

aws/resource_aws_ssm_resource_data_sync_test.go

+	return fmt.Sprintf(`
+    resource "aws_s3_bucket" "hoge" {
+      bucket = "tf-test-bucket-%d"
+      region = "us-east-1"


Any particular reason why this bucket should be in us-east-1? It's certainly no big deal, just that our primary test region is us-west-2.

radeksimko

👋
There's a few compilation errors to address:

aws/resource_aws_ssm_resource_data_sync.go:4:2: imported and not used: "fmt"
aws/resource_aws_ssm_resource_data_sync.go:7:2: imported and not used: "github.com/terraform-providers/terraform-provider-aws/vendor/github.com/aws/aws-sdk-go/aws/awserr"

😉

radeksimko · 2017-11-10T09:50:02Z

@atsushi-ishibashi I just added retry logic to work around the eventually consistent nature of S3 which emerged in the acceptance test:

=== RUN   TestAccAWSSsmResourceDataSync_basic
--- FAIL: TestAccAWSSsmResourceDataSync_basic (61.77s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_ssm_resource_data_sync.foo: 1 error(s) occurred:

		* aws_ssm_resource_data_sync.foo: ResourceDataSyncInvalidConfigurationException: S3 write failed for bucket [tf-test-bucket-5869476133795886706] with prefix [null] with sync name [tf-test-ssm-2hzj4] due to [Access Denied (Service: Amazon S3; Status Code: 403; Error Code: AccessDenied; Request ID: 31B58B3D98C61C90; S3 Extended Request ID: ra9YFypLVbeh6yzILpZ8S39ukPnmkCw6UBFWwSKdYeTLU7uKBgI5a0Mwut4Ds/7J)]. Please refer: http://docs.aws.amazon.com/systems-manager/latest/userguide/sysman-inventory-configuring.html#sysman-inventory-datasync
			status code: 400, request id: ccedb0c2-c5f8-11e7-9213-61c5dcb61d04

Let me know if you have any questions in regards to that patch, otherwise I'm happy to merge this.

atsushi-ishibashi · 2017-11-10T12:47:40Z

@radeksimko I missed that point... Of course, I appreciate the patch👍

ghost · 2020-04-10T17:39:04Z

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!

CRD methods for ResourceDayaSync

e3912c2

radeksimko added the new-resource Introduces a new resource. label Oct 14, 2017

atsushi-ishibashi changed the title ~~[WIP]New Resource: ResourceDataSyncS3Destination~~ New Resource: ResourceDataSyncS3Destination Oct 15, 2017

Modify checkDestroy, make docs

360b110

radeksimko changed the title ~~New Resource: ResourceDataSyncS3Destination~~ New Resource: aws_ssm_resource_data_sync Nov 9, 2017

radeksimko reviewed Nov 9, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Nov 9, 2017

Reflect 1st review

cb588ab

radeksimko reviewed Nov 10, 2017

View reviewed changes

Reflect 2nd review

926668e

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Nov 10, 2017

radeksimko suggested changes Nov 10, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Nov 10, 2017

goimports

879b0f8

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Nov 10, 2017

Retry creation due S3 eventual consistency

2e1236e

radeksimko approved these changes Nov 10, 2017

View reviewed changes

radeksimko merged commit a28808b into hashicorp:master Nov 10, 2017

ghost locked and limited conversation to collaborators Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_ssm_resource_data_sync #1895

New Resource: aws_ssm_resource_data_sync #1895

atsushi-ishibashi commented Oct 14, 2017 •

edited

Loading

atsushi-ishibashi commented Oct 23, 2017

radeksimko left a comment

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

radeksimko Nov 9, 2017

atsushi-ishibashi commented Nov 10, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Nov 10, 2017 •

edited

Loading

atsushi-ishibashi Nov 10, 2017

radeksimko Nov 10, 2017

radeksimko Nov 10, 2017

radeksimko left a comment

radeksimko commented Nov 10, 2017

atsushi-ishibashi commented Nov 10, 2017

ghost commented Apr 10, 2020

New Resource: aws_ssm_resource_data_sync #1895

New Resource: aws_ssm_resource_data_sync #1895

Conversation

atsushi-ishibashi commented Oct 14, 2017 • edited Loading

atsushi-ishibashi commented Oct 23, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Nov 10, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko commented Nov 10, 2017

atsushi-ishibashi commented Nov 10, 2017

ghost commented Apr 10, 2020

atsushi-ishibashi commented Oct 14, 2017 •

edited

Loading

atsushi-ishibashi commented Nov 10, 2017 •

edited

Loading

radeksimko Nov 10, 2017 •

edited

Loading