Resource - Neptune: aws_neptune_parameter_group #4724

dav009 · 2018-06-01T09:15:18Z

Reference: #4713

Changes proposed in this pull request:

adding a new resource aws_neptune_parameter_group as a first step to address Support Neptune #4713

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSNeptuneParameterGroup_basic'
=== RUN   TestAccAWSNeptuneParameterGroup_basic
--- PASS: TestAccAWSNeptuneParameterGroup_basic

$ make testacc TESTARGS='-run=TestAccAWSNeptuneParameterGroup_removeParam'
=== RUN   TestAccAWSNeptuneParameterGroup_removeParam
--- PASS: TestAccAWSNeptuneParameterGroup_removeParam

$ make testacc TESTARGS='-run=TestAccAWSNeptuneParameterGroup_only'
=== RUN   TestAccAWSNeptuneParameterGroup_only
--- PASS: TestAccAWSNeptuneParameterGroup_only

bflad

Hi @dav009 👋 Thanks for submitting this! Its looking pretty good so far. Please see the below for some feedback then we can get this in. Let us know if you have any questions or do not have time to implement the feedback.

bflad · 2018-06-03T11:58:41Z

aws/resource_aws_neptune_parameter_group.go

+			State: schema.ImportStatePassthrough,
+		},
+		Schema: map[string]*schema.Schema{
+			"name": &schema.Schema{


Minor nitpick: As of Go 1.7, the &schema.Schema declarations are optional due to the type declaration of map[string]*schema.Schema and we prefer to remove them.

bflad · 2018-06-03T12:34:35Z

aws/resource_aws_neptune_parameter_group.go

+
+	describeResp, err := conn.DescribeDBParameterGroups(&describeOpts)
+	if err != nil {
+		return err


We should check for an error here to indicate that the parameter group has been removed outside of Terraform and remove it from the Terraform state so it can successfully be recreated:

if isAWSErr(err, neptune.ErrCodeDBParameterGroupNotFoundFault, "") { log.Printf("[WARN] Neptune Parameter Group (%s) not found, removing from state", d.Id()) d.SetId("") return nil } return err

bflad · 2018-06-03T12:36:38Z

aws/resource_aws_neptune_parameter_group.go

+		return err
+	}
+
+	if len(describeResp.DBParameterGroups) != 1 ||


While we are trying to prevent errors/panics here we should check describeResp == nil as well in the (unlikely) scenario that it comes back empty from the API without an error.

bflad · 2018-06-03T12:39:20Z

aws/resource_aws_neptune_parameter_group.go

+		Source:               aws.String("user"),
+	}
+
+	describeParametersResp, err := conn.DescribeDBParameters(&describeParametersOpts)


This API call is not currently handling Marker for pagination, which means it may not receive all results in large or separated API responses. Instead, we should probably use the available SDK pagination function: DescribeDBParametersPages

bflad · 2018-06-03T12:40:46Z

aws/resource_aws_neptune_parameter_group.go

+		return err
+	}
+
+	d.Set("parameter", flattenNeptuneParameters(describeParametersResp.Parameters))


When setting the value of a non-scalar attribute into the Terraform state, we should prefer to error check it:

if err := d.Set("parameter", flattenNeptuneParameters(parameters)); err != nil { return fmt.Errorf("error setting parameter: %s", err) }

bflad · 2018-06-03T12:48:24Z

aws/resource_aws_neptune_parameter_group.go

