issue #4713 New resource aws_neptune_subnet_group #4782

saravanan30erd · 2018-06-07T21:34:08Z

Reference: #4713

saravanan30erd · 2018-06-09T14:13:39Z

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSNeptuneSubnetGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSNeptuneSubnetGroup -timeout 120m
=== RUN TestAccAWSNeptuneSubnetGroup_basic
--- PASS: TestAccAWSNeptuneSubnetGroup_basic (94.48s)
=== RUN TestAccAWSNeptuneSubnetGroup_namePrefix
--- PASS: TestAccAWSNeptuneSubnetGroup_namePrefix (95.88s)
=== RUN TestAccAWSNeptuneSubnetGroup_generatedName
--- PASS: TestAccAWSNeptuneSubnetGroup_generatedName (92.99s)
=== RUN TestAccAWSNeptuneSubnetGroup_withUndocumentedCharacters
--- PASS: TestAccAWSNeptuneSubnetGroup_withUndocumentedCharacters (104.20s)
=== RUN TestAccAWSNeptuneSubnetGroup_updateDescription
--- PASS: TestAccAWSNeptuneSubnetGroup_updateDescription (165.62s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 553.208s

bflad · 2018-06-14T12:15:22Z

This is now ready for rebase since the parameter group resource was merged yesterday 👍

bflad

Hi @saravanan30erd -- thanks for submitting this! I left some initial feedback below, a lot of which probably comes from copying the RDS subnet group resource and testing. Please let us know if you have any questions or do not have time to implement the feedback.

bflad · 2018-06-14T12:21:44Z

aws/resource_aws_neptune_subnet_group.go

+	}
+
+	var subnetGroups []*neptune.DBSubnetGroup
+	//describeResp, err := conn.DescribeDBSubnetGroups(&describeOpts)


Minor nitpick: comment can be removed 😄

bflad · 2018-06-14T12:24:09Z

aws/resource_aws_neptune_subnet_group.go

