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.

Sign up for GitHub

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

service/servicediscovery: Refactor waiter logic into separate package, add test sweepers #12765

Merged
merged 2 commits into from
Apr 15, 2020
Merged

service/servicediscovery: Refactor waiter logic into separate package, add test sweepers #12765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9d1451
service/servicediscovery: Refactor waiter logic into separate package…
bflad Apr 10, 2020
3078efc
internal/service/servicediscovery/waiter: Use if instead of switch
bflad Apr 15, 2020
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
35 changes: 35 additions & 0 deletions aws/internal/service/servicediscovery/waiter/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package waiter

import (
"fmt"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/servicediscovery"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

// OperationStatus fetches the Operation and its Status
func OperationStatus(conn *servicediscovery.ServiceDiscovery, operationID string) resource.StateRefreshFunc {
return func() (interface{}, string, error) {
input := &servicediscovery.GetOperationInput{
OperationId: aws.String(operationID),
}

output, err := conn.GetOperation(input)

if err != nil {
return nil, servicediscovery.OperationStatusFail, err
}

// Error messages can also be contained in the response with FAIL status
// "ErrorCode":"CANNOT_CREATE_HOSTED_ZONE",
// "ErrorMessage":"The VPC that you chose, vpc-xxx in region xxx, is already associated with another private hosted zone that has an overlapping name space, xxx.. (Service: AmazonRoute53; Status Code: 400; Error Code: ConflictingDomainExists; Request ID: xxx)"
// "Status":"FAIL",

if aws.StringValue(output.Operation.Status) == servicediscovery.OperationStatusFail {
return output, servicediscovery.OperationStatusFail, fmt.Errorf("%s: %s", aws.StringValue(output.Operation.ErrorCode), aws.StringValue(output.Operation.ErrorMessage))
}

return output, aws.StringValue(output.Operation.Status), nil
}
}
32 changes: 32 additions & 0 deletions aws/internal/service/servicediscovery/waiter/waiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package waiter

import (
"time"

"github.com/aws/aws-sdk-go/service/servicediscovery"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)

const (
// Maximum amount of time to wait for an Operation to return Success
OperationSuccessTimeout = 5 * time.Minute
)

// OperationSuccess waits for an Operation to return Success
func OperationSuccess(conn *servicediscovery.ServiceDiscovery, operationID string) (*servicediscovery.GetOperationOutput, error) {
stateConf := &resource.StateChangeConf{
Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
Target: []string{servicediscovery.OperationStatusSuccess},
Refresh: OperationStatus(conn, operationID),
Timeout: OperationSuccessTimeout,
}

outputRaw, err := stateConf.WaitForState()

switch output := outputRaw.(type) {
case *servicediscovery.GetOperationOutput:
return output, err
default:
return nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are only two possibilities, would if v, ok := outputRaw.(*servicediscovery.GetOperationOutput); ok { ... } work better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are functionally equivalent, yes. Will update.

}
57 changes: 31 additions & 26 deletions aws/resource_aws_service_discovery_http_namespace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ package aws

import (
"fmt"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/servicediscovery"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/servicediscovery/waiter"
)

func resourceAwsServiceDiscoveryHttpNamespace() *schema.Resource {
Expand Down Expand Up @@ -54,24 +54,34 @@ func resourceAwsServiceDiscoveryHttpNamespaceCreate(d *schema.ResourceData, meta
input.Description = aws.String(v.(string))
}

resp, err := conn.CreateHttpNamespace(input)
output, err := conn.CreateHttpNamespace(input)

if err != nil {
return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): %s", name, err)
return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): %w", name, err)
}

stateConf := &resource.StateChangeConf{
Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
Target: []string{servicediscovery.OperationStatusSuccess},
Refresh: servicediscoveryOperationRefreshStatusFunc(conn, *resp.OperationId),
Timeout: 5 * time.Minute,
if output == nil || output.OperationId == nil {
return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): creation response missing Operation ID", name)
}

opresp, err := stateConf.WaitForState()
operationOutput, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId))
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better naming 👍


if err != nil {
return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) creation: %s", name, err)
return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) creation: %w", name, err)
}

d.SetId(*opresp.(*servicediscovery.GetOperationOutput).Operation.Targets["NAMESPACE"])
if operationOutput == nil || operationOutput.Operation == nil {
return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): operation response missing Operation information", name)
}

