Skip to content

Commit

Permalink
d/aws_elb r/aws_elb: hashicorp#2004 review comments
Browse files Browse the repository at this point in the history
* Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic
* Replace unnecessary errwrap.Wrapf with fmt.Errorf
* Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta
* Properly name TestAccDataSourceAWSELB_basic resources
* Use t.Name() for description and TestName tags
  • Loading branch information
bflad authored and mdlavin committed Dec 9, 2017
1 parent 98855fb commit cfc6693
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 25 deletions.
5 changes: 2 additions & 3 deletions aws/data_source_aws_elb.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/elb"
"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/helper/schema"
)

Expand Down Expand Up @@ -202,12 +201,12 @@ func dataSourceAwsElbRead(d *schema.ResourceData, meta interface{}) error {
log.Printf("[DEBUG] Reading ELBs: %#v", input)
resp, err := elbconn.DescribeLoadBalancers(input)
if err != nil {
return errwrap.Wrapf("Error retrieving LB: {{err}}", err)
return fmt.Errorf("Error retrieving LB: %s", err)
}
if len(resp.LoadBalancerDescriptions) != 1 {
return fmt.Errorf("Search returned %d results, please revise so only one is returned", len(resp.LoadBalancerDescriptions))
}
d.SetId(*resp.LoadBalancerDescriptions[0].LoadBalancerName)

return flattenAwsELbResource(d, meta, resp.LoadBalancerDescriptions[0])
return flattenAwsELbResource(d, meta.(*AWSClient).ec2conn, elbconn, resp.LoadBalancerDescriptions[0])
}
31 changes: 16 additions & 15 deletions aws/data_source_aws_elb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,24 @@ import (
)

func TestAccDataSourceAWSELB_basic(t *testing.T) {
lbName := fmt.Sprintf("testaccawselb-basic-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
// Must be less than 32 characters for ELB name
rName := fmt.Sprintf("TestAccDataSourceAWSELB-%s", acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum))

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccDataSourceAWSELBConfigBasic(lbName),
Config: testAccDataSourceAWSELBConfigBasic(rName, t.Name()),
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "name", lbName),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "name", rName),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "cross_zone_load_balancing", "true"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "idle_timeout", "30"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "internal", "true"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "subnets.#", "2"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "security_groups.#", "1"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "tags.%", "1"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "tags.TestName", "TestAccAWSELB_basic"),
resource.TestCheckResourceAttr("data.aws_elb.elb_test", "tags.TestName", t.Name()),
resource.TestCheckResourceAttrSet("data.aws_elb.elb_test", "dns_name"),
resource.TestCheckResourceAttrSet("data.aws_elb.elb_test", "zone_id"),
),
Expand All @@ -34,15 +35,15 @@ func TestAccDataSourceAWSELB_basic(t *testing.T) {
})
}

func testAccDataSourceAWSELBConfigBasic(lbName string) string {
return fmt.Sprintf(`resource "aws_elb" "elb_test" {
name = "%s"
func testAccDataSourceAWSELBConfigBasic(rName, testName string) string {
return fmt.Sprintf(`
resource "aws_elb" "elb_test" {
name = "%[1]s"
internal = true
security_groups = ["${aws_security_group.elb_test.id}"]
subnets = ["${aws_subnet.elb_test.*.id}"]
idle_timeout = 30
enable_deletion_protection = false
listener {
instance_port = 80
Expand All @@ -52,7 +53,7 @@ func testAccDataSourceAWSELBConfigBasic(lbName string) string {
}
tags {
TestName = "TestAccAWSELB_basic"
TestName = "%[2]s"
}
}
Expand All @@ -67,7 +68,7 @@ resource "aws_vpc" "elb_test" {
cidr_block = "10.0.0.0/16"
tags {
TestName = "TestAccAWSELB_basic"
TestName = "%[2]s"
}
}
Expand All @@ -79,13 +80,13 @@ resource "aws_subnet" "elb_test" {
availability_zone = "${element(data.aws_availability_zones.available.names, count.index)}"
tags {
TestName = "TestAccAWSELB_basic"
TestName = "%[2]s"
}
}
resource "aws_security_group" "elb_test" {
name = "allow_all_elb_test"
description = "Used for ALB Testing"
name = "%[1]s"
description = "%[2]s"
vpc_id = "${aws_vpc.elb_test.id}"
ingress {
Expand All @@ -103,11 +104,11 @@ resource "aws_security_group" "elb_test" {
}
tags {
TestName = "TestAccAWSELB_basic"
TestName = "%[2]s"
}
}
data "aws_elb" "elb_test" {
name = "${aws_elb.elb_test.name}"
}`, lbName)
}`, rName, testName)
}
11 changes: 4 additions & 7 deletions aws/resource_aws_elb.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,13 +374,11 @@ func resourceAwsElbRead(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("Unable to find ELB: %#v", describeResp.LoadBalancerDescriptions)
}

return flattenAwsELbResource(d, meta, describeResp.LoadBalancerDescriptions[0])
return flattenAwsELbResource(d, meta.(*AWSClient).ec2conn, elbconn, describeResp.LoadBalancerDescriptions[0])
}

// flattenAwsELbResource takes a *elbv2.LoadBalancer and populates all respective resource fields.
func flattenAwsELbResource(d *schema.ResourceData, meta interface{}, lb *elb.LoadBalancerDescription) error {
elbconn := meta.(*AWSClient).elbconn

func flattenAwsELbResource(d *schema.ResourceData, ec2conn *ec2.EC2, elbconn *elb.ELB, lb *elb.LoadBalancerDescription) error {
describeAttrsOpts := &elb.DescribeLoadBalancerAttributesInput{
LoadBalancerName: aws.String(d.Id()),
}
Expand Down Expand Up @@ -415,7 +413,7 @@ func flattenAwsELbResource(d *schema.ResourceData, meta interface{}, lb *elb.Loa
var elbVpc string
if lb.VPCId != nil {
elbVpc = *lb.VPCId
sgId, err := sourceSGIdByName(meta, *lb.SourceSecurityGroup.GroupName, elbVpc)
sgId, err := sourceSGIdByName(ec2conn, *lb.SourceSecurityGroup.GroupName, elbVpc)
if err != nil {
return fmt.Errorf("[WARN] Error looking up ELB Security Group ID: %s", err)
} else {
Expand Down Expand Up @@ -849,8 +847,7 @@ func isLoadBalancerNotFound(err error) bool {
return ok && elberr.Code() == "LoadBalancerNotFound"
}

func sourceSGIdByName(meta interface{}, sg, vpcId string) (string, error) {
conn := meta.(*AWSClient).ec2conn
func sourceSGIdByName(conn *ec2.EC2, sg, vpcId string) (string, error) {
var filters []*ec2.Filter
var sgFilterName, sgFilterVPCID *ec2.Filter
sgFilterName = &ec2.Filter{
Expand Down

0 comments on commit cfc6693

Please sign in to comment.