Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 9 commits into from
Jun 9, 2017
Merged

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

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8f06bc3
Data Source support for Resource Group
whiskeyjay May 28, 2017
3c6e109
Better message for mismatching locations.
whiskeyjay May 30, 2017
cf72550
Reuse existing read code
whiskeyjay May 30, 2017
cdf98a0
Adds documentation
whiskeyjay Jun 2, 2017
e1577d1
Adds test
whiskeyjay Jun 2, 2017
c924379
Merge upstream changes.
whiskeyjay Jun 2, 2017
b0b9adb
Adds a function for composing ID strings
whiskeyjay Jun 5, 2017
09e1d83
Merge from upstream master
whiskeyjay Jun 7, 2017
dbe93e4
Change location to computed.
whiskeyjay Jun 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Reuse existing read code
  • Loading branch information
@whiskeyjay
whiskeyjay committed May 30, 2017
commit cf7255033db905ccc22283c6431bf40df1d30bee
22 changes: 10 additions & 12 deletions builtin/providers/azurerm/data_source_arm_resource_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package azurerm
import (
"fmt"

"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/helper/schema"
)

Expand All @@ -25,27 +24,26 @@ func dataSourceArmResourceGroup() *schema.Resource {

func dataSourceArmResourceGroupRead(d *schema.ResourceData, meta interface{}) error {
armClient := meta.(*ArmClient)
resourceGroupClient := armClient.resourceGroupClient

resourceGroupName := d.Get("name").(string)
result, err := resourceGroupClient.Get(resourceGroupName)
if err != nil {
return errwrap.Wrapf("Error reading Resource Group {{err}}", err)
location, getLocationOk := d.GetOk("location")
resourceId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", armClient.subscriptionId, resourceGroupName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a function and unit tests.


d.SetId(resourceId)

if err := resourceArmResourceGroupRead(d, meta); err != nil {
return err
}

if v, ok := d.GetOk("location"); ok {
location := azureRMNormalizeLocation(v.(string))
actualLocation := azureRMNormalizeLocation(*result.Location)
if getLocationOk {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what we are doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

actualLocation := azureRMNormalizeLocation(d.Get("location").(string))
location := azureRMNormalizeLocation(location)

if location != actualLocation {
return fmt.Errorf(`The location specified in Data Source (%s) doesn't match the actual location of the Resource Group "%s (%s)"`,
location, resourceGroupName, actualLocation)
}
}

d.Set("location", *result.Location)
flattenAndSetTags(d, result.Tags)
d.SetId(*result.ID)

return nil
}