+							Type:     schema.TypeString,
+							Required: true,
+						},
+						"apply_method": &schema.Schema{


The SDK provides two constants we can use for value validation and to ensure the value is correctly lowercase:

ValidateFunc: validation.StringInSlice([]string{ neptune.ApplyMethodImmediate, neptune.ApplyMethodPendingReboot, }, false),

bflad · 2018-06-03T12:50:31Z

aws/resource_aws_neptune_parameter_group.go

+
+		// We can only modify 20 parameters at a time, so walk them until
+		// we've got them all.
+		maxParams := 20


Minor nitpick: We should probably set this as a top level constant

bflad · 2018-06-03T12:54:57Z

aws/resource_aws_neptune_parameter_group_test.go

+						"aws_neptune_parameter_group.bar", "family", "neptune1"),
+					resource.TestCheckResourceAttr(
+						"aws_neptune_parameter_group.bar", "description", "Managed by Terraform"),
+					resource.TestCheckResourceAttr(


Minor nitpick: when testing nested attributes like TypeList and TypeSet, we should check the associated value count as well: resource.TestCheckResourceAttr("aws_neptune_parameter_group.bar", "parameter.#", "1")

We should apply this to any of the other tests below where we also expect a certain number of parameters (especially the removal test).

bflad · 2018-06-03T12:57:06Z

aws/resource_aws_neptune_parameter_group_test.go

+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSNeptuneParameterGroupDestroy,
+		Steps: []resource.TestStep{


To test importing the resource, we should add the following TestStep at the end:

{ ResourceName: "aws_neptune_parameter_group.bar", ImportState: true, ImportStateVerify: true, },

bflad · 2018-06-03T13:00:20Z

aws/structure.go

+	for _, i := range list {
+		if i.ParameterValue != nil {
+			result = append(result, map[string]interface{}{
+				"name":         strings.ToLower(*i.ParameterName),


To prevent potential panic scenarios, we should use the SDK function here and below to dereference the response objects, rather than *: e.g. aws.StringValue(i.ParameterName)

dav009 · 2018-06-08T15:31:46Z

@bflad

addressed your comments
added docs
🙇

bflad

Thanks so much for your work here @dav009! I found a few other things while reviewing and testing, which I made comments below but will fix in a commit after yours. Excited to see the beginning of Neptune support take off! 🚀

It was previously passing acceptance testing, but after feedback updates:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSNeptuneParameterGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSNeptuneParameterGroup -timeout 120m
=== RUN   TestAccAWSNeptuneParameterGroup_basic
--- PASS: TestAccAWSNeptuneParameterGroup_basic (14.14s)
=== RUN   TestAccAWSNeptuneParameterGroup_Description
--- PASS: TestAccAWSNeptuneParameterGroup_Description (14.47s)
=== RUN   TestAccAWSNeptuneParameterGroup_Parameter
--- PASS: TestAccAWSNeptuneParameterGroup_Parameter (25.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	54.107s

bflad · 2018-06-13T20:41:58Z

aws/resource_aws_neptune_parameter_group.go

+						"apply_method": {
+							Type:     schema.TypeString,
+							Optional: true,
+							Default:  "immediate",


This seems to be safer to default to pending-reboot as that is applicable to both static and dynamic parameters.

bflad · 2018-06-13T20:42:42Z

aws/resource_aws_neptune_parameter_group.go

+							Type:     schema.TypeString,
+							Required: true,
+						},
+						"apply_method": {


This argument was not documented originally and will be added post merge. Also adding validation:

ValidateFunc: validation.StringInSlice([]string{ neptune.ApplyMethodImmediate, neptune.ApplyMethodPendingReboot, }, false),

bflad · 2018-06-13T20:44:05Z

aws/resource_aws_neptune_parameter_group.go

+							// us what we used for apply_method previously, so
+							// by squashing state to an empty string we avoid
+							// needing to do an update for every future run.
+							StateFunc: func(interface{}) string { return "" },


I'm not sure where this StateFunc and the comment came from, but the current Neptune API does properly return ApplyMethod so we can set it in the Terraform state without modification.

bflad · 2018-06-13T20:44:42Z

aws/resource_aws_neptune_parameter_group.go

+						},
+					},
+				},
+				Set: resourceAwsNeptuneParameterHash,


It turns out that this custom Set function was extraneous as we want to have apply_method computed as a difference if it changed.

bflad · 2018-06-13T20:47:11Z

aws/structure.go

+	for _, i := range list {
+		if i.ParameterValue != nil {
+			result = append(result, map[string]interface{}{
+				"name":         strings.ToLower(aws.StringValue(i.ParameterName)),


The strings.ToLower() here seems extraneous. We can always add it back if for some reason its necessary.

bflad · 2018-06-13T20:47:58Z

aws/structure.go

+			result = append(result, map[string]interface{}{
+				"name":         strings.ToLower(aws.StringValue(i.ParameterName)),
+				"value":        aws.StringValue(i.ParameterValue),
+				"apply_method": strings.ToLower(aws.StringValue(i.ParameterName)),


Two issues:

Its reading ParameterName and not ApplyMethod

The strings.ToLower() seems extraneous.

bflad · 2018-06-13T20:49:43Z

website/docs/r/neptune_parameter_group.html.markdown

+* `family` - (Required) The family of the Neptune parameter group.
+* `description` - (Optional) The description of the Neptune parameter group. Defaults to "Managed by Terraform".
+* `parameter` - (Optional) A list of Neptune parameters to apply.
+* `tags`  - (Optional) A mapping of tags to assign to the resource.


The tags attribute is not currently implemented and will be removed from the documentation.

bflad · 2018-06-13T20:50:02Z

website/docs/r/neptune_parameter_group.html.markdown

+
+The following attributes are exported:
+
+* `id` - The dbNeptune parameter group name.


😎 Small typo dbNeptune

…nsure acceptance testing is per attribute make testacc TEST=./aws TESTARGS='-run=TestAccAWSNeptuneParameterGroup' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSNeptuneParameterGroup -timeout 120m === RUN TestAccAWSNeptuneParameterGroup_basic --- PASS: TestAccAWSNeptuneParameterGroup_basic (14.14s) === RUN TestAccAWSNeptuneParameterGroup_Description --- PASS: TestAccAWSNeptuneParameterGroup_Description (14.47s) === RUN TestAccAWSNeptuneParameterGroup_Parameter --- PASS: TestAccAWSNeptuneParameterGroup_Parameter (25.46s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 54.107s

bflad · 2018-06-14T14:15:27Z

This has been released in version 1.23.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-05T17:34:48Z

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!

dav009 added 2 commits June 1, 2018 18:03

adding neptune db parameter group resource

52ca380

better error message

51e0aea

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jun 1, 2018

dav009 mentioned this pull request Jun 1, 2018

Support Neptune #4713

Closed

bflad added new-resource Introduces a new resource. service/neptune Issues and PRs that pertain to the neptune service. labels Jun 1, 2018

bflad reviewed Jun 3, 2018

View reviewed changes

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

cleaning up schema declarations

488f921

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jun 8, 2018

dav009 added 11 commits June 8, 2018 21:21

adding docs

8a2fdb6

formatting docs

38f9ecb

adding sidebar link

fa1bf3e

better error handling

98d727c

relying on SDK error handling

296d9e3

neptune parameter import test

fd67933

fixing import test

6690fe1

fixing import test

4e5cccb

making maxParams a constant

5a59048

Using Neptune Describe Params pages

341c59b

Avoiding potential panic

01d8eb5

re-run ci

bead75e

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jun 8, 2018

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

bflad approved these changes Jun 13, 2018

View reviewed changes

bflad merged commit bead75e into hashicorp:master Jun 13, 2018

bflad added this to the v1.23.0 milestone Jun 13, 2018

bflad added a commit that referenced this pull request Jun 13, 2018

Update CHANGELOG for #4724

4580a05

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource - Neptune: aws_neptune_parameter_group #4724

Resource - Neptune: aws_neptune_parameter_group #4724

dav009 commented Jun 1, 2018 •

edited by bflad

Loading

bflad left a comment

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

dav009 commented Jun 8, 2018

bflad left a comment

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad Jun 13, 2018

bflad commented Jun 14, 2018

ghost commented Apr 5, 2020


		The following attributes are exported:

		* `id` - The dbNeptune parameter group name.

Resource - Neptune: aws_neptune_parameter_group #4724

Resource - Neptune: aws_neptune_parameter_group #4724

Conversation

dav009 commented Jun 1, 2018 • edited by bflad Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dav009 commented Jun 8, 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

bflad commented Jun 14, 2018

ghost commented Apr 5, 2020

dav009 commented Jun 1, 2018 •

edited by bflad

Loading