issue #4713 New Resource: aws_neptune_cluster #5050

saravanan30erd · 2018-07-02T15:59:42Z

~~Fixes #4713~~
Reference: #4713

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSNeptuneCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSNeptuneCluster -timeout 120m
=== RUN   TestAccAWSNeptuneClusterParameterGroup_basic
--- PASS: TestAccAWSNeptuneClusterParameterGroup_basic (55.86s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_namePrefix
--- PASS: TestAccAWSNeptuneClusterParameterGroup_namePrefix (54.48s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_generatedName
--- PASS: TestAccAWSNeptuneClusterParameterGroup_generatedName (55.00s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_Description
--- PASS: TestAccAWSNeptuneClusterParameterGroup_Description (55.70s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_Parameter
--- PASS: TestAccAWSNeptuneClusterParameterGroup_Parameter (94.20s)
=== RUN   TestAccAWSNeptuneClusterParameterGroup_Tags
--- PASS: TestAccAWSNeptuneClusterParameterGroup_Tags (134.52s)
=== RUN   TestAccAWSNeptuneCluster_basic
--- PASS: TestAccAWSNeptuneCluster_basic (130.39s)
=== RUN   TestAccAWSNeptuneCluster_namePrefix
--- PASS: TestAccAWSNeptuneCluster_namePrefix (152.58s)
=== RUN   TestAccAWSNeptuneCluster_takeFinalSnapshot
--- PASS: TestAccAWSNeptuneCluster_takeFinalSnapshot (212.69s)
=== RUN   TestAccAWSNeptuneCluster_updateTags
--- PASS: TestAccAWSNeptuneCluster_updateTags (177.25s)
=== RUN   TestAccAWSNeptuneCluster_updateIamRoles
--- PASS: TestAccAWSNeptuneCluster_updateIamRoles (219.52s)
=== RUN   TestAccAWSNeptuneCluster_kmsKey
--- PASS: TestAccAWSNeptuneCluster_kmsKey (196.87s)
=== RUN   TestAccAWSNeptuneCluster_encrypted
--- PASS: TestAccAWSNeptuneCluster_encrypted (142.91s)
=== RUN   TestAccAWSNeptuneCluster_backupsUpdate
--- PASS: TestAccAWSNeptuneCluster_backupsUpdate (199.69s)
=== RUN   TestAccAWSNeptuneCluster_iamAuth
--- PASS: TestAccAWSNeptuneCluster_iamAuth (130.87s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2012.587s
...

austinkelleher · 2018-07-05T20:57:01Z

Awesome! We are using Neptune and would really love to see this get merged! Thank you for all of your work on this @saravanan30erd!

bflad

Hi @saravanan30erd 👋 Thanks for submitting this! Please see the below for some initial feedback. Let us know if you have any questions or do not have time to implement these, thanks. Testing looks good so far 👍

bflad · 2018-07-17T02:16:26Z

aws/resource_aws_neptune_cluster.go

+			"cluster_members": {
+				Type:     schema.TypeSet,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+				Optional: true,


It is not valid to assign cluster_members in configurations, so Optional: true should be removed from this attribute.

bflad · 2018-07-17T02:21:00Z

aws/resource_aws_neptune_cluster.go

+			"neptune_cluster_parameter_group_name": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,


We might want to consider setting Default: "default.neptune1" or adding a DiffSuppressFunc here to properly allow Terraform to detect API drift and also allow the argument to be omitted instead of Computed: true. This could cause bug reports.

bflad · 2018-07-17T02:23:52Z

aws/resource_aws_neptune_cluster.go

+			Delete: schema.DefaultTimeout(120 * time.Minute),
+		},
+
+		Schema: map[string]*schema.Schema{


Nitpick: The attributes can be hard to follow because they are not in alphabetical order.

bflad · 2018-07-17T02:25:28Z

aws/resource_aws_neptune_cluster.go

+
+			"snapshot_identifier": {
+				Type:     schema.TypeString,
+				Computed: false,


Computed: false here is extraneous

bflad · 2018-07-17T02:26:34Z

aws/resource_aws_neptune_cluster.go

+			"port": {
+				Type:     schema.TypeInt,
+				Optional: true,
+				Computed: true,


This should be Default: 8182 and not Computed: true to properly detect drift from the API.

bflad · 2018-07-17T02:50:43Z

aws/resource_aws_neptune_cluster.go

+	d.Set("arn", arn)
+
+	if err := saveTagsNeptune(conn, d, arn); err != nil {
+		log.Printf("[WARN] Failed to save tags for Neptune Cluster (%s): %s", aws.StringValue(dbc.DBClusterIdentifier), err)


We should always prefer to return an error message and not just log this condition.

bflad · 2018-07-17T02:57:44Z

website/aws.erb

@@ -1677,6 +1677,10 @@
                            <a href="/docs/providers/aws/r/neptune_cluster_parameter_group.html">aws_neptune_cluster_parameter_group</a>
                        </li>

+                         <li<%= sidebar_current("docs-aws-resource-neptune-cluster") %>>


Minor nitpick: Due to the implementation of sidebar_current we should append a -x here and in the markdown page front matter to prevent the sidebar highlighting of both this resource and aws_neptune_cluster_parameter_group

bflad · 2018-07-17T02:59:31Z

website/docs/r/neptune_cluster.html.markdown

+  engine                  = "neptune"
+  backup_retention_period = 5
+  preferred_backup_window = "07:00-09:00"
+  skip_final_snapshot = "true"


Two items:

We should remove the quotes around true for here and below.

Minor nitpick: The example resource should have its = aligned to match terraform fmt output

bflad · 2018-07-17T03:03:52Z

website/docs/r/neptune_cluster.html.markdown

+
+The following arguments are supported:
+
+* `cluster_identifier` - (Optional, Forces new resources) The cluster identifier. If omitted, Terraform will assign a random, unique identifier.


The arguments should be alphabetically sorted, they are fairly hard to follow with the current ordering.

bflad · 2018-07-17T03:04:48Z

website/docs/r/neptune_cluster.html.markdown

+* `cluster_identifier` - The Neptune Cluster Identifier
+* `cluster_resource_id` - The Neptune Cluster Resource ID
+* `cluster_members` – List of Neptune Instances that are a part of this cluster
+* `availability_zones` - The availability zone of the instance


The attributes byline states In addition to all arguments above -- we should remove all arguments from this attributes list and alphabetically sort the remainder

saravanan30erd · 2018-07-17T19:15:01Z

@bflad updated the recommended changes 👍

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSNeptuneCluster_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -run=TestAccAWSNeptuneCluster_ -timeout 120m
=== RUN TestAccAWSNeptuneCluster_basic
--- PASS: TestAccAWSNeptuneCluster_basic (128.93s)
=== RUN TestAccAWSNeptuneCluster_namePrefix
--- PASS: TestAccAWSNeptuneCluster_namePrefix (139.78s)
=== RUN TestAccAWSNeptuneCluster_takeFinalSnapshot
--- PASS: TestAccAWSNeptuneCluster_takeFinalSnapshot (152.37s)
=== RUN TestAccAWSNeptuneCluster_updateTags
--- PASS: TestAccAWSNeptuneCluster_updateTags (177.87s)
=== RUN TestAccAWSNeptuneCluster_updateIamRoles
--- PASS: TestAccAWSNeptuneCluster_updateIamRoles (212.79s)
=== RUN TestAccAWSNeptuneCluster_kmsKey
--- PASS: TestAccAWSNeptuneCluster_kmsKey (192.41s)
=== RUN TestAccAWSNeptuneCluster_encrypted
--- PASS: TestAccAWSNeptuneCluster_encrypted (138.92s)
=== RUN TestAccAWSNeptuneCluster_backupsUpdate
--- PASS: TestAccAWSNeptuneCluster_backupsUpdate (167.89s)
=== RUN TestAccAWSNeptuneCluster_iamAuth
--- PASS: TestAccAWSNeptuneCluster_iamAuth (149.48s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 1460.477s

bflad

Great work, @saravanan30erd! 🚀

=== RUN   TestAccAWSNeptuneCluster_iamAuth
--- PASS: TestAccAWSNeptuneCluster_iamAuth (88.37s)
=== RUN   TestAccAWSNeptuneCluster_namePrefix
--- PASS: TestAccAWSNeptuneCluster_namePrefix (88.60s)
=== RUN   TestAccAWSNeptuneCluster_basic
--- PASS: TestAccAWSNeptuneCluster_basic (88.62s)
=== RUN   TestAccAWSNeptuneCluster_encrypted
--- PASS: TestAccAWSNeptuneCluster_encrypted (88.68s)
=== RUN   TestAccAWSNeptuneCluster_kmsKey
--- PASS: TestAccAWSNeptuneCluster_kmsKey (107.79s)
=== RUN   TestAccAWSNeptuneCluster_backupsUpdate
--- PASS: TestAccAWSNeptuneCluster_backupsUpdate (113.19s)
=== RUN   TestAccAWSNeptuneCluster_updateIamRoles
--- PASS: TestAccAWSNeptuneCluster_updateIamRoles (114.10s)
=== RUN   TestAccAWSNeptuneCluster_updateTags
--- PASS: TestAccAWSNeptuneCluster_updateTags (133.03s)
=== RUN   TestAccAWSNeptuneCluster_takeFinalSnapshot
--- PASS: TestAccAWSNeptuneCluster_takeFinalSnapshot (159.10s)

bflad · 2018-07-17T23:28:31Z

aws/resource_aws_neptune_cluster.go

+
+	// Check if any of the parameters that require a cluster modification after creation are set
+	var clusterUpdate bool
+	clusterUpdate = false


Minor nitpick: Boolean zero values are false so this line is extraneous. Reference: https://tour.golang.org/basics/12

Otherwise this could be declared as: clusterUpdate := false

bflad · 2018-07-18T20:43:15Z

This has been released in version 1.28.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-04T17:37:39Z

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 11 commits June 27, 2018 22:27

add schema for neptune cluster

454db9d

create cluster from snapshot

cc5b78e

create cluster from input param

6110e87

add read func for neptune cluster

8fd3da2

add update func for neptune cluster

dbba6f4

add delete func for neptune cluster

39f045e

remove invalid parameters

65e4c7d

Fix update func pending state variable

cd6f391

add basic acceptance test

aa7d4bb

acceptance tests for remaining features

e2a7ef9

add documentation for aws_neptune_cluster

6294586

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

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

bflad added this to the v1.28.0 milestone Jul 11, 2018

bflad self-requested a review July 11, 2018 19:43

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

add default value to port attribute

5fafe71

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

saravanan30erd added 4 commits July 17, 2018 22:12

deduplicate the cluster update logic

37bac27

return the errors instead of logging

74309fc

correct the documentation

97d8d0d

align the schema order

0ea2b54

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

bflad approved these changes Jul 17, 2018

View reviewed changes

bflad merged commit 4e21c0d into hashicorp:master Jul 17, 2018

bflad added a commit that referenced this pull request Jul 17, 2018

Update CHANGELOG for #5050

5df92f3

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #4713 New Resource: aws_neptune_cluster #5050

issue #4713 New Resource: aws_neptune_cluster #5050

saravanan30erd commented Jul 2, 2018 •

edited by bflad

Loading

austinkelleher commented Jul 5, 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

bflad Jul 17, 2018

bflad Jul 17, 2018

saravanan30erd commented Jul 17, 2018

bflad left a comment

bflad Jul 17, 2018

bflad commented Jul 18, 2018

ghost commented Apr 4, 2020


		The following arguments are supported:

		* `cluster_identifier` - (Optional, Forces new resources) The cluster identifier. If omitted, Terraform will assign a random, unique identifier.

issue #4713 New Resource: aws_neptune_cluster #5050

issue #4713 New Resource: aws_neptune_cluster #5050

Conversation

saravanan30erd commented Jul 2, 2018 • edited by bflad Loading

austinkelleher commented Jul 5, 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

saravanan30erd commented Jul 17, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jul 18, 2018

ghost commented Apr 4, 2020

saravanan30erd commented Jul 2, 2018 •

edited by bflad

Loading