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.

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

New data source 'azurerm_resources' #3529

Merged
merged 11 commits into from
Oct 14, 2019

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented May 27, 2019

This PR introduces the new data source 'azurerm_resources'.

This data source can be used to get a list of resources which match specified criteria.

  • Get resources from a specific group
  • Get resources with one or more specific tags
  • Get all resources from a subscription

All attributes can be combined to fine tune the results.

The resulting list can be used in subsequent resources.

  • New Data source 'azurerm_resourcew'
  • Add tests
  • Add documentation

Example Usage

// Get Resources from a Resource Group
data "azurerm_resources" "test" {
  resource_group_name = "myResourceGroup"
}

// Get Resources with specific Tags
data "azurerm_resources" "test" {
  required_tags {
    environment = "production"
    role        = "webserver"
  }
}

Argument Reference

  • name - (Optional) The name of the Resource.

  • resource_group_name - (Optional) The name of the Resource group where the Resources are located.

  • type - (Optional) The Resource Type of the Resources you want to list (e.g. Microsoft.Network/virtualNetworks). A full list of available Resource Types can be found here.

  • required_tags - (Optional) A mapping of tags which the Resource has to have in order to be included in the result.

Attributes Reference

  • resources - One or more resource blocks as defined below.

The resource block contains:

  • name - The name of this resource.

  • id - The Resource ID of this resource.

  • type - The type of this resoource.

  • location - The location of this resource.

  • tags - The type of resource that this is, such as Microsoft.Network/virtualNetworks.

@tiwood
Copy link
Contributor Author

tiwood commented May 27, 2019

It seems tests sometimes fail because the resources I try to get with the data sources are deleted before the data source ran or the data source ran before the resources are created. Any ideas why that happens?

@tiwood
Copy link
Contributor Author

tiwood commented Oct 8, 2019

Tests pass.

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor'|grep -v 'examples') -v -run=TestAccDataSourceAzureRMResource_* -timeout 180m -count 1 -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccDataSourceAzureRMResourceGroup_basic
=== PAUSE TestAccDataSourceAzureRMResourceGroup_basic
=== RUN   TestAccDataSourceAzureRMResource_ByResourceID
=== PAUSE TestAccDataSourceAzureRMResource_ByResourceID
=== RUN   TestAccDataSourceAzureRMResource_ByName
=== PAUSE TestAccDataSourceAzureRMResource_ByName
=== RUN   TestAccDataSourceAzureRMResource_ByResourceGroup
=== PAUSE TestAccDataSourceAzureRMResource_ByResourceGroup
=== RUN   TestAccDataSourceAzureRMResource_ByResourceType
--- PASS: TestAccDataSourceAzureRMResource_ByResourceType (118.21s)
=== RUN   TestAccDataSourceAzureRMResource_FilteredByTags
=== PAUSE TestAccDataSourceAzureRMResource_FilteredByTags
=== CONT  TestAccDataSourceAzureRMResourceGroup_basic
=== CONT  TestAccDataSourceAzureRMResource_ByResourceGroup
=== CONT  TestAccDataSourceAzureRMResource_ByResourceID
=== CONT  TestAccDataSourceAzureRMResource_ByName
=== CONT  TestAccDataSourceAzureRMResource_FilteredByTags
--- PASS: TestAccDataSourceAzureRMResourceGroup_basic (86.27s)
--- PASS: TestAccDataSourceAzureRMResource_FilteredByTags (107.27s)
--- PASS: TestAccDataSourceAzureRMResource_ByResourceGroup (109.61s)
--- PASS: TestAccDataSourceAzureRMResource_ByName (112.08s)
--- PASS: TestAccDataSourceAzureRMResource_ByResourceID (119.29s)
PASS
ok

@tombuildsstuff @katbyte, can you review this PR?

@tombuildsstuff tombuildsstuff self-assigned this Oct 9, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @tiwood

Thanks for this PR - apologies for the delayed review here, we've been chatting about this internally trying to work out what's needed to get this in.