namespaceID, ok := operationOutput.Operation.Targets[servicediscovery.OperationTargetTypeNamespace]

if !ok {
return fmt.Errorf("error creating Service Discovery HTTP Namespace (%s): operation response missing Namespace ID", name)
}

d.SetId(aws.StringValue(namespaceID))

return resourceAwsServiceDiscoveryHttpNamespaceRead(d, meta)
}

Expand Down Expand Up @@ -105,25 +115,20 @@ func resourceAwsServiceDiscoveryHttpNamespaceDelete(d *schema.ResourceData, meta
Id: aws.String(d.Id()),
}

resp, err := conn.DeleteNamespace(input)
if err != nil {
if isAWSErr(err, servicediscovery.ErrCodeNamespaceNotFound, "") {
d.SetId("")
return nil
}
return fmt.Errorf("error deleting Service Discovery HTTP Namespace (%s): %s", d.Id(), err)
}
output, err := conn.DeleteNamespace(input)

stateConf := &resource.StateChangeConf{
Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
Target: []string{servicediscovery.OperationStatusSuccess},
Refresh: servicediscoveryOperationRefreshStatusFunc(conn, *resp.OperationId),
Timeout: 5 * time.Minute,
if isAWSErr(err, servicediscovery.ErrCodeNamespaceNotFound, "") {
return nil
}

_, err = stateConf.WaitForState()
if err != nil {
return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %s", d.Id(), err)
return fmt.Errorf("error deleting Service Discovery HTTP Namespace (%s): %w", d.Id(), err)
}

if output != nil && output.OperationId != nil {
if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil {
return fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %w", d.Id(), err)
}
}

return nil
Expand Down
81 changes: 81 additions & 0 deletions aws/resource_aws_service_discovery_http_namespace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,96 @@ package aws

import (
"fmt"
"log"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/servicediscovery"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform-plugin-sdk/helper/acctest"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/terraform"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/servicediscovery/waiter"
)

func init() {
resource.AddTestSweepers("aws_service_discovery_http_namespace", &resource.Sweeper{
Name: "aws_service_discovery_http_namespace",
F: testSweepServiceDiscoveryHttpNamespaces,
Dependencies: []string{
"aws_service_discovery_service",
},
})
}

func testSweepServiceDiscoveryHttpNamespaces(region string) error {
client, err := sharedClientForRegion(region)
if err != nil {
return fmt.Errorf("error getting client: %s", err)
}
conn := client.(*AWSClient).sdconn
var sweeperErrs *multierror.Error

input := &servicediscovery.ListNamespacesInput{
Filters: []*servicediscovery.NamespaceFilter{
{
Condition: aws.String(servicediscovery.FilterConditionEq),
Name: aws.String(servicediscovery.NamespaceFilterNameType),
Values: aws.StringSlice([]string{servicediscovery.NamespaceTypeHttp}),
},
},
}

err = conn.ListNamespacesPages(input, func(page *servicediscovery.ListNamespacesOutput, isLast bool) bool {
if page == nil {
return !isLast
}

for _, namespace := range page.Namespaces {
if namespace == nil {
continue
}

id := aws.StringValue(namespace.Id)
input := &servicediscovery.DeleteNamespaceInput{
Id: namespace.Id,
}

log.Printf("[INFO] Deleting Service Discovery HTTP Namespace: %s", id)
output, err := conn.DeleteNamespace(input)

if err != nil {
sweeperErr := fmt.Errorf("error deleting Service Discovery HTTP Namespace (%s): %w", id, err)
log.Printf("[ERROR] %s", sweeperErr)
sweeperErrs = multierror.Append(sweeperErrs, sweeperErr)
continue
}

if output != nil && output.OperationId != nil {
if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil {
sweeperErr := fmt.Errorf("error waiting for Service Discovery HTTP Namespace (%s) deletion: %w", id, err)
log.Printf("[ERROR] %s", sweeperErr)
sweeperErrs = multierror.Append(sweeperErrs, sweeperErr)
continue
}
}
Comment on lines +56 to +77
Copy link
Contributor

Choose a reason for hiding this comment

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

The deletion logic is repeated here and in the resource itself. Would it make sense to have it in one place and call it for the resource and the sweeper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most likely, yes. Ideally we can setup the sweepers to call the existing resource deletion logic rather than re-inventing the wheel each sweeper. Probably worth a followup design proposal though. 👍

}

return !isLast
})

if testSweepSkipSweepError(err) {
log.Printf("[WARN] Skipping Service Discovery HTTP Namespaces sweep for %s: %s", region, err)
return sweeperErrs.ErrorOrNil() // In case we have completed some pages, but had errors
}

if err != nil {
sweeperErrs = multierror.Append(sweeperErrs, fmt.Errorf("error retrieving Service Discovery HTTP Namespaces: %w", err))
}

return sweeperErrs.ErrorOrNil()
}

func TestAccAWSServiceDiscoveryHttpNamespace_basic(t *testing.T) {
resourceName := "aws_service_discovery_http_namespace.test"
rName := fmt.Sprintf("tf-acc-test-%s", acctest.RandStringFromCharSet(8, acctest.CharSetAlpha))
Expand Down
50 changes: 30 additions & 20 deletions aws/resource_aws_service_discovery_private_dns_namespace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package aws

import (
"time"
"fmt"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/servicediscovery"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/servicediscovery/waiter"
)

func resourceAwsServiceDiscoveryPrivateDnsNamespace() *schema.Resource {
Expand Down Expand Up @@ -65,24 +66,34 @@ func resourceAwsServiceDiscoveryPrivateDnsNamespaceCreate(d *schema.ResourceData
input.Description = aws.String(v.(string))
}

resp, err := conn.CreatePrivateDnsNamespace(input)
output, err := conn.CreatePrivateDnsNamespace(input)

if err != nil {
return err
return fmt.Errorf("error creating Service Discovery Private DNS Namespace (%s): %w", name, err)
}

stateConf := &resource.StateChangeConf{
Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
Target: []string{servicediscovery.OperationStatusSuccess},
Refresh: servicediscoveryOperationRefreshStatusFunc(conn, *resp.OperationId),
Timeout: 5 * time.Minute,
if output == nil || output.OperationId == nil {
return fmt.Errorf("error creating Service Discovery Private DNS Namespace (%s): creation response missing Operation ID", name)
}

opresp, err := stateConf.WaitForState()
operationOutput, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId))

if err != nil {
return err
return fmt.Errorf("error waiting for Service Discovery Private DNS Namespace (%s) creation: %w", name, err)
}

if operationOutput == nil || operationOutput.Operation == nil {
return fmt.Errorf("error creating Service Discovery Private DNS Namespace (%s): operation response missing Operation information", name)
}

namespaceID, ok := operationOutput.Operation.Targets[servicediscovery.OperationTargetTypeNamespace]

if !ok {
return fmt.Errorf("error creating Service Discovery Private DNS Namespace (%s): operation response missing Namespace ID", name)
}

d.SetId(*opresp.(*servicediscovery.GetOperationOutput).Operation.Targets["NAMESPACE"])
d.SetId(aws.StringValue(namespaceID))

return resourceAwsServiceDiscoveryPrivateDnsNamespaceRead(d, meta)
}

Expand Down Expand Up @@ -117,18 +128,17 @@ func resourceAwsServiceDiscoveryPrivateDnsNamespaceDelete(d *schema.ResourceData
Id: aws.String(d.Id()),
}

resp, err := conn.DeleteNamespace(input)
output, err := conn.DeleteNamespace(input)

if err != nil {
return err
return fmt.Errorf("error deleting Service Discovery Private DNS Namespace (%s): %w", d.Id(), err)
}

stateConf := &resource.StateChangeConf{
Pending: []string{servicediscovery.OperationStatusSubmitted, servicediscovery.OperationStatusPending},
Target: []string{servicediscovery.OperationStatusSuccess},
Refresh: servicediscoveryOperationRefreshStatusFunc(conn, *resp.OperationId),
Timeout: 5 * time.Minute,
if output != nil && output.OperationId != nil {
if _, err := waiter.OperationSuccess(conn, aws.StringValue(output.OperationId)); err != nil {
return fmt.Errorf("error waiting for Service Discovery Private DNS Namespace (%s) deletion: %w", d.Id(), err)
}
}

_, err = stateConf.WaitForState()
return err
return nil
}
Loading