+		}
+		return !lastPage
+	}); err != nil {
+		if neptuneerr, ok := err.(awserr.Error); ok && neptuneerr.Code() == "DBSubnetGroupNotFoundFault" {


Nitpick: We can use the helper function and SDK provided constant here -- we should also provide a log message when removing a resource from the state, e.g.

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

bflad · 2018-06-14T12:27:05Z

aws/resource_aws_neptune_subnet_group.go

+	}
+
+	if len(subnetGroups) == 0 {
+		return fmt.Errorf("Unable to find Neptune Subnet Group: %#v", subnetGroups)


We should provide the log message and remove from state, similar to the above error handling, rather than returning an error here.

bflad · 2018-06-14T12:32:38Z

aws/resource_aws_neptune_subnet_group.go

+		return fmt.Errorf("Unable to find Neptune Subnet Group: %#v", subnetGroups)
+	}
+
+	var subnetGroup *neptune.DBSubnetGroup


Given that we force lowercase naming via the validation functions, it should be safe to assume here that we can directly access the first element of the slice instead of this extra logic.

subnetGroup := subnetGroups[0]

bflad · 2018-06-14T12:39:02Z

aws/resource_aws_neptune_subnet_group.go

+	for _, s := range subnetGroup.Subnets {
+		subnets = append(subnets, aws.StringValue(s.SubnetIdentifier))
+	}
+	d.Set("subnet_ids", subnets)


For TypeSet attributes, we should perform error checking, e.g.

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

In this case I believe you'll find that setting this attribute needs to be occur with schema.NewSet(), e.g. something like

if err := d.Set("subnet_ids", schema.NewSet(schema.HashString, subnets)); err != nil { return fmt.Errorf("error setting subnet_ids: %s", err) }

bflad · 2018-06-14T13:34:04Z

aws/resource_aws_neptune_subnet_group_test.go

+func TestAccAWSNeptuneSubnetGroup_basic(t *testing.T) {
+	var v neptune.DBSubnetGroup
+
+	testCheck := func(*terraform.State) error {


This function seems extraneous

bflad · 2018-06-14T13:48:46Z

aws/resource_aws_neptune_subnet_group_test.go

+
+// Regression test for https://github.com/hashicorp/terraform/issues/2603 and
+// https://github.com/hashicorp/terraform/issues/2664
+func TestAccAWSNeptuneSubnetGroup_withUndocumentedCharacters(t *testing.T) {


Acceptable characters are in the CreateDBSubnetGroup API documentation:

Constraints: Must contain no more than 255 letters, numbers, periods, underscores, spaces, or hyphens. Must not be default.

This validation should be performed in unit testing and not acceptance testing.

removed this acceptance test.

Unit testing for this validation already covered in validators_test.go. still we need to add unit testing here?

bflad · 2018-06-14T13:54:48Z

aws/tagsNeptune.go

+	// First, we're creating everything we have
+	create := make(map[string]interface{})
+	for _, t := range newTags {
+		create[*t.Key] = *t.Value


To prevent potential panics in the unlikely cases that the key or value is nil we may want to consider using aws.StringValue() instead of direct * dereferences across this file.

bflad · 2018-06-14T13:58:32Z

aws/validators.go

+		errors = append(errors, fmt.Errorf(
+			"%q cannot be longer than 255 characters", k))
+	}
+	if regexp.MustCompile(`(?i)^default$`).MatchString(value) {


Since we have already validated that the value must be lowercase previously, we should opt to directly check the string instead, e.g.

if value == "default" {

bflad · 2018-06-14T13:59:54Z

aws/validators.go

+		errors = append(errors, fmt.Errorf(
+			"only lowercase alphanumeric characters, hyphens, underscores, periods, and spaces allowed in %q", k))
+	}
+	if len(value) > 229 {


We should opt to include how we came to this value, e.g.

prefixMaxLength := 255 - resource.UniqueIDSuffixLength

saravanan30erd · 2018-06-14T16:27:07Z

@bflad done all the changes.

bflad

Thanks for the updates @saravanan30erd! Let's get this in. 🚀

(I'll add the import tests post-merge)

4 tests passed (all tests)
=== RUN   TestAccAWSNeptuneSubnetGroup_basic
--- PASS: TestAccAWSNeptuneSubnetGroup_basic (9.29s)
=== RUN   TestAccAWSNeptuneSubnetGroup_namePrefix
--- PASS: TestAccAWSNeptuneSubnetGroup_namePrefix (9.38s)
=== RUN   TestAccAWSNeptuneSubnetGroup_generatedName
--- PASS: TestAccAWSNeptuneSubnetGroup_generatedName (9.66s)
=== RUN   TestAccAWSNeptuneSubnetGroup_updateDescription
--- PASS: TestAccAWSNeptuneSubnetGroup_updateDescription (16.64s)

bflad · 2018-06-15T20:26:05Z

aws/resource_aws_neptune_subnet_group.go

+		Read:   resourceAwsNeptuneSubnetGroupRead,
+		Update: resourceAwsNeptuneSubnetGroupUpdate,
+		Delete: resourceAwsNeptuneSubnetGroupDelete,
+		Importer: &schema.ResourceImporter{


Sorry I missed this before, looks like we're missing import test cases, e.g.

{ ResourceName: "aws_neptune_subnet_group.test", ImportState: true, ImportStateVerify: true, },

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

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:27Z

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 2 commits June 6, 2018 00:15

issue hashicorp#4713 add new resource aws_neptune_subnet_group

7a0fc25

correction based on feedback in open PR

41c3125

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

saravanan30erd added 2 commits June 9, 2018 20:31

issue hashicorp#4713 add acceptance test and documentation

5c85945

add pagination support

bdf88ab

saravanan30erd force-pushed the neptune_subnet_group branch from 2c7bd59 to bdf88ab Compare June 9, 2018 16:33

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

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

remove conflicts in config file

dff00c9

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

bflad reviewed Jun 14, 2018

View reviewed changes

corrections based on feedback

8d68464

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jun 14, 2018

remove the unnecessary acceptance test

2f1f9c6

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

bflad approved these changes Jun 15, 2018

View reviewed changes

bflad merged commit 2f1f9c6 into hashicorp:master Jun 15, 2018

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

Update CHANGELOG for #4782

755e8a8

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #4713 New resource aws_neptune_subnet_group #4782

issue #4713 New resource aws_neptune_subnet_group #4782

saravanan30erd commented Jun 7, 2018 •

edited by bflad

Loading

saravanan30erd commented Jun 9, 2018

bflad commented Jun 14, 2018

bflad left a comment

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

saravanan30erd Jun 14, 2018

saravanan30erd Jun 15, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

bflad Jun 14, 2018

saravanan30erd commented Jun 14, 2018

bflad left a comment

bflad Jun 15, 2018

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020

issue #4713 New resource aws_neptune_subnet_group #4782

issue #4713 New resource aws_neptune_subnet_group #4782

Conversation

saravanan30erd commented Jun 7, 2018 • edited by bflad Loading

saravanan30erd commented Jun 9, 2018

bflad commented Jun 14, 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

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 commented Jun 14, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020

saravanan30erd commented Jun 7, 2018 •

edited by bflad

Loading