New resource: aws_neptune_cluster_instance #5376
Conversation
bflad
added
new-resource
There was a problem hiding this comment.
Thanks for contributing this, @saravanan30erd! Overall this is looking pretty good -- a few minor feedback items and it should be good to go. Please let us know if you have any questions or do not have time to address them. 👍
| } | ||
|
|
||
| if err := saveTagsNeptune(conn, d, aws.StringValue(db.DBInstanceArn)); err != nil { | ||
| log.Printf("[WARN] Failed to save tags for Neptune Cluster Instance (%s): %s", aws.StringValue(db.DBInstanceIdentifier), err) |
There was a problem hiding this comment.
We should return an error here instead of just logging 😄
|
|
||
| log.Printf("[DEBUG] Neptune Cluster Instance destroy configuration: %s", opts) | ||
| if _, err := conn.DeleteDBInstance(&opts); err != nil { | ||
| return err |
There was a problem hiding this comment.
We should allow previously deleted Neptune instances to not error on Terraform resource deletion:
if err != nil {
if isAWSErr(err, neptune.ErrCodeDBInstanceNotFoundFault, "") {
return nil
}
return fmt.Errorf("error deleting Neptune cluster instance %q: %s", d.Id(), err)
}|
|
||
| if len(resp.DBInstances) != 1 || | ||
| aws.StringValue(resp.DBInstances[0].DBInstanceIdentifier) != id { | ||
| if err != nil { |
There was a problem hiding this comment.
Anytime err != nil, it is previously returned -- this conditional seems like it should be removed.
| Default: 0, | ||
| }, | ||
|
|
||
| "publicly_accessible": { |
There was a problem hiding this comment.
Is this attribute updatable? It is not handled by the update function currently and should either be added there or have ForceNew: true added
| requestUpdate = true | ||
| } | ||
|
|
||
| if d.HasChange("monitoring_role_arn") { |
There was a problem hiding this comment.
We might want to add an acceptance test to verify that both the MonitoringRoleArn and MonitoringInterval parameters do not need to specified at the same time during update. See also: #2188
There was a problem hiding this comment.
@bflad Looks like there is no support for enhanced monitoring for neptune engine 👍
aws_neptune_cluster_instance.cluster_instances: error creating Neptune Instance: InvalidParameterCombination: Enhanced Monitoring is not supported for the neptune database engine. Go to the RDS documentation for information about supported database engines.
bflad
added
the
waiting-response
|
@bflad done all the changes |
bflad
removed
the
waiting-response
There was a problem hiding this comment.
LGTM with 3 minor things which will be adjusted on merge so we can release this today. Thanks @saravanan30erd 🚀
=== RUN TestAccAWSNeptuneClusterInstance_generatedName
--- PASS: TestAccAWSNeptuneClusterInstance_generatedName (637.04s)
=== RUN TestAccAWSNeptuneClusterInstance_withSubnetGroup
--- PASS: TestAccAWSNeptuneClusterInstance_withSubnetGroup (682.77s)
=== RUN TestAccAWSNeptuneClusterInstance_namePrefix
--- PASS: TestAccAWSNeptuneClusterInstance_namePrefix (707.84s)
=== RUN TestAccAWSNeptuneClusterInstance_withaz
--- PASS: TestAccAWSNeptuneClusterInstance_withaz (726.35s)
=== RUN TestAccAWSNeptuneClusterInstance_kmsKey
--- PASS: TestAccAWSNeptuneClusterInstance_kmsKey (764.09s)
=== RUN TestAccAWSNeptuneClusterInstance_basic
--- PASS: TestAccAWSNeptuneClusterInstance_basic (1438.20s)
| Computed: true, | ||
| }, | ||
|
|
||
| "port": { |
There was a problem hiding this comment.
Looks like this argument is missing documentation
| } | ||
|
|
||
| if db.Endpoint != nil { | ||
| d.Set("endpoint", db.Endpoint.Address) |
There was a problem hiding this comment.
Nitpick: with the aws_db_instance resource we have 3 attributes (address, endpoint, and port):
For a few reasons I think we should follow suit here:
- Provides an attribute with only the hostname
- Matches the API naming for address
- Ancillary: Reduces any confusion working between the two resources
| Read: resourceAwsNeptuneClusterInstanceRead, | ||
| Update: resourceAwsNeptuneClusterInstanceUpdate, | ||
| Delete: resourceAwsNeptuneClusterInstanceDelete, | ||
| Importer: &schema.ResourceImporter{ |
There was a problem hiding this comment.
Resource import documentation is missing
* Add address attribute * Add port argument documentation * Add import documentation * Clarify configurable timeouts documentation * Set identifier_prefix ConflictsWith identifier
|
This has been released in version 1.30.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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! |
ghost
locked and limited conversation to collaborators
Reference #4713
Changes proposed in this pull request:
Added new resource aws_neptune_cluster_instance