On the whole this looks pretty good, however there's a few things that need adjusting before we can merge this; in particular Terraform Core has a limit on the size of a Resource's State, as such we'll need to ensure we filter to a subset of the resources in the subscription, rather than storing the lot to ensure we don't hit this limit. That said, if we can fix those up this is otherwise looking good to me 👍

Thanks!

azurerm/data_source_resource.go Outdated Show resolved Hide resolved
website/docs/d/resource.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resource.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resource.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resource.html.markdown Outdated Show resolved Hide resolved
azurerm/data_source_resource.go Outdated Show resolved Hide resolved
azurerm/data_source_resource.go Outdated Show resolved Hide resolved
azurerm/data_source_resource.go Outdated Show resolved Hide resolved
azurerm/data_source_resource.go Outdated Show resolved Hide resolved
azurerm/provider.go Outdated Show resolved Hide resolved
@tiwood tiwood changed the title New data source 'azurerm_resource' New data source 'azurerm_resources' Oct 9, 2019
@tiwood
Copy link
Contributor Author

tiwood commented Oct 9, 2019

@tombuildsstuff, thank you for the review.
I've updated the code as requested.

@ghost ghost removed the waiting-response label Oct 9, 2019
@tiwood tiwood requested a review from tombuildsstuff October 9, 2019 15:28
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @tiwood

Thanks for pushing those changes - taking a look through I noticed a few more minor issues but this otherwise LGTM 👍

Thanks!

azurerm/data_source_resources.go Outdated Show resolved Hide resolved
azurerm/data_source_resources.go Outdated Show resolved Hide resolved
website/docs/d/resources.html.markdown Show resolved Hide resolved
website/docs/d/resources.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resources.html.markdown Outdated Show resolved Hide resolved
katbyte
katbyte previously requested changes Oct 9, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @tiwood,

However running it in our CI system it does seem like all the tests fail:

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMResources_ByName
=== PAUSE TestAccDataSourceAzureRMResources_ByName
=== CONT  TestAccDataSourceAzureRMResources_ByName
--- FAIL: TestAccDataSourceAzureRMResources_ByName (101.02s)
    testing.go:569: Step 0 error: Check failed: Check 1/1 error: data.azurerm_resources.test: Attribute 'resources.#' expected "1", got "0"
FAIL

------- Stderr: -------
2019/10/09 17:03:03 [DEBUG] Registering Data Sources for "Compute"..
2019/10/09 17:03:03 [DEBUG] Registering Resources for "Compute"..

@tiwood
Copy link
Contributor Author

tiwood commented Oct 9, 2019

Thanks for the changes @tiwood,

However running it in our CI system it does seem like all the tests fail:

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMResources_ByName
=== PAUSE TestAccDataSourceAzureRMResources_ByName
=== CONT  TestAccDataSourceAzureRMResources_ByName
--- FAIL: TestAccDataSourceAzureRMResources_ByName (101.02s)
    testing.go:569: Step 0 error: Check failed: Check 1/1 error: data.azurerm_resources.test: Attribute 'resources.#' expected "1", got "0"
FAIL

------- Stderr: -------
2019/10/09 17:03:03 [DEBUG] Registering Data Sources for "Compute"..
2019/10/09 17:03:03 [DEBUG] Registering Resources for "Compute"..

I've just seen this happening on my side too. It seems we have a replication lag issue here, the API sometimes returns no results.

I can add a retry function to the data source or we can add a sleep to the test case (somehow?), but that dosen't sound right to me.

@katbyte
Copy link
Collaborator

katbyte commented Oct 9, 2019

🤔 given its a search index probably updating a retry in the resource makes less sense then a delay in the test steps.

@tiwood
Copy link
Contributor Author

tiwood commented Oct 9, 2019

🤔 given its a search index probably updating a retry in the resource makes less sense then a delay in the test steps.

Any pointers how I could do that?

I've added some code to account for the replication lag/indexing, which seems to work now:

stateConf := &resource.StateChangeConf{
		Pending:                   []string{"ResourcesNotFound"},
		Target:                    []string{"ResourcesFound"},
		Refresh:                   dataArmResourcesStateStatusCodeRefreshFunc(ctx, meta, filter),
		Timeout:                   2 * time.Minute,
		MinTimeout:                30 * time.Second,
		ContinuousTargetOccurence: 2,
}

func dataArmResourcesStateStatusCodeRefreshFunc(ctx context.Context, meta interface{}, filter string) resource.StateRefreshFunc {
	return func() (interface{}, string, error) {
		resourcesClient := meta.(*ArmClient).Resource.ResourcesClient
		resp, err := resourcesClient.ListComplete(ctx, filter, "", nil)

		if err != nil {
			return nil, "", fmt.Errorf("Error while waiting for the resources to become available: %+v", err)
		}

		if resp.Value().ID == nil {
			return resp, "ResourcesNotFound", nil
		}

		return resp, "ResourcesFound", nil
	}
}

@katbyte
Copy link
Collaborator

katbyte commented Oct 9, 2019

Thanks! Give me a bit to discuss the best way forward internally and we'll get back to you 🙂

@tiwood
Copy link
Contributor Author

tiwood commented Oct 11, 2019

I've pushed the changes which include waitForState() and the changes requested by @tombuildsstuff.

Please let me know if you want to put a delay in the tests rather than wait for state in the data source itself.

@tombuildsstuff tombuildsstuff self-assigned this Oct 14, 2019
@tombuildsstuff
Copy link
Contributor

hey @tiwood

Thanks for pushing those changes.

Taking a look through it appears the "replication issue" referred to above is a known issue in the test framework; where the Data Source will be called before the Resource has finished provisioning, unless there's an explicit depends_on. To work around this we're using two steps for provisioning the tests in the data sources, for example:

func TestAccDataSourceAzureRMResources_ByName(t *testing.T) {
	dataSourceName := "data.azurerm_resources.test"
	ri := tf.AccRandTimeInt()
	rs := acctest.RandString(4)
	location := testLocation()

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:  func() { testAccPreCheck(t) },
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccDataSourceAzureRMStorageAccount_basic(ri, rs, location),
			},
			{
				Config: testAccDataSourceAzureRMResources_ByName(ri, rs, location),
				Check: resource.ComposeTestCheckFunc(
					resource.TestCheckResourceAttr(dataSourceName, "resources.#", "1"),
				),
			},
		},
	})
}

which means that the Resources will exist before the Data Source looks them up (we could also use a depends_on, but we're intentionally not since this is a test framework specific issue)

When updating the code to account for this (and removing the WaitForState check) the tests pass:

 $ acctests azurerm TestAccDataSourceAzureRMResources_ByName
=== RUN   TestAccDataSourceAzureRMResources_ByName
=== PAUSE TestAccDataSourceAzureRMResources_ByName
=== CONT  TestAccDataSourceAzureRMResources_ByName
--- PASS: TestAccDataSourceAzureRMResources_ByName (134.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	134.198s

Since I've made these changes locally to be able to confirm this I hope you don't mind but I'm going to push a commit with said change so that we can get this merged - but this otherwise LGTM 👍

Thanks!

@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review October 14, 2019 07:31

dismissing since changes have been pushed

@tombuildsstuff
Copy link
Contributor

Tests pass:

Screenshot 2019-10-14 at 09 40 32

@tombuildsstuff tombuildsstuff merged commit dddeb70 into hashicorp:master Oct 14, 2019
tombuildsstuff added a commit that referenced this pull request Oct 14, 2019
@ghost
Copy link

ghost commented Oct 29, 2019

This has been released in version 1.36.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.36.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Nov 13, 2019

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!

@ghost ghost locked and limited conversation to collaborators Nov 13, 2019
@tiwood tiwood deleted the ds_arm_resource branch November 13, 2019 19:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants