New Resource: `azurerm_email_communication_service_domain` #26179

jkroepke · 2024-06-02T08:21:32Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Implements a Terraform Provider for https://learn.microsoft.com/en-us/rest/api/communication/resourcemanager/domains?view=rest-communication-resourcemanager-2023-03-31

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

=== RUN   TestAccEmailServiceDomain_basic
=== PAUSE TestAccEmailServiceDomain_basic
=== CONT  TestAccEmailServiceDomain_basic
=== RUN   TestAccEmailServiceDomain_requiresImport
=== PAUSE TestAccEmailServiceDomain_requiresImport
=== CONT  TestAccEmailServiceDomain_requiresImport
=== RUN   TestAccEmailServiceDomain_update
=== PAUSE TestAccEmailServiceDomain_update
=== CONT  TestAccEmailServiceDomain_update
--- PASS: TestAccEmailServiceDomain_requiresImport (394.07s)
--- PASS: TestAccEmailServiceDomain_basic (398.19s)
--- PASS: TestAccEmailServiceDomain_update (499.17s)
PASS

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

New Resource: azurerm_email_communication_service_domain [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #22995

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

jackofallops

Hi @jkroepke - Thanks for this PR, it's off to a good start. Can you can take a look at the comments and requested changes below?

Thanks

internal/services/communication/communication_service_resource.go

jackofallops · 2024-06-03T11:58:28Z

internal/services/communication/communication_service_resource.go

@@ -146,7 +147,8 @@ func (r CommunicationServiceResource) Create() sdk.ResourceFunc {
 				// The location is always `global` from the Azure Portal
 				Location: location.Normalize("global"),
 				Properties: &communicationservices.CommunicationServiceProperties{
-					DataLocation: model.DataLocation,
+					DataLocation:  model.DataLocation,
+					LinkedDomains: pointer.To(model.LinkedDomains),


Should this be nil on create if there are no LinkedDomains or an empty list? Azure API's typically expect any unspecified value to be nil.

LinkedDomains is an optional value, so nil should be used. if the user does not provide LinkedDomains, model. LinkedDomains should be a nil value, since its an slice? Pretty much the same behavior as for tags here.

the pointer.To() function won't return nil there, it will return an empty slice. If the value should be nil for an empty slice, then that will need to be conditionally set outside the construction of the CommunicationServiceProperties if model.LinkedDomains is not empty. If an empty slice there is treated the same way as nil by the API, then this is fine as-is.

Tags have a slightly different behaviour, and are a map[string]string (or similar, depending on service implementation), so are handled differently.

if the pointer.To() function won't return nil there, could you please explain why the debugger shows me a nil value here? I'm running the TestAccCommunicationService_basic test here:

I as know, pointer.To() just creates a pointer of an variable. If the end-user omits linked_domains, the model.LinkedDomains value is nil.

Could you please explain, why we need an additional condition here? I just want to get the reason behind.

I added a test case for any empty list anyways which is passing.

internal/services/communication/communication_service_resource.go

internal/services/communication/email_service_domain_resource.go

internal/services/communication/email_service_domain_resource_test.go

Co-authored-by: jackofallops <[email protected]>

…test.go Co-authored-by: jackofallops <[email protected]>

Co-authored-by: jackofallops <[email protected]>

jkroepke · 2024-06-03T19:23:55Z

Thanks @jackofallops for taking a look here, please take another look.

jkroepke · 2024-06-04T06:57:36Z

I need also some assistance for the following case:

After the recent changes, running TestAccCommunicationService_update

Returns me that error:

        deleting Domain (Subscription: "..."
        Resource Group Name: "acctestRG-communicationservice-240604084022507093"
        Email Service Name: "acctest-CommunicationService-240604084022507093"
        Domain Name: "AzureManagedDomain"): performing Delete: unexpected status 400
        (400 Bad Request) with error: PreconditionFailed: Unlink from
        CommunicationServices resources:
        acctest-communicationservice-240604084022507093 before deleting this Domains
        resource.

Terraform tries to delete the domain, before updating azurerm_communication_service to unlink the domain.

How I can adjust the behavior of Terraform that azurerm_communication_service get updated first, before deleting a domain?

jackofallops · 2024-06-05T14:26:46Z

I need also some assistance for the following case:

After the recent changes, running TestAccCommunicationService_update

Returns me that error:
        deleting Domain (Subscription: "..."
        Resource Group Name: "acctestRG-communicationservice-240604084022507093"
        Email Service Name: "acctest-CommunicationService-240604084022507093"
        Domain Name: "AzureManagedDomain"): performing Delete: unexpected status 400
        (400 Bad Request) with error: PreconditionFailed: Unlink from
        CommunicationServices resources:
        acctest-communicationservice-240604084022507093 before deleting this Domains
        resource.
Terraform tries to delete the domain, before updating azurerm_communication_service to unlink the domain.

How I can adjust the behavior of Terraform that azurerm_communication_service get updated first, before deleting a domain?

I'm afraid this is a limitation, resources are deleted before others are updated. In these cases it's necessary to create an additional "association" resource to establish the necessary dependency link to allow the appropriate sequence of actions to take place. An example of this is azurerm_application_load_balancer_subnet_association. We'd need an additional resource here to provide this functionality, i.e. update the azurerm_communication_service with one (or more?) domains.

jkroepke · 2024-06-05T14:36:09Z

In these cases it's necessary to create an additional "association" resource

Okay, I understand. Is it fine to you that:

remove the linked_domain property from communication_service and
setup an additional resource in a separate PR

katbyte

LGTM ⛰️

<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>changes detected:
	"hashicorp/azurerm" updated from "3.108.0" to "3.109.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.109.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.109.0
FEATURES:

* **New Data Source:** `azurerm_automation_runbook` ([#26359](hashicorp/terraform-provider-azurerm#26359 **New Resource:** `azurerm_data_protection_backup_instance_postgresql_flexible_server` ([#26249](hashicorp/terraform-provider-azurerm#26249 **New Resource:** `azurerm_email_communication_service_domain` ([#26179](hashicorp/terraform-provider-azurerm#26179 **New Resource:** `azurerm_system_center_virtual_machine_manager_cloud` ([#25429](hashicorp/terraform-provider-azurerm#25429 **New Resource:** `azurerm_system_center_virtual_machine_manager_virtual_machine_template` ([#25449](hashicorp/terraform-provider-azurerm#25449 **New Resource:** `azurerm_system_center_virtual_machine_manager_virtual_network` ([#25451](https://github.com/hashicorp/terraform-provider-azurerm/issues/25451))

ENHANCEMENTS:

* Data Source: `azurerm_hdinsight_cluster` - export the `cluster_id` attribute ([#26228](hashicorp/terraform-provider-azurerm#26228 `azurerm_cosmosdb_sql_container` - support for the `partition_key_kind` and `partition_key_paths` properties ([#26372](hashicorp/terraform-provider-azurerm#26372 `azurerm_data_protection_backup_instance_blob_storage` - support for the `storage_account_container_names` property ([#26232](hashicorp/terraform-provider-azurerm#26232 `azurerm_virtual_network_peering` - support for the `peer_complete_virtual_networks_enabled`, `only_ipv6_peering_enabled`, `local_subnet_names`, and `remote_subnet_names` properties ([#26229](hashicorp/terraform-provider-azurerm#26229 `azurerm_virtual_desktop_host_pool` - changing the `preferred_app_group_type` property no longer creates a new resource ([#26333](hashicorp/terraform-provider-azurerm#26333 `azurerm_maps_account` - support for the `location`, `identity`, `cors` and `data_store` properties ([#26397](https://github.com/hashicorp/terraform-provider-azurerm/issues/26397))

BUG FIXES:

* `azurerm_automation_job_schedule` - updates `azurerm_automation_job_schedule` to use a composite resource id and allows `azurerm_automation_runbook` to be updated without causing `azurerm_automation_job_schedule` to recreate ([#22164](hashicorp/terraform-provider-azurerm#22164 `azurerm_databricks_workspace`- correctly allow disabling the default firewall ([#26339](hashicorp/terraform-provider-azurerm#26339 `azurerm_virtual_hub_*` - spliting create and update so lifecycle ignore changes works correctly ([#26310](https://github.com/hashicorp/terraform-provider-azurerm/issues/26310))

DEPRECATIONS:

* Data Source: `azurerm_mariadb_server` - deprecated since the service is retiring. Please use `azurerm_mysql_flexible_server` instead ([#26354](hashicorp/terraform-provider-azurerm#26354 `azurerm_mariadb_configuration` - deprecated since the service is retiring. Please use `azurerm_mysql_flexible_server_configuration` instead ([#26354](hashicorp/terraform-provider-azurerm#26354 `azurerm_mariadb_database` - deprecated since the service is retiring. Please use `azurerm_mysql_flexible_database` instead ([#26354](hashicorp/terraform-provider-azurerm#26354 `azurerm_mariadb_firewall_rule` - deprecated since the service is retiring. Please use `azurerm_mysql_flexible_server_firewall_rule` instead ([#26354](hashicorp/terraform-provider-azurerm#26354 `azurerm_mariadb_server` - deprecated since the service is retiring. Please use `azurerm_mysql_flexible_server` instead ([#26354](hashicorp/terraform-provider-azurerm#26354 `azurerm_mariadb_virtual_network_rule` - deprecated since the service is retiring ([#26354](https://github.com/hashicorp/terraform-provider-azurerm/issues/26354))


</pre> </details> </details> <a href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/256/">Jenkins pipeline link</a> </action> </Actions> --- <table> <tr> <td width="77"> <img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli logo" width="50" height="50"> </td> <td> <p> Created automatically by <a href="https://www.updatecli.io/">Updatecli</a> </p> <details><summary>Options:</summary> <br /> <p>Most of Updatecli configuration is done via <a href="https://www.updatecli.io/docs/prologue/quick-start/">its manifest(s)</a>.</p> <ul> <li>If you close this pull request, Updatecli will automatically reopen it, the next time it runs.</li> <li>If you close this pull request and delete the base branch, Updatecli will automatically recreate it, erasing all previous commits made.</li> </ul> <p> Feel free to report any issues at <a href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br /> If you find this tool useful, do not hesitate to star <a href="https://github.com/updatecli/updatecli/stargazers">our GitHub repository</a> as a sign of appreciation, and/or to tell us directly on our <a href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>! </p> </details> </td> </tr> </table> Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>

afh92t23i3 · 2024-06-21T09:33:03Z

In these cases it's necessary to create an additional "association" resource

Okay, I understand. Is it fine to you that:

remove the linked_domain property from communication_service and

setup an additional resource in a separate PR

Is this second bullet point "setup an additional resource in a separate PR" already implemented or in implementation? Can you please link this PR here? I would be highly interested in this feature :)

jkroepke · 2024-06-21T11:45:57Z

Once, I have open the PR, I can link it.

github-actions · 2024-07-22T02:12:50Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added documentation dependencies service/communication size/XL waiting-response labels Jun 2, 2024

jkroepke force-pushed the communication_mail branch from 2d7880d to 1826856 Compare June 2, 2024 08:30

github-actions bot removed the waiting-response label Jun 2, 2024

New Resource: azurerm_email_communication_service_domain

9f9a387

jkroepke force-pushed the communication_mail branch from 1826856 to 9f9a387 Compare June 2, 2024 08:33

github-actions bot added waiting-response and removed waiting-response labels Jun 2, 2024

jackofallops requested changes Jun 3, 2024

View reviewed changes

github-actions bot added the waiting-response label Jun 3, 2024

Update internal/services/communication/client/client.go

0e99e0e

Co-authored-by: jackofallops <[email protected]>

github-actions bot removed the waiting-response label Jun 3, 2024

jkroepke and others added 7 commits June 3, 2024 19:51

Update internal/services/communication/helper/domain_record.go

e0e3f90

Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/email_service_domain_resource_…

e0c0b0f

…test.go Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/email_service_domain_resource_…

7352ac0

…test.go Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/email_service_domain_resource_…

ede4545

…test.go Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/email_service_domain_resource.go

76a50dd

Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/email_service_domain_resource.go

ef01457

Co-authored-by: jackofallops <[email protected]>

Update internal/services/communication/helper/domain_record.go

ae6e9b8

Co-authored-by: jackofallops <[email protected]>

jkroepke requested a review from jackofallops June 3, 2024 19:23

New Resource: azurerm_email_communication_service_domain

9ca3017

jkroepke force-pushed the communication_mail branch from fc64ce8 to 9ca3017 Compare June 3, 2024 19:26

github-actions bot added the waiting-response label Jun 3, 2024

make terrafmt

dcba883

github-actions bot added waiting-response and removed waiting-response labels Jun 3, 2024

github-actions bot added waiting-response and removed waiting-response labels Jun 4, 2024

added empty list test

118ccc1

jkroepke force-pushed the communication_mail branch from 3a995da to 118ccc1 Compare June 4, 2024 10:42

github-actions bot removed the waiting-response label Jun 4, 2024

jkroepke force-pushed the communication_mail branch from 6bb60b5 to cb377fb Compare June 5, 2024 14:41

remove linked_domain property

4984856

jkroepke force-pushed the communication_mail branch from cb377fb to 4984856 Compare June 5, 2024 14:42

Merge branch 'hashicorp:main' into communication_mail

1f9e8ab

katbyte approved these changes Jun 21, 2024

View reviewed changes

katbyte merged commit 3c25ede into hashicorp:main Jun 21, 2024
34 checks passed

katbyte added a commit that referenced this pull request Jun 21, 2024

Update CHANGELOG.md #26179

1eee0af

github-actions bot added this to the v3.109.0 milestone Jun 21, 2024

jkroepke deleted the communication_mail branch June 21, 2024 11:44

jkroepke mentioned this pull request Jun 22, 2024

New Resource: azurerm_communication_service_email_domain_association #26432

Merged

14 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_email_communication_service_domain` #26179

New Resource: `azurerm_email_communication_service_domain` #26179

jkroepke commented Jun 2, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Jun 3, 2024

jkroepke Jun 3, 2024

jackofallops Jun 4, 2024

jkroepke Jun 4, 2024

jkroepke Jun 4, 2024

jkroepke commented Jun 3, 2024

jkroepke commented Jun 4, 2024

jackofallops commented Jun 5, 2024

jkroepke commented Jun 5, 2024 •

edited

Loading

katbyte left a comment

afh92t23i3 commented Jun 21, 2024

jkroepke commented Jun 21, 2024

github-actions bot commented Jul 22, 2024

New Resource: azurerm_email_communication_service_domain #26179

New Resource: azurerm_email_communication_service_domain #26179

Conversation

jkroepke commented Jun 2, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Jun 3, 2024

Choose a reason for hiding this comment

jkroepke Jun 3, 2024

Choose a reason for hiding this comment

jackofallops Jun 4, 2024

Choose a reason for hiding this comment

jkroepke Jun 4, 2024

Choose a reason for hiding this comment

jkroepke Jun 4, 2024

Choose a reason for hiding this comment

jkroepke commented Jun 3, 2024

jkroepke commented Jun 4, 2024

jackofallops commented Jun 5, 2024

jkroepke commented Jun 5, 2024 • edited Loading

katbyte left a comment

Choose a reason for hiding this comment

afh92t23i3 commented Jun 21, 2024

jkroepke commented Jun 21, 2024

github-actions bot commented Jul 22, 2024

New Resource: `azurerm_email_communication_service_domain` #26179

New Resource: `azurerm_email_communication_service_domain` #26179

jkroepke commented Jun 2, 2024 •

edited

Loading

jkroepke commented Jun 5, 2024 •

edited

Loading