Add Proximity Placement Group Resources + Support

[tobydoescode]

This pull request adds support for Proximity Placement Groups (PPG).

It adds the following:

• azurerm_proximity_placement_group (resource)
• azurerm_proximity_placement_group (data source)

Additionally it adds the proximity placement group proxy to the following supported resources:

• azurerm_availability_set
• azurerm_virtual_machine
• azurerm_virtual_machine_scale_set

(fixes #4127)

[WodansSon]

@evoio Thank you for this PR, LGTM! 🚀

[WodansSon]

@evoio If you can fix up the Travis failures and resolve the conflicts I think we will be good to go.

[katbyte]

Hi @evoio,

Thank you for the PR. In addition to the comments i've left inline i have some questions about the casing of the ID returned. Is it that the resource group name its self is returned in lower case or the resourceName part of the path resourcename ?

And is it that the id we get from the PPG api incorrect, or the value returned from the VM/VMSS api incorrect?

Also, has a bug report been files/opened anywhere? if not could we open on on the azure for go sdk and link all fixes for it with an additional comment:
this can be removed when http://link.to.issue has been fixed

[tobydoescode]

Hi @katbyte,

wrt the ID it is the VM/VMSS API that is incorrect. This however is not limited to the newer PPG property. I re-used a lot of the code for adding availability sets to add PPG and I noticed that it was setting the availability set ID to lower case. I initially removed this because I thought it was odd but as it happens the availability set ID has the very same issue. We couldn't get them to fix availability sets now because it will break a lot of people's state files.

I'll review all of the suggestions tomorrow and fix the conflicts too - a lot of which I think came out of go fmt. When I ran go fmt I got a lot of changes to other files that I did not commit because I didn't want to bloat my pull request with files I had no interest in changing.

[tobydoescode]

Contrary to my previous comment I have made most of the requested changes already and fixed the conflict. There are a couple queries which I've commented back on and I need to add the requested test.

[tobydoescode]

@katbyte I've now addressed all of the points you brought up - though I have follow up comments on two of them.

[WodansSon]

@evoio hey can you fix the conflicts on the PR and I'll take another look. Thanks! 😄

[tobydoescode]

Hi @jeffreyCline I was going to leave the conflict until someone said the rest was okay - based on @katbyte 's review. Its going to keep getting conflicts because of go fmt changing indentation.

[tobydoescode]

@jeffreyCline conflict resolved, saw that structure of client.go has changed again and it wasn't formatting.

[srakesh28]

Could we also consider support for PPG with zonal resource ?

azurerm_availability_zones

Thank you
Rakesh Shah

[tobydoescode]

Hi @srakesh28, this PR is specifically targeting PPG which are another level of control over VM placement. They are related but separate functionality - you cannot specify an AZ when creating a PPG. I haven't tried, but you may be able to specify both an AZ and PPG when creating a VM to get all your VM in close proximity in the specified AZ. If Azure allows this it should be possible on acceptance of this branch - just use the "zones" and "proximity_placement_group_id" fields together. Though it's a bit of an edge case why you want this anyway.

I haven't personally used AZs in Azure yet but from reading up on them I'm not certain what an "azurerm_availability_zones" resource/data would do. AZ are not something you can create so they are not suited to a resource. You would want the equivalent data object in AWS because there are varying numbers of AZ per region but with Azure as far as I can see A) there are either 3 or not supported in a region and B) the API doesn't allow you to query them as far as I can see.

[tobydoescode]

Is anyone able to review this? As far as I'm aware there haven't been any updates required for some time.

[curd0]

Is there any estimate when this is going to be merged?

[katbyte]

I hope you dont' mind @evoio but i've pushes some changes to get this merged. Thanks again for the PR!

[katbyte]

Could we add tags.SchemaForDataSource to the resource so this has an effect?

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccDataSourceProximityPlacementGroup_basic
=== PAUSE TestAccDataSourceProximityPlacementGroup_basic
=== CONT  TestAccDataSourceProximityPlacementGroup_basic

------- Stderr: -------
panic: Invalid address to set: []string{"tags"}

[tobydoescode]

@katbyte absolutely no issues here. Extremely well timed, our customer has finally decided to move forward using PPG.

[ghost]

This has been released in version 1.34.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.34.0"
}
# ... other configuration ...

[ghost]

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!