Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #4566 add new attribute dns_name in aws_redshift_cluster #4582

Merged
merged 6 commits into from
May 23, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions aws/resource_aws_redshift_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ func resourceAwsRedshiftCluster() *schema.Resource {
Computed: true,
},

"dns_name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not call this address to match the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but I checked the other resources to see how they are naming the address attributes(aws_alb, aws_elb) and used the same naming convention.

Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This attribute is not configurable so Optional: true, should be removed. https://www.terraform.io/docs/extend/schemas/schema-behaviors.html#optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, I just used the previous output attribute endpoint format.

Computed: true,
},

"cluster_public_key": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -567,6 +573,7 @@ func resourceAwsRedshiftClusterRead(d *schema.ResourceData, meta interface{}) er
if rsc.Endpoint.Port != nil {
endpoint = fmt.Sprintf("%s:%d", endpoint, *rsc.Endpoint.Port)
}
d.Set("dns_name", rsc.Endpoint.Address)
d.Set("port", rsc.Endpoint.Port)
d.Set("endpoint", endpoint)
}
Expand Down
43 changes: 43 additions & 0 deletions aws/resource_aws_redshift_cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,32 @@ func TestAccAWSRedshiftCluster_basic(t *testing.T) {
})
}

func TestAccAWSRedshiftCluster_dnsName(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding a Computed: true attribute that is always set, we can just add the singular attribute check (resource.TestMatchResourceAttr mentioned below) to the _basic test instead of a whole new acceptance test 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

var v redshift.Cluster

ri := rand.New(rand.NewSource(time.Now().UnixNano())).Int()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we have a helper for this: acctest.RandInt()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a bunch of PRs related to replacing all these across the codebase so you won't need to do anything: #4625

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok, I ll revert it.

config := testAccAWSRedshiftClusterConfig_basic(ri)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSRedshiftClusterDestroy,
Steps: []resource.TestStep{
{
Config: config,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSRedshiftClusterExists("aws_redshift_cluster.default", &v),
resource.TestCheckResourceAttr(
"aws_redshift_cluster.default", "cluster_type", "single-node"),
resource.TestCheckResourceAttr(
"aws_redshift_cluster.default", "publicly_accessible", "true"),
testAccCheckAWSRedshiftClusterDNSName(&v, ri),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can verify the value from the state using resource.TestMatchResourceAttr() (and also making it generic for any region/partition):

resource.TestMatchResourceAttr("aws_redshift_cluster.default", "address", regexp.MustCompile(fmt.Sprintf("^tf-redshift-cluster-%d.*\.redshift\..*", ri)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done.

),
},
},
})
}

func TestAccAWSRedshiftCluster_withFinalSnapshot(t *testing.T) {
var v redshift.Cluster

Expand Down Expand Up @@ -630,6 +656,23 @@ func testAccCheckAWSRedshiftClusterMasterUsername(c *redshift.Cluster, value str
}
}

func testAccCheckAWSRedshiftClusterDNSName(c *redshift.Cluster, rInt int) resource.TestCheckFunc {
return func(s *terraform.State) error {
if c.Endpoint == nil {
return fmt.Errorf("Expected cluster Endpoint to be set, got nil")
}

//validate the cluster address
str := fmt.Sprintf("tf-redshift-cluster-%d.?[a-zA-Z0-9].*?.us-west-2.redshift.amazonaws.com", rInt)
re := regexp.MustCompile(str)
if !re.MatchString(*c.Endpoint.Address) {
return fmt.Errorf("Expected cluster DNS name, got %s", *c.Endpoint.Address)
}

return nil
}
}

func TestResourceAWSRedshiftClusterIdentifierValidation(t *testing.T) {
cases := []struct {
Value string
Expand Down
1 change: 1 addition & 0 deletions website/docs/r/redshift_cluster.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ The following attributes are exported:
* `encrypted` - Whether the data in the cluster is encrypted
* `cluster_security_groups` - The security groups associated with the cluster
* `vpc_security_group_ids` - The VPC security group Ids associated with the cluster
* `dns_name` - The DNS name of the cluster
* `port` - The Port the cluster responds on
* `cluster_version` - The version of Redshift engine software
* `cluster_parameter_group_name` - The name of the parameter group to be associated with this cluster
Expand Down