aws_db_cluster_snapshot resource and data source #4526
Conversation
…aws_db_snapshot ``` % TESTARGS="-run TestAccAwsDbClusterSnapshotDataSource_basic" make testacc ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./... -v -run TestAccAwsDbClusterSnapshotDataSource_basic -timeout 120m ? github.com/terraform-providers/terraform-provider-aws [no test files] === RUN TestAccAwsDbClusterSnapshotDataSource_basic --- PASS: TestAccAwsDbClusterSnapshotDataSource_basic (693.03s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 693.069s % TESTARGS="-run TestAccAWSDBClusterSnapshot_basic" make testacc ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./... -v -run TestAccAWSDBClusterSnapshot_basic -timeout 120m ? github.com/terraform-providers/terraform-provider-aws [no test files] === RUN TestAccAWSDBClusterSnapshot_basic --- PASS: TestAccAWSDBClusterSnapshot_basic (699.86s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 699.907s ```
bflad
added
new-resource
There was a problem hiding this comment.
Hi @4dz 👋 Thanks so much for submitting this and sorry for the lengthy delay in getting it reviewed. 😅 Overall this pull request is in decent shape, but I left some comments below. Normally we'd allow you to address these, however we have an immediate need for this resource to perform some additional acceptance testing with the aws_rds_cluster resource snapshot_identifier argument. I'll add a follow up commit after yours which addresses these items so we can get this in.
Thanks again! 🚀
| "db_cluster_identifier": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
Minor nitpick: ForceNew: true for data source attributes should be extraneous as data sources are implicitly refreshed each Terraform run.
| snapshotIdentifier, snapshotIdentifierOk := d.GetOk("db_cluster_snapshot_identifier") | ||
|
|
||
| if !clusterIdentifierOk && !snapshotIdentifierOk { | ||
| return fmt.Errorf("One of db_cluster_snapshot_identifier or db_cluster_identifier must be assigned") |
There was a problem hiding this comment.
Minor lint nitpick: When creating a fmt.Errorf() without extra arguments, we should prefer errors.New()
| } | ||
|
|
||
| func DBClusterSnapshotDescriptionAttributes(d *schema.ResourceData, snapshot *rds.DBClusterSnapshot) error { | ||
| d.SetId(*snapshot.DBClusterSnapshotIdentifier) |
There was a problem hiding this comment.
To prevent potential panics with nil dereferences, we should use the SDK provided function to dereference this attribute instead. e.g. aws.StringValue(snapshot.DBClusterSnapshotIdentifier)
|
|
||
| func DBClusterSnapshotDescriptionAttributes(d *schema.ResourceData, snapshot *rds.DBClusterSnapshot) error { | ||
| d.SetId(*snapshot.DBClusterSnapshotIdentifier) | ||
| d.Set("allocated_storage", snapshot.AllocatedStorage) |
There was a problem hiding this comment.
These attributes should be covered by an acceptance test check 😄
| { | ||
| Config: testAccCheckAwsDbClusterSnapshotDataSourceConfig(rInt), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testAccCheckAwsDbClusterSnapshotDataSourceID("data.aws_db_cluster_snapshot.snapshot"), |
There was a problem hiding this comment.
When testing data sources that have closely matching resource counterparts, we recommend comparing the attribute values between the two, e.g.
resource.TestCheckResourceAttrPair("data.aws_db_cluster_snapshot.snapshot", "allocated_storage", "aws_db_cluster_snapshot.test", "allocated_storage"),|
|
||
| resp, err := conn.DescribeDBClusterSnapshots(opts) | ||
| if err != nil { | ||
| snapshoterr, ok := err.(awserr.Error) |
There was a problem hiding this comment.
Minor nitpick: We have a helper function for this type of check to reduce code complexity and should also prefer to use the SDK provided constant. e.g.
if isAWSErr(err, rds.ErrCodeDBClusterSnapshotNotFoundFault, "") {|
|
||
| snapshot := resp.DBClusterSnapshots[0] | ||
|
|
||
| return resp, *snapshot.Status, nil |
There was a problem hiding this comment.
To prevent potential panics with nil dereferences, we should use the SDK provided function to dereference this attribute instead. e.g. aws.StringValue(snapshot.Status)
| } | ||
| _, err := conn.DeleteDBClusterSnapshot(params) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
When expecting to delete this resource, we should allow it to pass without error when the underlying resource has already been deleted. In this case:
if err != nil {
if isAWSErr(err, rds.ErrCodeDBClusterSnapshotNotFoundFault, "") {
return nil
}
return fmt.Errorf("error deleting RDS DB Cluster Snapshot %q: %s", d.Id(), err)
}| } | ||
| resp, err := conn.DescribeDBClusterSnapshots(params) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
When expecting to read this resource, we should allow it to pass without error by removing the resource from the Terraform state (to force recreation) when the underlying resource has already been deleted. In this case:
if err != nil {
if isAWSErr(err, rds.ErrCodeDBClusterSnapshotNotFoundFault, "") {
log.Printf("[WARN] RDS DB Cluster Snapshot %q not found, removing from state", d.Id())
d.SetId("")
return nil
}
return fmt.Errorf("error reading RDS DB Cluster Snapshot %q: %s", d.Id(), err)
}| return err | ||
| } | ||
|
|
||
| snapshot := resp.DBClusterSnapshots[0] |
There was a problem hiding this comment.
To prevent potential panics and ensure our snapshot matches what we expect, we should perform some additional checking before performing this reference:
if resp == nil || len(resp.DBClusterSnapshots) == 0 || resp.DBClusterSnapshots[0] == nil || aws.StringValue(resp.DBClusterSnapshots[0].DBClusterSnapshotIdentifer) != d.Id() {
log.Printf("[WARN] RDS DB Cluster Snapshot %q not found, removing from state", d.Id())
d.SetId("")
return nil
}=== RUN TestAccAWSDBClusterSnapshot_basic --- PASS: TestAccAWSDBClusterSnapshot_basic (146.93s) === RUN TestAccAWSDbClusterSnapshotDataSource_DbClusterSnapshotIdentifier --- PASS: TestAccAWSDbClusterSnapshotDataSource_DbClusterSnapshotIdentifier (128.45s) === RUN TestAccAWSDbClusterSnapshotDataSource_DbClusterIdentifier --- PASS: TestAccAWSDbClusterSnapshotDataSource_DbClusterIdentifier (137.24s) === RUN TestAccAWSDbClusterSnapshotDataSource_MostRecent --- PASS: TestAccAWSDbClusterSnapshotDataSource_MostRecent (197.72s)
|
Wow that’s great, and some good feedback too. I found an alternative way to handle my use case but I’m glad it was useful!
… On 8 Aug 2018, at 19:17, Brian Flad ***@***.***> wrote:
Merged #4526 into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
This has been released in version 1.31.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
Implements #4525
This is my first pull request/attempt at writing in Go! Really its plagiarised/heavily influenced by the aws_db_snapshot source code and documentation.
Changes proposed in this pull request:
Output from acceptance testing: