Redshift snapshot copy grant #5134

f0rk · 2018-07-10T01:27:25Z

Changes proposed in this pull request:

Add new Redshift Snapshot Copy Grant resource.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAWSRedshiftSnapshotCopyGrant_Basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAWSRedshiftSnapshotCopyGrant_Basic -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAWSRedshiftSnapshotCopyGrant_Basic
--- PASS: TestAWSRedshiftSnapshotCopyGrant_Basic (20.90s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       20.913s

The one open item on this that I am not sure how to tackle is tags. Any pointers there would be appreciated.

bflad

Hi @f0rk 👋 Thanks for submitting this! Please see the below for some initial feedback and let us know if you have any questions or do not have time to implement it.

bflad · 2018-07-17T13:50:16Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+
+	var out *redshift.CreateSnapshotCopyGrantOutput
+
+	err := resource.Retry(3*time.Minute, func() *resource.RetryError {


Given there are no resource.RetryableError() calls, the resource.Retry logic should be removed.

bflad · 2018-07-17T13:51:40Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+
+	log.Printf("[DEBUG] Created new Redshift SnapshotCopyGrant: %s", *out.SnapshotCopyGrant.SnapshotCopyGrantName)
+	d.SetId(grantName)
+	d.Set("snapshot_copy_grant_name", out.SnapshotCopyGrant.SnapshotCopyGrantName)


d.Set() calls should only occur in the read function. This allows us to ensure we have successfully created and can read back the API object.

bflad · 2018-07-17T13:53:31Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+		return nil
+	}
+
+	if *grant.KmsKeyId != "" {


To prevent nil dereferencing panics, this if statement should either check against nil or be removed (as d.Set() can appropriately handle same type pointers (e.g. *string with TypeString))

bflad · 2018-07-17T13:54:54Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+	grant, err := findAwsRedshiftSnapshotCopyGrantWithRetry(conn, grantName)
+
+	if err != nil {
+		return true, err


While the error should take precedence, this should probably return false, err

bflad · 2018-07-17T14:00:37Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+	return nil, NewAwsRedshiftSnapshotCopyGrantMissingError(fmt.Sprintf("[DEBUG] Grant %s not found", grantName))
+}
+
+// Custom error, so we don't have to rely on


We can remove this in preference of using the existing upstream resource.NotFoundError

aws/resource_aws_redshift_snapshot_copy_grant_test.go

bflad · 2018-07-17T14:04:03Z

aws/resource_aws_redshift_snapshot_copy_grant_test.go

+
+func testAccAWSRedshiftSnapshotCopyGrant_Basic(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_redshift_snapshot_copy_grant" "%s" {


Minor nitpick: the resource name should be static, e.g. basic or test

bflad · 2018-07-17T14:05:54Z

aws/resource_aws_redshift_snapshot_copy_grant_test.go

+		CheckDestroy: testAccCheckAWSRedshiftSnapshotCopyGrantDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAWSRedshiftSnapshotCopyGrant_Basic("basic"),


The random name being passed in should be randomized to prevent issues with concurrent testing or a failed run, e.g.

rName := acctest.RandomWithPrefix("tf-acc-test") // ... testAccAWSRedshiftSnapshotCopyGrant_Basic(rName) // ... resource.TestCheckResourceAttr("aws_redshift_snapshot_copy_grant.basic", "snapshot_copy_grant_name", rName),

website/docs/r/redshift_snapshot_copy_grant.html.markdown

bflad · 2018-07-17T14:10:01Z

website/docs/r/redshift_snapshot_copy_grant.html.markdown

+    snapshot_copy_grant_name = "my-grant"
+}
+
+resource "aws_redshift_parameter_group" "test" {


We prefer to only show very relevant example configuration in our documentation in case other resources/attributes/implementations change, e.g.

resource "aws_redshift_snapshot_copy_grant" "example" { snapshot_copy_grant_name = "example" } resource "aws_redshift_cluster" "example" { # ... other configuration ... snapshot_copy { destination_region = "us-east-2" grant_name = "${aws_redshift_snapshot_copy_grant.example.snapshot_copy_grant_name}" } }

f0rk · 2018-07-17T16:36:22Z

Hi @bflad,

Thanks for the feedback. I'll work on making these changes. Do you have any pointers about how to implement tags?

bflad · 2018-07-17T16:41:49Z

I'd check out how resource_aws_redshift_subnet_group.go is doing it -- briefly looking at the SnapshotCopyGrant SDK operations, it seems the implementation here might be similar. 😄

f0rk · 2018-07-17T16:44:15Z

Thanks!

f0rk · 2018-10-08T22:19:33Z

Hey @bflad this is ready for another round of review.

bflad

Looks good, @f0rk! I'll fix the little typo on merge.

--- PASS: TestAccAWSRedshiftSnapshotCopyGrant_Basic (4.58s)

bflad · 2018-10-09T01:44:15Z

aws/resource_aws_redshift_snapshot_copy_grant.go

+	out, err = conn.CreateSnapshotCopyGrant(&input)
+
+	if err != nil {
+		log.Printf("[ERROR] An error occured creating new AWS Redshift SnapshotCopyGrant: %s", err)


Looks like we caught a minor typo here:

$ make lint ==> Checking source code against linters... aws/resource_aws_redshift_snapshot_copy_grant.go:67:31:warning: "occured" is a misspelling of "occurred" (misspell) make: *** [lint] Error 1

That said, we generally prefer to return this type of messaging to the operator directly, instead of just in the debug logging, e.g.

return fmt.Errorf("error creating Redshift Snapshot Copy Grant (%s): %s", grantName, err)

bflad · 2018-10-10T19:54:34Z

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

ghost · 2020-04-02T18:00:05Z

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!

f0rk added 3 commits July 9, 2018 18:51

initial commit on snapshot copy grant

bfc612a

mostly complete implementation of redshift snapshot copy grant

97cdc60

no tags support for now

bf1e583

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 10, 2018

bflad added new-resource Introduces a new resource. service/redshift Issues and PRs that pertain to the redshift service. labels Jul 10, 2018

bflad reviewed Jul 17, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 17, 2018

f0rk added 2 commits October 8, 2018 16:55

Merge branch 'master' into redshift_snapshot_copy_grant

0ea543b

implement tags, code cleanup (thanks @bflad!)

046cc1b

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 9, 2018

bflad approved these changes Oct 9, 2018

View reviewed changes

bflad added this to the v1.40.0 milestone Oct 9, 2018

bflad merged commit 046cc1b into hashicorp:master Oct 9, 2018

bflad added a commit that referenced this pull request Oct 9, 2018

Update CHANGELOG for #5134

915afe0

ghost locked and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redshift snapshot copy grant #5134

Redshift snapshot copy grant #5134

f0rk commented Jul 10, 2018

bflad left a comment

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

f0rk commented Jul 17, 2018

bflad commented Jul 17, 2018

f0rk commented Jul 17, 2018

f0rk commented Oct 8, 2018

bflad left a comment

bflad Oct 9, 2018

bflad commented Oct 10, 2018

ghost commented Apr 2, 2020


		var out *redshift.CreateSnapshotCopyGrantOutput

		err := resource.Retry(3time.Minute, func() resource.RetryError {

Redshift snapshot copy grant #5134

Redshift snapshot copy grant #5134

Conversation

f0rk commented Jul 10, 2018

bflad 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

f0rk commented Jul 17, 2018

bflad commented Jul 17, 2018

f0rk commented Jul 17, 2018

f0rk commented Oct 8, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Oct 10, 2018

ghost commented Apr 2, 2020