New Resource: aws_swf_domain #2803
Conversation
jen20
added
enhancement
radeksimko
added
service/route53
|
Any chance this PR will be reviewed soon? I'd love to get this in. Thanks! |
|
Can we get this get merged? |
There was a problem hiding this comment.
Hi @robinjoseph08 👋 Thanks for submitting this and sorry for the delay in review 😅 Please see the below for some initial feedback. Let us know if you have any questions or do not have time to implement the feedback. Thanks!
| Create: resourceAwsSwfDomainCreate, | ||
| Read: resourceAwsSwfDomainRead, | ||
| Delete: resourceAwsSwfDomainDelete, | ||
| Importer: &schema.ResourceImporter{ |
There was a problem hiding this comment.
The acceptance testing is currently not exercising resource importing. To add it we can add the following TestStep to TestAccAwsSwfDomain_basic:
{
ResourceName: "aws_swf_domain.test",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"name_prefix"}, // this line is only necessary if the test configuration is using name_prefix
},| ForceNew: true, | ||
| ConflictsWith: []string{"name_prefix"}, | ||
| }, | ||
| "name_prefix": &schema.Schema{ |
There was a problem hiding this comment.
Minor nitpick: the &schema.Schema declaration here is unnecessary since Go 1.7 as its already defined by map[string]*schema.Schema above
The same applies to both description and workflow_execution_retention_period_in_days below.
| } else { | ||
| name = resource.UniqueId() | ||
| } | ||
| d.Set("name", name) |
There was a problem hiding this comment.
The d.Set("name", name) here should be extraneous as its handled (properly) in the read function.
|
|
||
| resp, err := conn.DescribeDomain(input) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should perform a check here to allow Terraform to properly suggest recreating the resource if it is deleted outside Terraform as well as return a more helpful message about when the error occurred:
if err != nil {
if isAWSErr(err, swf.ErrCodeUnknownResourceFault, "") {
log.Printf("[WARN] SWF Domain %q not found, removing from state", d.Id())
d.SetId("")
return nil
}
return fmt.Errorf("error reading SWF Domain: %s", err)
}| return err | ||
| } | ||
|
|
||
| info := resp.DomainInfo |
There was a problem hiding this comment.
To prevent potential panics, we should perform nil checks for the nested structures:
if resp.DomainInfo == nil || resp.DomainInfo.Configuration == nil {
log.Printf("[WARN] SWF Domain %q not found, removing from state", d.Id())
d.SetId("")
return nil
}|
|
||
| _, err := conn.DeprecateDomain(input) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
We should allow the resource to skip returning an error if its already been deprecated/deleted:
if err != nil {
if isAWSErr(err, swf.ErrCodeDomainDeprecatedFault, "") {
return nil
}
if isAWSErr(err, swf.ErrCodeUnknownResourceFault, "") {
return nil
}
return fmt.Errorf("error deleting SWF Domain: %s", err)
}| return err | ||
| } | ||
|
|
||
| d.SetId("") |
There was a problem hiding this comment.
Minor nitpick: calling d.SetId("") during the delete function is extraneous
| "github.com/hashicorp/terraform/terraform" | ||
| ) | ||
|
|
||
| func TestAccAwsSwfDomain_basic(t *testing.T) { |
There was a problem hiding this comment.
Since this resource is accepting three ways to configure the name (via name, via name_prefix, and via generated ID), there should be three separate acceptance tests to cover each scenario separately.
bflad
added
the
waiting-response
|
Hey @bflad, I don't think I'll be able to get to this soon unfortunately. But anyone is free to pick it up! |
make testacc TEST=./aws TESTARGS='-run=TestAccAWSSwfDomain' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSSwfDomain -timeout 120m === RUN TestAccAWSSwfDomain_basic --- PASS: TestAccAWSSwfDomain_basic (13.01s) === RUN TestAccAWSSwfDomain_NamePrefix --- PASS: TestAccAWSSwfDomain_NamePrefix (11.40s) === RUN TestAccAWSSwfDomain_GeneratedName --- PASS: TestAccAWSSwfDomain_GeneratedName (12.78s) === RUN TestAccAWSSwfDomain_Description --- PASS: TestAccAWSSwfDomain_Description (11.61s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 48.829s
|
Thanks @robinjoseph08! I have addressed PR feedback in ec25406 and it passes acceptance testing: |
|
This has been released in version 1.27.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
breathingdust
removed
the
waiting-response
#131