[MS] provider/azurerm: Data Source for Azure Resource Group

[whiskeyjay]

This is a simple data source that allows users to reference existing Azure resource groups by name.

Test output:

$ make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccDataSourceAzureRMResourceGroup_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/02 10:04:05 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccDataSourceAzureRMResourceGroup_basic -timeout 120m
=== RUN   TestAccDataSourceAzureRMResourceGroup_basic
--- PASS: TestAccDataSourceAzureRMResourceGroup_basic (43.61s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    43.619s

[stack72]

Hi @whiskeyjay

I left 2 points inline - would be good to get your thoughts on these

Please can you also add the test result output for the test to this?

Thanks

Paul

[stack72]

Can we create a helper func for this? It can look something like:

func buildResourceId(meta interface{}, resourceType string, resourceName string) string {

}

[whiskeyjay]

Added a function and unit tests.

[stack72]

I don't see what we are doing here?

[whiskeyjay]

Trying to compare the location value specified in the TF config (if any) with the actual location of the resource group.

[stack72]

So I think we should remove the ability to set the location in the dataSource - we should only need the resource group name and then Azure will give us the correct region - that way we don't need to worry about comparing the value

Thoughts?

[whiskeyjay]

In some cases users might care about using a specific region. Being able to specify the location provides a way to make sure the retrieved resource group matches the desired location or stop by an error.

[stack72]

@whiskeyjay so the issue is, all we are doing is using this as a check - we are not passing this to the API. If a user puts the wrong region in the location field then Terraform may fail even though they get the correct resource group

If you believe this is a valid usecase, please can you add 2 more acceptance tests to cover this:

1. To show that a user has matched the correct location and resource group
2. To show that when a user enters the wrong location that Terraform will throw an error - look at ExpectError in testing framework

Thanks

Paul

[whiskeyjay]

@stack72 thanks for the advice! Regarding "location", if I chose to use the "list resource" API to implement this data source, then location can be passed to Azure as part of the filter criteria. In that case, the "location" parameter might feel more natural when looking at the code. But since the "list" call will be a two step operation - first get back the resource ID if there is a match, then parse the ID and call get resource group to retrieve details, it makes it a bit unnecessarily complex for such a simple resource. That's the reason for the current implementation.

So I'm adding two more test cases as you suggested.

For test 2, however, I ran into a problem - if my test configuration contains the resource and data source with mismatching locations, the ExpectError works but test still fails with "Error destroying resource! WARNING: Dangling resources may exist." - it seems upon destroying the resource group, the data source throws the error again and prevents the process to complete.

I also tried to split the resource group creation and data source error verification into two test steps, that makes the test to pass, however the resource group created in test step 1 won't be deleted. The debug trace says destroy tests won't be executed because state is nil.

What am I missing here?

Thanks!

[whiskeyjay]

@stack72 Never mind, after some more discussion with @StephenWeatherford and @rcarun, we decided to make "location" computed only. So I have removed some code, re-run the acc test.

$ make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccDataSourceAzureRMResourceGroup_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/07 16:39:28 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccDataSourceAzureRMResourceGroup_basic -timeout 120m
=== RUN   TestAccDataSourceAzureRMResourceGroup_basic
--- PASS: TestAccDataSourceAzureRMResourceGroup_basic (65.61s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    65.628s

[whiskeyjay]

Added a function for composing resource ID strings.
Test result for the data source itself is pasted in the original PR message.
Thanks!

[whiskeyjay]

Added a function and unit tests.

[stack72]

This LGTM now - thanks for this @whiskeyjay

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