New Resource: aws_neptune_cluster_parameter_group #4860

saravanan30erd · 2018-06-17T13:43:22Z

Reference #4713

Changes proposed in this pull request:

Change 1
added new resource neptune cluster parameter group

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSNeptuneClusterParameterGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSNeptuneClusterParameterGroup -timeout 120m
=== RUN   TestAccAWSNeptuneClusterParameterGroup_basic
--- PASS: TestAccAWSNeptuneClusterParameterGroup_basic (50.01s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_namePrefix
--- PASS: TestAccAWSNeptuneClusterParameterGroup_namePrefix (44.31s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_generatedName
--- PASS: TestAccAWSNeptuneClusterParameterGroup_generatedName (46.43s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_withoutParameter
--- PASS: TestAccAWSNeptuneClusterParameterGroup_withoutParameter (46.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	186.824s
...

bflad · 2018-06-18T11:55:04Z

aws/validators_test.go

+		_, errors := validateNeptuneParamGroupName(tc.Value, "aws_neptune_cluster_parameter_group_name")
+
+		if len(errors) != tc.ErrCount {
+			t.Fatalf("Expected the Neptune Parameter Group Name to trigger a validation error")


One of the test cases is generating unit test failures:

--- FAIL: TestValidateNeptuneParamGroupNamePrefix (0.00s) validators_test.go:2733: Expected the Neptune Parameter Group Name to trigger a validation error

Can you please include tc.Value in the error messaging here and below, then fix the test case? Thanks!

bflad

Thanks for submitting this @saravanan30erd! I left some feedback beyond just the unit testing piece. It will probably be good to go after. Please let us know if you have any questions or do not have time to implement this.

bflad · 2018-06-18T12:00:29Z

aws/resource_aws_neptune_cluster_parameter_group_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSNeptuneClusterParameterGroupExists("aws_neptune_cluster_parameter_group.bar", &v),
+					testAccCheckAWSNeptuneClusterParameterGroupAttributes(&v, parameterGroupName),
+					resource.TestCheckResourceAttr(


We should check the arn attribute value as well:

resource.TestMatchResourceAttr("aws_neptune_cluster_parameter_group.bar", "arn", regexp.MustCompile(fmt.Sprintf("^arn:[^:]+:neptune:[^:]+:\\d{12}:cluster-pg:%s", parameterGroupName)))

bflad · 2018-06-18T12:01:36Z

aws/resource_aws_neptune_cluster_parameter_group.go

+		}
+	}
+
+	arn := arn.ARN{


We should use the value of the ARN from the Terraform state (coming from the API) rather than manually generating it:

arn := d.Get("arn").(string)

Since we're calling update after create, we'll need to d.Set("arn", ...) in the create function as well as read.

Yeah, thats what I am looking for.

thanks, done.

bflad · 2018-06-18T12:20:24Z

aws/resource_aws_neptune_cluster_parameter_group.go

+
+	describeResp, err := conn.DescribeDBClusterParameterGroups(&describeOpts)
+	if err != nil {
+		if isAWSErr(err, neptune.ErrCodeDBParameterGroupNotFoundFault, "") {


The SDK provides a separate constant for cluster parameter groups: ErrCodeDBClusterParameterGroupNotFoundFault

bflad · 2018-06-18T12:24:01Z

aws/resource_aws_neptune_cluster_parameter_group.go

+
+	if len(describeResp.DBClusterParameterGroups) != 1 ||
+		aws.StringValue(describeResp.DBClusterParameterGroups[0].DBClusterParameterGroupName) != d.Id() {
+		return fmt.Errorf("Unable to find Cluster Parameter Group: %#v", describeResp.DBClusterParameterGroups)


Just like the regular parameter groups we should presume the API will return the correct result (terraform's plan would look very wrong otherwise) -- checking for zero length to recreate the resource instead of returning an error:

if len(describeResp.DbClusterParameterGroups) == 0 { log.Printf("[WARN] Neptune Cluster Parameter Group (%s) not found, removing from state", d.Id()) d.SetId("") return nil }

sure, I understood. But anyway we don't need to check for zero length right? since it should be covered by ErrCodeDBClusterParameterGroupNotFoundFault.

We've seen cases where some service APIs will respond with no errors and also an empty result set. In any case, its best to include the handling to prevent Terraform from crashing with the Go panic of index not in range.

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

aws/resource_aws_neptune_cluster_parameter_group.go

+	log.Printf("[DEBUG] Deleting Neptune Cluster Parameter Group: %s", d.Id())
+	_, err := conn.DeleteDBClusterParameterGroup(&input)
+	if err != nil {
+		if isAWSErr(err, neptune.ErrCodeDBParameterGroupNotFoundFault, "") {


The SDK provides a separate constant for cluster parameter groups: ErrCodeDBClusterParameterGroupNotFoundFault

bflad · 2018-06-18T12:25:31Z

aws/resource_aws_neptune_cluster_parameter_group_test.go

+		}
+
+		if err != nil {
+			if isAWSErr(err, neptune.ErrCodeDBParameterGroupNotFoundFault, "") {


The SDK provides a separate constant for cluster parameter groups: ErrCodeDBClusterParameterGroupNotFoundFault

@bflad Looks like ErrCodeDBClusterParameterGroupNotFoundFault only available for createdbcluster and modifydbcluster and ErrCodeDBParameterGroupNotFoundFault is the one used for other functions.
https://docs.aws.amazon.com/sdk-for-go/api/service/neptune/#Neptune.DeleteDBClusterParameterGroup

🙁 this is either an error in the AWS documentation or an oversight. I will reach out to AWS support for clarification.

Sorry this was a bad assumption on my part. This behavior is frustrating:

$ aws neptune describe-db-cluster-parameter-groups --db-cluster-parameter-group-name does-not-exist An error occurred (DBParameterGroupNotFound) when calling the DescribeDBClusterParameterGroups operation: DBClusterParameterGroup not found: does-not-exist $ aws neptune delete-db-cluster-parameter-group --db-cluster-parameter-group-name does-not-exist An error occurred (DBParameterGroupNotFound) when calling the DeleteDBClusterParameterGroup operation: DBClusterParameterGroup not found: does-not-exist $ aws neptune modify-db-cluster-parameter-group --db-cluster-parameter-group-name does-not-exist --parameters 'ParameterName=test,ParameterValue=""' An error occurred (DBParameterGroupNotFound) when calling the ModifyDBClusterParameterGroup operation: Parameter group does not exist

I guess we should stick with the non-Cluster constant and add a comment to each instance about why its not using the Cluster constant. My apologies.

yes, acceptance tests were failed when I was tryingErrCodeDBClusterParameterGroupNotFoundFault .

bflad · 2018-06-18T12:29:38Z

website/docs/r/neptune_cluster_parameter_group.html.markdown

+---
+layout: "aws"
+page_title: "AWS: aws_neptune_cluster_parameter_group"
+sidebar_current: "docs-aws-resource-aws-neptune-cluster-parameter-group"


Documentation page is missing a sidebar link in website/aws.erb

done, also I found duplicate entry for neptune parameter group in this file and removed it.

saravanan30erd · 2018-06-18T17:12:47Z

@bflad thanks for feedback, I done all the changes 👍

bflad

This should be pretty good to go, thanks @saravanan30erd! I'll add an import test case post-merge and ensure that parameters are updating correctly via acceptance testing. 🚀

4 tests passed (all tests)
--- PASS: TestAccAWSNeptuneClusterParameterGroup_namePrefix (6.58s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_generatedName
--- PASS: TestAccAWSNeptuneClusterParameterGroup_generatedName (6.65s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_withoutParameter
--- PASS: TestAccAWSNeptuneClusterParameterGroup_withoutParameter (6.75s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_basic
--- PASS: TestAccAWSNeptuneClusterParameterGroup_basic (6.85s)

bflad · 2018-06-25T11:46:26Z

This has been released in version 1.24.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:18:33Z

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!

saravanan30erd added 4 commits June 16, 2018 13:07

add new resource aws_neptune_cluster_parameter_group

324c0a2

add acceptance test

eb7a3de

add document for neptune_cluster_parameter_group

89924d0

fix the issue in setting tags

ac4f6c0

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

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

bflad reviewed Jun 18, 2018

View reviewed changes

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

saravanan30erd changed the title ~~Neptune clus param grp~~ New Resource: aws_neptune_cluster_parameter_group Jun 18, 2018

fix the unit testing

05c3a3c

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

corrections based on feedback

2764126

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

bflad added this to the v1.24.0 milestone Jun 18, 2018

bflad approved these changes Jun 18, 2018

View reviewed changes

bflad merged commit 2764126 into hashicorp:master Jun 18, 2018

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

Update CHANGELOG for #4860

30c1f5c

darrenhaken pushed a commit to darrenhaken/terraform-provider-aws that referenced this pull request Jun 20, 2018

Update CHANGELOG for hashicorp#4860

53709e3

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_neptune_cluster_parameter_group #4860

New Resource: aws_neptune_cluster_parameter_group #4860

saravanan30erd commented Jun 17, 2018 •

edited by bflad

Loading

bflad Jun 18, 2018

bflad left a comment

bflad Jun 18, 2018

bflad Jun 18, 2018

saravanan30erd Jun 18, 2018

saravanan30erd Jun 18, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

saravanan30erd Jun 18, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

saravanan30erd Jun 18, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

saravanan30erd Jun 18, 2018 •

edited

Loading

bflad Jun 18, 2018

saravanan30erd Jun 18, 2018

saravanan30erd commented Jun 18, 2018

bflad left a comment

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020

New Resource: aws_neptune_cluster_parameter_group #4860

New Resource: aws_neptune_cluster_parameter_group #4860

Conversation

saravanan30erd commented Jun 17, 2018 • edited by bflad Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saravanan30erd Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saravanan30erd commented Jun 18, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020

saravanan30erd commented Jun 17, 2018 •

edited by bflad

Loading

saravanan30erd Jun 18, 2018 •

edited

Loading