New resource azurerm_virtual_desktop_scaling_plan_host_pool_association

[alexwilcox9]

PR for the creation of a new resource azurerm_virtual_desktop_scaling_plan_host_pool_association

Why

In my AVD deployments I have a normal worktime scaling plan and a holiday period scaling plan. I'd like to have a variable to easily switch from one SP to another. Using the host_pool block within the current azurerm_virtual_desktop_scaling_plan you would need to do something with dynamic blocks and there doesn't seem to be a clean solution.
Having a dedicated resource would make this much easier.

It would also be more inline with the existing resource azurerm_virtual_desktop_workspace_application_group_association (which this PR is mostly copied from)

Help

When using this resource alongside azurerm_virtual_desktop_scaling_plan there will be conflicts where an association has been created in the new resource but not declared in a host_pool block. Is there a way to prevent the host_pool block being checked if it hasn't been declared in the users configuration? Rather than the user having to add an ignore_changes to their code.

Documentation and Tests

If you're happy with this resource in principle I'll get the docs and tests written up
Thanks in advance!

[mbfrahry]

Hey @alexwilcox9, this PR is looking on the right track but we'll need some tests to confirm it's functionality.

Your ask about what we could be done about host_pool on the parent resource showing a diff could be solved by adding Computed: true to host_pool.

I am wondering if there is a way we could solve this with a conditionals though. Could you open an issue describing your issue and what you're trying to do in a little more detail?

[alexwilcox9]

Sorry for the slow response, only just got time to come back around to this.

So to describe what I'm trying to do in more detail; I'm looking to make it simpler to switch which scaling plan is currently assigned to a hostpool.

Currently you could probably use a dynamic block to only create the host_pool block when a conditional is true but having a separate resource for association is more consistent with the rest of the AVD resources and makes cleaner code

Here's an example of how I think it would be done with conditionals
https://gist.github.com/alexwilcox9/033de7fe2ad46236be0d6ee9062a8462

Whereas the same could be achieved with the new resource as follows

resource "azurerm_virtual_desktop_scaling_plan_host_pool_association" "example" {
  host_pool_id    = azurerm_virtual_desktop_host_pool.example.id
  scaling_plan_id = var.holiday ? azurerm_virtual_desktop_scaling_plan.holiday.id : azurerm_virtual_desktop_scaling_plan.example.id
  enabled         = true
}

Another problem I find with the conditionals is that Terraform tries to update both scaling plans at the same time causing an error. Meaning I always need to run the apply twice

azurerm_virtual_desktop_scaling_plan.holiday: Modifying... [id=/subscriptions/<SUB ID>/resourceGroups/example-rg/providers/Microsoft.DesktopVirtualization/scalingPlans/holiday-scalingplan]
azurerm_virtual_desktop_scaling_plan.example: Modifying... [id=/subscriptions/<SUB ID>/resourceGroups/example-rg/providers/Microsoft.DesktopVirtualization/scalingPlans/regular-scalingplan]
azurerm_virtual_desktop_scaling_plan.example: Modifications complete after 3s [id=/subscriptions/<SUB ID>/resourceGroups/example-rg/providers/Microsoft.DesktopVirtualization/scalingPlans/regular-scalingplan]
╷
│ Error: updating Scaling Plan (Subscription: "<SUB ID>"
│ Resource Group Name: "example-rg"
│ Scaling Plan Name: "holiday-scalingplan"): unexpected status 400 with error: 400: ActivityId: e4a56646-173e-4820-88a2-8a85aa4beee9 Error: ≤{"error":{"code":"BadRequest","message":"Host pools already assigned to another scaling plan: '/subscriptions/<SUB ID>/resourceGroups/example-rg/providers/Microsoft.DesktopVirtualization/hostPools/example-hostpool'","target":"/api/scalingPlans/subscriptions/<SUB ID>/resourceGroups/example-rg/scalingPlans/holiday-scalingplan"}}≥
│ 
│   with azurerm_virtual_desktop_scaling_plan.holiday,
│   on main.tf line 98, in resource "azurerm_virtual_desktop_scaling_plan" "holiday":
│   98: resource "azurerm_virtual_desktop_scaling_plan" "holiday" {
│ 
╵

I hope that helps explain why I think this new resource would be beneficial

[mbfrahry]

Hey @alexwilcox9, I can see the use for this resource after you walked me through how awful it is to achieve what you're trying to do.

I've left another review as I do have concerns that bringing this resource in will cause breaking changes as you've changed existing tests.

[mbfrahry]

We'll want a unit test around this similar to https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/bot/parse/bot_channel_test.go#L24

[alexwilcox9]

Just wanted to clarify on this front - this ID is made up of two existing resource IDs that already have tests written so should my tests duplicate those and build up both resource IDs from scratch or just test the parsing of the two combined

For example testing the following
/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/example-resource-group/providers/Microsoft.DesktopVirtualization/scalingPlans/scalingPlanValue Invalid
/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/example-resource-group/providers/Microsoft.DesktopVirtualization/scalingPlans/scalingPlanValue|/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/example-resource-group/providers/Microsoft.DesktopVirtualization/hostPools/hostPoolValue Valid
/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/example-resource-group/providers/Microsoft.DesktopVirtualization/scalingPlans/scalingPlanValue|/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/example-resource-group/providers/Microsoft.DesktopVirtualization/hostPools/hostPoolValue|/extra Invalid

[mbfrahry]

We really shouldn't change existing tests as breaking changes could pop up because a test was changed and broke an existing test but changing the config masked the breaking change

[alexwilcox9]

I saw an opportunity to tidy up the tests, when Scaling Plans first released they were only in a handful of regions and didn't have a built in role so creating a custom one was required.
Now there's a built in role and more regions are supported so wanted to update the tests to reflect that

[github-actions]

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

[alexwilcox9]

Hey, thanks for the review and apologies for the delay in getting back to it. I've replied to your comments and made some changes

[alexwilcox9]

I also found that during any create/update/delete operation the scaling plans schedule would disappear.
I'm not sure why this happens as other attributes like description and time zone that I don't touch are left alone. In any case, I've added the existing schedules to the patch properties and that seems to solve it

[katbyte]

Thanks for the changes @alexwilcox9 - Looks like this is missing documentation for the new resource. once that is added i think this can be reviewed one final time and merged

[katbyte]

Thanks @alexwilcox9 ! LGTM 🍖

[github-actions]

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.