New resource: aws_directory_service_conditional_forwarder #4071
Conversation
spirius
changed the title
There was a problem hiding this comment.
Awesome work, @spirius! This PR looked pretty well implemented and I only had a few nitpicks which I will take care of on merge. Thanks so much! 🚀
1 test passed (all tests)
=== RUN TestACCAwsDirectoryServiceConditionForwarder_basic
--- PASS: TestACCAwsDirectoryServiceConditionForwarder_basic (1831.84s)
| }, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "directory_id": &schema.Schema{ |
There was a problem hiding this comment.
Nitpick: The &schema.Schema declarations here are extra as of Go 1.7 since its declared above in map[string]*schema.Schema
| }, | ||
| }, | ||
|
|
||
| "domain_name": &schema.Schema{ |
There was a problem hiding this comment.
To match the API, should this attribute be remote_domain_name?
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: validation.StringMatch(regexp.MustCompile("^([a-zA-Z0-9]+[\\.-])+([a-zA-Z0-9])+[.]?$"), "'domain_name' is incorrect"), |
There was a problem hiding this comment.
Excellent usage of validation.StringMatch()! Should we maybe return a more helpful error message like invalid value, see the RemoteDomainName attribute documentation: https://docs.aws.amazon.com/directoryservice/latest/devguide/API_ConditionalForwarder.html?
| return err | ||
| } | ||
|
|
||
| d.SetId("") |
There was a problem hiding this comment.
Minor nitpick: we no longer require d.SetId("") on deletion 👍 #4191
|
|
||
| if err != nil { | ||
| if isAWSErr(err, directoryservice.ErrCodeEntityDoesNotExistException, "") { | ||
| d.SetId("") |
There was a problem hiding this comment.
Minor nitpick: we should log here and below when we're removing a resource 👍 log.Printf("[WARN] Directory Service Conditional Forwarder (%s) not found, removing from state", d.Id())
|
|
||
| d.Set("dns_ips", flattenStringList(cfd.DnsIpAddrs)) | ||
| d.Set("directory_id", directoryId) | ||
| d.Set("domain_name", *cfd.RemoteDomainName) |
There was a problem hiding this comment.
Nitpick: To prevent a panic in the unlikely scenario of AWS not returning this value, we should either remove the * dereference (d.Set() can automatically handle nil values) or wrap it in aws.StringValue(cfd.RemoteDomainName)
| return nil | ||
| } | ||
|
|
||
| func parseDSConditionalForwarderId(id string) (directoryId, domainName string) { |
There was a problem hiding this comment.
Maintainability note: We generally try to directly return an error as the last output from these so its obvious it needs to be handled by implementing functions (rather than expecting everywhere to know to check for "" directoryId and domainName) 👍
| "github.com/hashicorp/terraform/terraform" | ||
| ) | ||
|
|
||
| func TestACCAwsDirectoryServiceConditionForwarder_basic(t *testing.T) { |
There was a problem hiding this comment.
This function should be named TestAccAWS (lowercase cc being the most important)
|
This has been released in version 1.15.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
Test result