New Data Source: aws_redshift_cluster #2603
Conversation
kl4w
changed the title
radeksimko
added
the
service/redshift
radeksimko
changed the title
There was a problem hiding this comment.
Thanks so much for this @kl4w and sorry for the delayed review. 😅 This looks great with a few minor things I'll fix on merge. 🚀
3 tests passed (all tests)
=== RUN TestAccAWSDataSourceRedshiftCluster_logging
--- PASS: TestAccAWSDataSourceRedshiftCluster_logging (979.28s)
=== RUN TestAccAWSDataSourceRedshiftCluster_basic
--- PASS: TestAccAWSDataSourceRedshiftCluster_basic (1114.10s)
=== RUN TestAccAWSDataSourceRedshiftCluster_vpc
--- PASS: TestAccAWSDataSourceRedshiftCluster_vpc (1295.04s)
| return fmt.Errorf("Error describing Redshift Cluster: %s, error: %#v", cluster, err) | ||
| } | ||
|
|
||
| rsc := *resp.Clusters[0] |
There was a problem hiding this comment.
To prevent panics, we should perform nil and len() checks before here. I will fix this on merge.
| d.Set("automated_snapshot_retention_period", rsc.AutomatedSnapshotRetentionPeriod) | ||
| d.Set("availability_zone", rsc.AvailabilityZone) | ||
| d.Set("cluster_identifier", rsc.ClusterIdentifier) | ||
| d.Set("cluster_parameter_group_name", rsc.ClusterParameterGroups[0].ParameterGroupName) |
There was a problem hiding this comment.
To prevent panics, we should perform nil and len() checks before here. I will fix this on merge.
| }) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("Error describing Redshift Cluster: %s, error: %#v", cluster, err) |
There was a problem hiding this comment.
We can use the %s (string) value of AWS SDK errors which should make the output more readable. I will fix this on merge.
| return loggingErr | ||
| } | ||
|
|
||
| if *loggingStatus.LoggingEnabled { |
There was a problem hiding this comment.
To prevent panics, we should perform nil check on loggingStatus as well -- rarely the AWS API can return no error and empty objects. I will fix this on merge.
| { | ||
| Config: testAccAWSDataSourceRedshiftClusterConfig(rInt), | ||
| Check: resource.ComposeAggregateTestCheckFunc( | ||
| resource.TestCheckResourceAttrSet("data.aws_redshift_cluster.test", "allow_version_upgrade"), |
There was a problem hiding this comment.
FYI we can actually test the data source attribute values against the resource attribute values using resource.TestCheckResourceAttrPair(), which is a slightly better check.
| Provides details about a specific redshift cluster | ||
| --- | ||
|
|
||
| # aws\_redshift\_cluster |
There was a problem hiding this comment.
This PR predates the removal of extraneous forward slashes. I will remove them on merge. 👍
|
This has been released in version 1.16.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
TF_ACC=1 go test ./aws -v -run=TestAccAWSDataSourceRedshiftCluster -timeout 120m === RUN TestAccAWSDataSourceRedshiftCluster_basic --- PASS: TestAccAWSDataSourceRedshiftCluster_basic (911.35s) === RUN TestAccAWSDataSourceRedshiftCluster_vpc --- PASS: TestAccAWSDataSourceRedshiftCluster_vpc (1121.35s) === RUN TestAccAWSDataSourceRedshiftCluster_logging --- PASS: TestAccAWSDataSourceRedshiftCluster_logging (779.21s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 2811.937s