New data source `azurerm_search_service` #10181

jackbatzner · 2021-01-13T19:19:32Z

tombuildsstuff

Thanks for this PR :)

I've taken a look through and this is looking good, if we can fix up the comments and linting issues then this should otherwise be good to go 👍

Thanks!

tombuildsstuff · 2021-01-14T09:45:28Z

azurerm/internal/services/search/search_service_data_source.go

+		if utils.ResponseWasNotFound(resp.Response) {
+			log.Printf("[INFO] Error reading Search Service %q - removing from state", d.Id())
+			d.SetId("")
+			return nil


since this is a Data Source we should raise an error if this is not found rather than returning nil here - so could we make this:

Suggested change

return nil

return fmt.Errorf("%s was not found", id)

(the format string for "id" will be "Search Service %q (Resource Group %q)")

tombuildsstuff · 2021-01-14T09:47:10Z

azurerm/internal/services/search/search_service_data_source_test.go

+		{
+			Config: r.basic(data),
+			Check: resource.ComposeTestCheckFunc(
+				check.That(data.ResourceName).Key("id").Exists(),


we can remove this, since otherwise the resource won't be in the state/will raise an error (which'll be hit before this point)

Suggested change

check.That(data.ResourceName).Key("id").Exists(),

tombuildsstuff · 2021-01-14T09:47:54Z

website/docs/d/search_service.html.markdown

+
+* `replica_count` - The number of replica's which have been created.
+
+-> **Note:** `partition_count` and `replica_count` can only be configured when using a `standard` sku.


since this is a data source, this isn't configurable - so we can remove this (since the fields should get a default value either way)

Suggested change

-> **Note:** `partition_count` and `replica_count` can only be configured when using a `standard` sku.

tombuildsstuff · 2021-01-14T09:48:36Z

website/docs/d/search_service.html.markdown

+
+* `query_keys` - A `query_keys` block as defined below.
+
+* `public_network_access_enabled` - Whether or not public network access is allowed for this resource. Defaults to `true`.


this is a Data Source which is read-only, so the default value doesn't apply here - could we also update the messaging to highlight this isn't configurable:

Suggested change

* `public_network_access_enabled` - Whether or not public network access is allowed for this resource. Defaults to `true`.

* `public_network_access_enabled` - Whether or not public network access is enabled for this Search Service.

tombuildsstuff · 2021-01-14T09:49:45Z

website/docs/d/search_service.html.markdown

+
+* `identity` - An `identity` block as defined below.
+
+* `tags` - (Optional) A mapping of tags which should be assigned to the Search Service.


(as above) since this isn't configurable can we update the wording to highlight that this is read only:

Suggested change

* `tags` - (Optional) A mapping of tags which should be assigned to the Search Service.

* `tags` - A mapping of tags which are assigned to the Search Service.

We don't actually set the tags on this data source so I'll just remove this line 😄

tombuildsstuff · 2021-01-14T09:50:07Z

website/docs/d/search_service.html.markdown

+
+A `identity` block supports the following:
+
+* `type` - The Type of Identity which should be used for the Search Service. At this time the only possible value is `SystemAssigned`.


(as above)

Suggested change

* `type` - The Type of Identity which should be used for the Search Service. At this time the only possible value is `SystemAssigned`.

* `type` - The type of Managed Identity which used for the Search Service.

jackbatzner · 2021-01-21T17:46:55Z

Thanks for the review @tombuildsstuff ! Let me know if there are any other changes you see

tombuildsstuff

hey @Brunhil

Thanks for pushing those changes - I've taken a look through and left some comments inline but if we can fix those up then this should be good to go 👍

Thanks!

tombuildsstuff · 2021-01-22T08:43:05Z

azurerm/internal/services/search/search_service_data_source.go

+			"name": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,


as the linter says, a Data Sources properties can't be ForceNew - so we can remove this:

Suggested change

ForceNew: true,

tombuildsstuff · 2021-01-22T08:43:47Z

azurerm/internal/services/search/search_service_data_source.go

+			"identity": {
+				Type:     schema.TypeList,
+				Computed: true,
+				MaxItems: 1,


as the linter mentions, since this is a Data Source we can remove the max items here, since these are read-only/not user configurable:

Suggested change

MaxItems: 1,

tombuildsstuff · 2021-01-22T08:55:05Z

azurerm/internal/services/search/search_service_data_source.go

+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			log.Printf("[INFO] Error reading Search Service %q - removing from state", d.Id())
+			d.SetId("")


we can remove both of these lines since the error will raise this anyway

tombuildsstuff · 2021-01-22T08:56:08Z

azurerm/internal/services/search/search_service_data_source.go

+	id, err := parse.SearchServiceID(d.Id())
+	if err != nil {
+		return err
+	}


a Data Source won't have an ID at this point, instead it needs to look this up using the name and resource_group_name - that said we need to build this up to use it below - so can we make this:

Suggested change

}

subscriptionId := meta.(*clients.Client).Account.SubscriptionID

id := parse.NewSearchServiceID(subscriptionId, d.Get("resource_group_name").(string), d.Get("name").(string))

tombuildsstuff · 2021-01-22T08:56:47Z

azurerm/internal/services/search/search_service_data_source.go

+	}
+
+	d.Set("name", id.Name)
+	d.Set("resource_group_name", id.ResourceGroup)


since the user is specifying these, we can remove these and just set the ID instead (which is needed for this to get tracked into the state):

Suggested change

d.Set("resource_group_name", id.ResourceGroup)

d.SetID(id.ID())

tombuildsstuff · 2021-01-22T08:59:08Z

website/docs/d/search_service.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.


for the data source this will already exist, so we can update the language here:

Suggested change

* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.

* `name` - (Required) The Name of the Search Service.

(since this is a Data Source this also doesn't force a new resource

tombuildsstuff · 2021-01-22T08:59:25Z

website/docs/d/search_service.html.markdown

+
+* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.
+
+* `resource_group_name` - (Required) The name of the Resource Group where the Search Service should exist. Changing this forces a new Search Service to be created.


(same here)

Suggested change

* `resource_group_name` - (Required) The name of the Resource Group where the Search Service should exist. Changing this forces a new Search Service to be created.

* `resource_group_name` - (Required) The name of the Resource Group where the Search Service exists.

katbyte · 2021-01-23T19:02:10Z

Test is also failing with

------- Stdout: -------
=== RUN   TestAccDataSourceSearchService_basic
=== PAUSE TestAccDataSourceSearchService_basic
=== CONT  TestAccDataSourceSearchService_basic
    testing.go:684: Step 0 error: errors during apply:
        
        Error: Cannot parse Azure ID: parse "": empty url
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test424173270/main.tf line 24:
          (source code not available)
        
        
--- FAIL: TestAccDataSourceSearchService_basic (136.98s)
FAIL

tombuildsstuff

LGTM - thanks for pushing those changes @Brunhil

azurerm/internal/services/search/search_service_data_source_test.go

…st.go Co-authored-by: Tom Harvey <[email protected]>

katbyte

Even after committing that change it the test is still failing:

------- Stdout: -------
=== RUN   TestAccDataSourceSearchService_basic
=== PAUSE TestAccDataSourceSearchService_basic
=== CONT  TestAccDataSourceSearchService_basic
    testing.go:684: Step 0 error: Check failed: Check 6/8 error: data.azurerm_search_service.test: Attribute 'identity.0.type' expected to be set
--- FAIL: TestAccDataSourceSearchService_basic (104.58s)
FAIL

jackbatzner · 2021-01-28T02:55:56Z

Turns out I was missing the identity block which I added in the most recent commit. Hopefully things are good now @katbyte ! 🤞

katbyte

Aside from one comment this LGTM 👍

azurerm/internal/services/search/search_service_data_source_test.go

…st.go

ghost · 2021-01-28T18:11:39Z

This has been released in version 2.45.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 = "~> 2.45.0"
}
# ... other configuration ...

ghost · 2021-02-27T14:51:23Z

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 👉 [email protected]. Thanks!

New data source azurerm_search_service

f6b7d1c

ghost added documentation size/L labels Jan 13, 2021

Jack added 3 commits January 13, 2021 13:44

Fix copy/pasta error

6ba2550

Fix

2d14c65

Use right test helper

b4ba625

tombuildsstuff added new-data-source service/search labels Jan 14, 2021

tombuildsstuff suggested changes Jan 14, 2021

View reviewed changes

tombuildsstuff added the waiting-response label Jan 14, 2021

tombuildsstuff added this to the v2.44.0 milestone Jan 14, 2021

tombuildsstuff modified the milestones: v2.44.0, v2.45.0 Jan 21, 2021

Review feedback

672c97d

ghost removed the waiting-response label Jan 21, 2021

jackbatzner requested a review from tombuildsstuff January 21, 2021 19:04

tombuildsstuff suggested changes Jan 22, 2021

View reviewed changes

tombuildsstuff added the waiting-response label Jan 22, 2021

More PR feedback

fdaa63b

jackbatzner requested a review from tombuildsstuff January 26, 2021 20:58

tombuildsstuff removed the waiting-response label Jan 27, 2021

tombuildsstuff approved these changes Jan 27, 2021

View reviewed changes

tombuildsstuff reviewed Jan 27, 2021

View reviewed changes

azurerm/internal/services/search/search_service_data_source_test.go Outdated Show resolved Hide resolved

Update azurerm/internal/services/search/search_service_data_source_te…

cb29eea

…st.go Co-authored-by: Tom Harvey <[email protected]>

katbyte requested changes Jan 28, 2021

View reviewed changes

Manually define TF for test

3d27642

ghost added size/XL and removed size/L labels Jan 28, 2021

jackbatzner requested a review from katbyte January 28, 2021 02:56

katbyte approved these changes Jan 28, 2021

View reviewed changes

azurerm/internal/services/search/search_service_data_source_test.go Outdated Show resolved Hide resolved

Update azurerm/internal/services/search/search_service_data_source_te…

de4b23d

…st.go

katbyte merged commit eb49bad into hashicorp:master Jan 28, 2021

katbyte added a commit that referenced this pull request Jan 28, 2021

CHANGELOG.md for #10181

a0ad7c3

ghost locked as resolved and limited conversation to collaborators Feb 27, 2021

jackbatzner deleted the data_search_service branch March 2, 2021 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New data source `azurerm_search_service` #10181

New data source `azurerm_search_service` #10181

jackbatzner commented Jan 13, 2021

tombuildsstuff left a comment

tombuildsstuff Jan 14, 2021

tombuildsstuff Jan 14, 2021

tombuildsstuff Jan 14, 2021

tombuildsstuff Jan 14, 2021

tombuildsstuff Jan 14, 2021

jackbatzner Jan 21, 2021

tombuildsstuff Jan 14, 2021

jackbatzner commented Jan 21, 2021

tombuildsstuff left a comment

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

tombuildsstuff Jan 22, 2021

katbyte commented Jan 23, 2021

tombuildsstuff left a comment

katbyte left a comment

jackbatzner commented Jan 28, 2021

katbyte left a comment

ghost commented Jan 28, 2021

ghost commented Feb 27, 2021


		* `replica_count` - The number of replica's which have been created.

		-> Note: `partition_count` and `replica_count` can only be configured when using a `standard` sku.


		* `query_keys` - A `query_keys` block as defined below.

		* `public_network_access_enabled` - Whether or not public network access is allowed for this resource. Defaults to `true`.


		* `identity` - An `identity` block as defined below.

		* `tags` - (Optional) A mapping of tags which should be assigned to the Search Service.

	* `tags` - (Optional) A mapping of tags which should be assigned to the Search Service.
	* `tags` - A mapping of tags which are assigned to the Search Service.


		A `identity` block supports the following:

		* `type` - The Type of Identity which should be used for the Search Service. At this time the only possible value is `SystemAssigned`.

	* `type` - The Type of Identity which should be used for the Search Service. At this time the only possible value is `SystemAssigned`.
	* `type` - The type of Managed Identity which used for the Search Service.

	}
	subscriptionId := meta.(*clients.Client).Account.SubscriptionID
	id := parse.NewSearchServiceID(subscriptionId, d.Get("resource_group_name").(string), d.Get("name").(string))

	d.Set("resource_group_name", id.ResourceGroup)
	d.SetID(id.ID())


		The following arguments are supported:

		* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.

	* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.
	* `name` - (Required) The Name of the Search Service.


		* `name` - (Required) The Name which should be used for this Search Service. Changing this forces a new Search Service to be created.

		* `resource_group_name` - (Required) The name of the Resource Group where the Search Service should exist. Changing this forces a new Search Service to be created.

New data source azurerm_search_service #10181

New data source azurerm_search_service #10181

Conversation

jackbatzner commented Jan 13, 2021

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackbatzner commented Jan 21, 2021

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Jan 23, 2021

tombuildsstuff left a comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

jackbatzner commented Jan 28, 2021

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Jan 28, 2021

ghost commented Feb 27, 2021

New data source `azurerm_search_service` #10181

New data source `azurerm_search_service` #10181