From 740b8bb9cbf91064adc47b96fc728e2cac364f5c Mon Sep 17 00:00:00 2001 From: Clint Date: Fri, 2 Sep 2016 09:24:17 -0500 Subject: [PATCH] provider/aws: Run errcheck in tests (#8579) * provider/aws: Add errcheck to Makefile, error on unchecked errors * more exceptions * updates for errcheck to pass * reformat and spilt out the ignore statements * narrow down ignores * fix typo, only ignore Close and Write, instead of close or write --- Makefile | 5 +++- .../aws/import_aws_security_group.go | 5 +++- .../aws/resource_aws_autoscaling_group.go | 9 ++++-- .../aws/resource_aws_dynamodb_table.go | 30 ++++++++++++++----- .../providers/aws/resource_aws_ebs_volume.go | 9 ++++-- .../aws/resource_aws_internet_gateway.go | 7 ++++- .../aws/resource_aws_route53_zone.go | 5 +++- .../providers/aws/resource_aws_s3_bucket.go | 4 ++- .../aws/resource_aws_security_group_rule.go | 5 +++- .../providers/aws/resource_aws_sqs_queue.go | 4 ++- .../aws/resource_aws_ssm_document.go | 12 ++++++-- .../aws/resource_aws_vpn_connection.go | 22 +++++++++----- .../aws/resource_aws_vpn_connection_test.go | 5 +++- scripts/errcheck.sh | 25 ++++++++++++++++ 14 files changed, 117 insertions(+), 30 deletions(-) create mode 100755 scripts/errcheck.sh diff --git a/Makefile b/Makefile index 109f32611..836824e83 100644 --- a/Makefile +++ b/Makefile @@ -39,7 +39,7 @@ plugin-dev: generate mv $(GOPATH)/bin/$(PLUGIN) $(GOPATH)/bin/terraform-$(PLUGIN) # test runs the unit tests -test: fmtcheck generate +test: fmtcheck errcheck generate TF_ACC= go test $(TEST) $(TESTARGS) -timeout=30s -parallel=4 # testacc runs acceptance tests @@ -89,4 +89,7 @@ fmt: fmtcheck: @sh -c "'$(CURDIR)/scripts/gofmtcheck.sh'" +errcheck: + @sh -c "'$(CURDIR)/scripts/errcheck.sh'" + .PHONY: bin default generate test vet fmt fmtcheck tools diff --git a/builtin/providers/aws/import_aws_security_group.go b/builtin/providers/aws/import_aws_security_group.go index d710169dd..120f66380 100644 --- a/builtin/providers/aws/import_aws_security_group.go +++ b/builtin/providers/aws/import_aws_security_group.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/schema" ) @@ -80,7 +81,9 @@ func resourceAwsSecurityGroupImportState( // XXX If the rule contained more than one source security group, this // will choose one of them. We actually need to create one rule for each // source security group. - setFromIPPerm(d, sg, perm) + if err := setFromIPPerm(d, sg, perm); err != nil { + return nil, errwrap.Wrapf("Error importing AWS Security Group: {{err}}", err) + } results = append(results, d) } } diff --git a/builtin/providers/aws/resource_aws_autoscaling_group.go b/builtin/providers/aws/resource_aws_autoscaling_group.go index a10c96cb4..d7149225b 100644 --- a/builtin/providers/aws/resource_aws_autoscaling_group.go +++ b/builtin/providers/aws/resource_aws_autoscaling_group.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -479,11 +480,15 @@ func resourceAwsAutoscalingGroupUpdate(d *schema.ResourceData, meta interface{}) } if shouldWaitForCapacity { - waitForASGCapacity(d, meta, capacitySatifiedUpdate) + if err := waitForASGCapacity(d, meta, capacitySatifiedUpdate); err != nil { + return errwrap.Wrapf("Error waiting for AutoScaling Group Capacity: {{err}}", err) + } } if d.HasChange("enabled_metrics") { - updateASGMetricsCollection(d, conn) + if err := updateASGMetricsCollection(d, conn); err != nil { + return errwrap.Wrapf("Error updating AutoScaling Group Metrics collection: {{err}}", err) + } } return resourceAwsAutoscalingGroupRead(d, meta) diff --git a/builtin/providers/aws/resource_aws_dynamodb_table.go b/builtin/providers/aws/resource_aws_dynamodb_table.go index 04515b27f..cd8db72e6 100644 --- a/builtin/providers/aws/resource_aws_dynamodb_table.go +++ b/builtin/providers/aws/resource_aws_dynamodb_table.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -342,7 +343,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er dynamodbconn := meta.(*AWSClient).dynamodbconn // Ensure table is active before trying to update - waitForTableToBeActive(d.Id(), meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } if d.HasChange("read_capacity") || d.HasChange("write_capacity") { req := &dynamodb.UpdateTableInput{ @@ -361,7 +364,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er return err } - waitForTableToBeActive(d.Id(), meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } } if d.HasChange("stream_enabled") || d.HasChange("stream_view_type") { @@ -380,7 +385,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er return err } - waitForTableToBeActive(d.Id(), meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } } if d.HasChange("global_secondary_index") { @@ -462,8 +469,13 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er return err } - waitForTableToBeActive(d.Id(), meta) - waitForGSIToBeActive(d.Id(), *gsi.IndexName, meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } + + if err := waitForGSIToBeActive(d.Id(), *gsi.IndexName, meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB GSIT to be active: {{err}}", err) + } } } @@ -488,7 +500,9 @@ func resourceAwsDynamoDbTableUpdate(d *schema.ResourceData, meta interface{}) er return err } - waitForTableToBeActive(d.Id(), meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } } } } @@ -692,7 +706,9 @@ func resourceAwsDynamoDbTableRead(d *schema.ResourceData, meta interface{}) erro func resourceAwsDynamoDbTableDelete(d *schema.ResourceData, meta interface{}) error { dynamodbconn := meta.(*AWSClient).dynamodbconn - waitForTableToBeActive(d.Id(), meta) + if err := waitForTableToBeActive(d.Id(), meta); err != nil { + return errwrap.Wrapf("Error waiting for Dynamo DB Table update: {{err}}", err) + } log.Printf("[DEBUG] DynamoDB delete table: %s", d.Id()) diff --git a/builtin/providers/aws/resource_aws_ebs_volume.go b/builtin/providers/aws/resource_aws_ebs_volume.go index 43914ef6b..550a9c696 100644 --- a/builtin/providers/aws/resource_aws_ebs_volume.go +++ b/builtin/providers/aws/resource_aws_ebs_volume.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" ) @@ -137,7 +138,9 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error d.SetId(*result.VolumeId) if _, ok := d.GetOk("tags"); ok { - setTags(conn, d) + if err := setTags(conn, d); err != nil { + return errwrap.Wrapf("Error setting tags for EBS Volume: {{err}}", err) + } } return readVolume(d, result) @@ -146,7 +149,9 @@ func resourceAwsEbsVolumeCreate(d *schema.ResourceData, meta interface{}) error func resourceAWSEbsVolumeUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ec2conn if _, ok := d.GetOk("tags"); ok { - setTags(conn, d) + if err := setTags(conn, d); err != nil { + return errwrap.Wrapf("Error updating tags for EBS Volume: {{err}}", err) + } } return resourceAwsEbsVolumeRead(d, meta) } diff --git a/builtin/providers/aws/resource_aws_internet_gateway.go b/builtin/providers/aws/resource_aws_internet_gateway.go index a6c576386..9aec1d68e 100644 --- a/builtin/providers/aws/resource_aws_internet_gateway.go +++ b/builtin/providers/aws/resource_aws_internet_gateway.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" ) @@ -48,7 +49,7 @@ func resourceAwsInternetGatewayCreate(d *schema.ResourceData, meta interface{}) d.SetId(*ig.InternetGatewayId) log.Printf("[INFO] InternetGateway ID: %s", d.Id()) - resource.Retry(5*time.Minute, func() *resource.RetryError { + err = resource.Retry(5*time.Minute, func() *resource.RetryError { igRaw, _, err := IGStateRefreshFunc(conn, d.Id())() if igRaw != nil { return nil @@ -60,6 +61,10 @@ func resourceAwsInternetGatewayCreate(d *schema.ResourceData, meta interface{}) } }) + if err != nil { + return errwrap.Wrapf("{{err}}", err) + } + err = setTags(conn, d) if err != nil { return err diff --git a/builtin/providers/aws/resource_aws_route53_zone.go b/builtin/providers/aws/resource_aws_route53_zone.go index c97e9a4b3..69f74cec6 100644 --- a/builtin/providers/aws/resource_aws_route53_zone.go +++ b/builtin/providers/aws/resource_aws_route53_zone.go @@ -7,6 +7,7 @@ import ( "strings" "time" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -265,7 +266,9 @@ func resourceAwsRoute53ZoneDelete(d *schema.ResourceData, meta interface{}) erro r53 := meta.(*AWSClient).r53conn if d.Get("force_destroy").(bool) { - deleteAllRecordsInHostedZoneId(d.Id(), d.Get("name").(string), r53) + if err := deleteAllRecordsInHostedZoneId(d.Id(), d.Get("name").(string), r53); err != nil { + return errwrap.Wrapf("{{err}}", err) + } } log.Printf("[DEBUG] Deleting Route53 hosted zone: %s (ID: %s)", diff --git a/builtin/providers/aws/resource_aws_s3_bucket.go b/builtin/providers/aws/resource_aws_s3_bucket.go index e87d893b3..f4637d93e 100644 --- a/builtin/providers/aws/resource_aws_s3_bucket.go +++ b/builtin/providers/aws/resource_aws_s3_bucket.go @@ -1363,7 +1363,9 @@ func normalizeRoutingRules(w []*s3.RoutingRule) (string, error) { } var rules []map[string]interface{} - json.Unmarshal(withNulls, &rules) + if err := json.Unmarshal(withNulls, &rules); err != nil { + return "", err + } var cleanRules []map[string]interface{} for _, rule := range rules { diff --git a/builtin/providers/aws/resource_aws_security_group_rule.go b/builtin/providers/aws/resource_aws_security_group_rule.go index ef34e9122..e112878a5 100644 --- a/builtin/providers/aws/resource_aws_security_group_rule.go +++ b/builtin/providers/aws/resource_aws_security_group_rule.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -250,7 +251,9 @@ func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{}) log.Printf("[DEBUG] Found rule for Security Group Rule (%s): %s", d.Id(), rule) d.Set("type", ruleType) - setFromIPPerm(d, sg, p) + if err := setFromIPPerm(d, sg, p); err != nil { + return errwrap.Wrapf("Error setting IP Permission for Security Group Rule: {{err}}", err) + } return nil } diff --git a/builtin/providers/aws/resource_aws_sqs_queue.go b/builtin/providers/aws/resource_aws_sqs_queue.go index cb76fbb45..ba600ca86 100644 --- a/builtin/providers/aws/resource_aws_sqs_queue.go +++ b/builtin/providers/aws/resource_aws_sqs_queue.go @@ -169,7 +169,9 @@ func resourceAwsSqsQueueUpdate(d *schema.ResourceData, meta interface{}) error { QueueUrl: aws.String(d.Id()), Attributes: attributes, } - sqsconn.SetQueueAttributes(req) + if _, err := sqsconn.SetQueueAttributes(req); err != nil { + return fmt.Errorf("[ERR] Error updating SQS attributes: %s", err) + } } return resourceAwsSqsQueueRead(d, meta) diff --git a/builtin/providers/aws/resource_aws_ssm_document.go b/builtin/providers/aws/resource_aws_ssm_document.go index 94dbaa0d3..c45575455 100644 --- a/builtin/providers/aws/resource_aws_ssm_document.go +++ b/builtin/providers/aws/resource_aws_ssm_document.go @@ -119,7 +119,9 @@ func resourceAwsSsmDocumentCreate(d *schema.ResourceData, meta interface{}) erro d.SetId(*resp.DocumentDescription.Name) if v, ok := d.GetOk("permissions"); ok && v != nil { - setDocumentPermissions(d, meta) + if err := setDocumentPermissions(d, meta); err != nil { + return err + } } else { log.Printf("[DEBUG] Not setting permissions for %q", d.Id()) } @@ -189,7 +191,9 @@ func resourceAwsSsmDocumentRead(d *schema.ResourceData, meta interface{}) error func resourceAwsSsmDocumentUpdate(d *schema.ResourceData, meta interface{}) error { if _, ok := d.GetOk("permissions"); ok { - setDocumentPermissions(d, meta) + if err := setDocumentPermissions(d, meta); err != nil { + return err + } } else { log.Printf("[DEBUG] Not setting document permissions on %q", d.Id()) } @@ -200,7 +204,9 @@ func resourceAwsSsmDocumentUpdate(d *schema.ResourceData, meta interface{}) erro func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) error { ssmconn := meta.(*AWSClient).ssmconn - deleteDocumentPermissions(d, meta) + if err := deleteDocumentPermissions(d, meta); err != nil { + return err + } log.Printf("[INFO] Deleting SSM Document: %s", d.Id()) diff --git a/builtin/providers/aws/resource_aws_vpn_connection.go b/builtin/providers/aws/resource_aws_vpn_connection.go index 020af898b..1fe3844f7 100644 --- a/builtin/providers/aws/resource_aws_vpn_connection.go +++ b/builtin/providers/aws/resource_aws_vpn_connection.go @@ -12,6 +12,7 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -309,11 +310,14 @@ func resourceAwsVpnConnectionRead(d *schema.ResourceData, meta interface{}) erro d.Set("customer_gateway_configuration", vpnConnection.CustomerGatewayConfiguration) if vpnConnection.CustomerGatewayConfiguration != nil { - tunnelInfo := xmlConfigToTunnelInfo(*vpnConnection.CustomerGatewayConfiguration) - d.Set("tunnel1_address", tunnelInfo.Tunnel1Address) - d.Set("tunnel1_preshared_key", tunnelInfo.Tunnel1PreSharedKey) - d.Set("tunnel2_address", tunnelInfo.Tunnel2Address) - d.Set("tunnel2_preshared_key", tunnelInfo.Tunnel2PreSharedKey) + if tunnelInfo, err := xmlConfigToTunnelInfo(*vpnConnection.CustomerGatewayConfiguration); err != nil { + log.Printf("[ERR] Error unmarshaling XML configuration for (%s): %s", d.Id(), err) + } else { + d.Set("tunnel1_address", tunnelInfo.Tunnel1Address) + d.Set("tunnel1_preshared_key", tunnelInfo.Tunnel1PreSharedKey) + d.Set("tunnel2_address", tunnelInfo.Tunnel2Address) + d.Set("tunnel2_preshared_key", tunnelInfo.Tunnel2PreSharedKey) + } } if err := d.Set("vgw_telemetry", telemetryToMapList(vpnConnection.VgwTelemetry)); err != nil { @@ -416,9 +420,11 @@ func telemetryToMapList(telemetry []*ec2.VgwTelemetry) []map[string]interface{} return result } -func xmlConfigToTunnelInfo(xmlConfig string) TunnelInfo { +func xmlConfigToTunnelInfo(xmlConfig string) (*TunnelInfo, error) { var vpnConfig XmlVpnConnectionConfig - xml.Unmarshal([]byte(xmlConfig), &vpnConfig) + if err := xml.Unmarshal([]byte(xmlConfig), &vpnConfig); err != nil { + return nil, errwrap.Wrapf("Error Unmarshalling XML: {{err}}", err) + } // don't expect consistent ordering from the XML sort.Sort(vpnConfig) @@ -431,5 +437,5 @@ func xmlConfigToTunnelInfo(xmlConfig string) TunnelInfo { Tunnel2PreSharedKey: vpnConfig.Tunnels[1].PreSharedKey, } - return tunnelInfo + return &tunnelInfo, nil } diff --git a/builtin/providers/aws/resource_aws_vpn_connection_test.go b/builtin/providers/aws/resource_aws_vpn_connection_test.go index fd5cb1a2d..b22469b21 100644 --- a/builtin/providers/aws/resource_aws_vpn_connection_test.go +++ b/builtin/providers/aws/resource_aws_vpn_connection_test.go @@ -119,7 +119,10 @@ func testAccAwsVpnConnection( } func TestAWSVpnConnection_xmlconfig(t *testing.T) { - tunnelInfo := xmlConfigToTunnelInfo(testAccAwsVpnTunnelInfoXML) + tunnelInfo, err := xmlConfigToTunnelInfo(testAccAwsVpnTunnelInfoXML) + if err != nil { + t.Fatalf("Error unmarshalling XML: %s", err) + } if tunnelInfo.Tunnel1Address != "FIRST_ADDRESS" { t.Fatalf("First address from tunnel XML was incorrect.") } diff --git a/scripts/errcheck.sh b/scripts/errcheck.sh new file mode 100755 index 000000000..6c7ea99d2 --- /dev/null +++ b/scripts/errcheck.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash + +# Check gofmt +echo "==> Checking AWS provider for unchecked errors..." +echo "==> NOTE: at this time we only look for uncheck errors in the AWS package" + +if ! which errcheck > /dev/null; then + echo "==> Installing errcheck..." + go get -u github.com/kisielk/errcheck +fi + +err_files=$(errcheck -ignoretests -ignore \ + 'github.com/hashicorp/terraform/helper/schema:Set' \ + -ignore 'bytes:.*' \ + -ignore 'io:Close|Write' \ + ./builtin/providers/aws/...) + +if [[ -n ${err_files} ]]; then + echo 'Unchecked errors found in the following places:' + echo "${err_files}" + echo "Please handle returned errors. You can check directly with \`make errcheck\`" + exit 1 +fi + +exit 0