-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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.
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
New Resource: azurerm_communication_service_email_domain_association
#26432
New Resource: azurerm_communication_service_email_domain_association
#26432
Conversation
Signed-off-by: Jan-Otto Kröpke <[email protected]>
8254226
to
f50852f
Compare
…l_domain_association
…l_domain_association
@jackofallops Kindly requesting an review here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jkroepke - I've left some comments and changes below if you can take a look, I'll continue review and get tests run once they're addressed.
Thanks!
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
internal/services/communication/communication_service_email_domain_association_resource.go
Show resolved
Hide resolved
…main_association_resource.go
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
…main_association_resource.go
…main_association_resource.go
…main_association_resource_test.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource_test.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource_test.go Co-authored-by: jackofallops <[email protected]>
…main_association_resource_test.go Co-authored-by: jackofallops <[email protected]>
internal/services/communication/communication_service_email_domain_association_resource_test.go
Outdated
Show resolved
Hide resolved
…main_association_resource_test.go
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
I apply all suggestions, tests are green on my system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jkroepke - Thanks for the quick turnaround on those changes, just a couple more that I think I missed last pass. Once they're done I can get the tests run on the CI and we should be good to merge.
Thanks again!
internal/services/communication/communication_service_email_domain_association_resource.go
Outdated
Show resolved
Hide resolved
var model EmailDomainAssociationResourceModel | ||
|
||
if err := metadata.Decode(&model); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can only be sure of having the resource ID at this point, so we should not need to do this, and work exclusively with the ID (and the data it contains) for delete operations. It's also unused in the rest of the function here too.
var model EmailDomainAssociationResourceModel | |
if err := metadata.Decode(&model); err != nil { | |
return err | |
} |
…main_association_resource.go Co-authored-by: jackofallops <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @jkroepke - This LGTM now 👍
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
Follow-Up from: #26179 (comment)
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_communication_service_email_domain_association
[GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #22995
Note
If this PR changes meaningfully during the course of review please update the title and description as required.