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

azurerm_search_service - upgrade search service API to version 2023-11-01 and expose semantic_search_sku field #23698

Merged
merged 4 commits into from
Oct 27, 2023

Conversation

WodansSon
Copy link
Collaborator

No description provided.

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

Thanks for this PR - I've taken a look through and left a few comments inline, mostly to make use of the None pattern for this field - but if we can update this to account for that, then this otherwise LGTM 👍

Thanks!

Comment on lines 167 to 169
Default: services.SearchSemanticSearchDisabled,
ValidateFunc: validation.StringInSlice([]string{
string(services.SearchSemanticSearchDisabled),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should treat disabled as a None value - meaning we can remove this default and the possible value here:

Suggested change
Default: services.SearchSemanticSearchDisabled,
ValidateFunc: validation.StringInSlice([]string{
string(services.SearchSemanticSearchDisabled),
ValidateFunc: validation.StringInSlice([]string{

In favour of this field being empty/unset when null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -233,6 +245,11 @@ func resourceSearchServiceCreate(d *pluginsdk.ResourceData, meta interface{}) er
return fmt.Errorf("%q SKUs in %q mode can have a maximum of 3 partitions, got %d", string(services.SkuNameStandardThree), string(services.HostingModeHighDensity), partitionCount)
}

// NOTE: Semantic Search SKU cannot be set if the SKU is 'free'
if skuName == services.SkuNameFree && semanticSearchSku != string(services.SearchSemanticSearchDisabled) {
return fmt.Errorf("'semantic_search_sku' can only be set to %q if the 'sku' field is set to %q, got %q", string(services.SearchSemanticSearchDisabled), string(services.SkuNameFree), semanticSearchSku)
Copy link
Contributor

Choose a reason for hiding this comment

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

which allows this to become:

Suggested change
return fmt.Errorf("'semantic_search_sku' can only be set to %q if the 'sku' field is set to %q, got %q", string(services.SearchSemanticSearchDisabled), string(services.SkuNameFree), semanticSearchSku)
return fmt.Errorf("`semantic_search_sku` can only be specified when `sku` is not set to %q string(services.SkuNameFree)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -281,6 +298,7 @@ func resourceSearchServiceCreate(d *pluginsdk.ResourceData, meta interface{}) er
DisableLocalAuth: pointer.To(!localAuthenticationEnabled),
PartitionCount: pointer.To(partitionCount),
ReplicaCount: pointer.To(replicaCount),
SemanticSearch: pointer.To(services.SearchSemanticSearch(semanticSearchSku)),
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case can we conditionally assign this field depending on whether it's != ""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -209,6 +220,7 @@ func resourceSearchServiceCreate(d *pluginsdk.ResourceData, meta interface{}) er
cmkEnforcementEnabled := d.Get("customer_managed_key_enforcement_enabled").(bool)
localAuthenticationEnabled := d.Get("local_authentication_enabled").(bool)
authenticationFailureMode := d.Get("authentication_failure_mode").(string)
semanticSearchSku := d.Get("semantic_search_sku").(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this field is Optional, we can then default this here:

Suggested change
semanticSearchSku := d.Get("semantic_search_sku").(string)
semanticSearchSku := services.SearchSemanticSearchDisabled
if v := d.Get("semantic_search_sku").(string); v != "" {
semanticSearchSku = services.SearchSemanticSearch(v)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 456 to 463
semanticSearchSku := d.Get("semantic_search_sku").(string)

// NOTE: Semantic Search SKU cannot be set if the SKU is 'free'
if pointer.From(model.Sku.Name) == services.SkuNameFree && semanticSearchSku != string(services.SearchSemanticSearchDisabled) {
return fmt.Errorf("'semantic_search_sku' can only be set to %q if the 'sku' field is set to %q, got %q", string(services.SearchSemanticSearchDisabled), string(services.SkuNameFree), semanticSearchSku)
}

model.Properties.SemanticSearch = pointer.To(services.SearchSemanticSearch(semanticSearchSku))
Copy link
Contributor

Choose a reason for hiding this comment

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

Which means this can become:

Suggested change
semanticSearchSku := d.Get("semantic_search_sku").(string)
// NOTE: Semantic Search SKU cannot be set if the SKU is 'free'
if pointer.From(model.Sku.Name) == services.SkuNameFree && semanticSearchSku != string(services.SearchSemanticSearchDisabled) {
return fmt.Errorf("'semantic_search_sku' can only be set to %q if the 'sku' field is set to %q, got %q", string(services.SearchSemanticSearchDisabled), string(services.SkuNameFree), semanticSearchSku)
}
model.Properties.SemanticSearch = pointer.To(services.SearchSemanticSearch(semanticSearchSku))
semanticSearchSku := services.SearchSemanticSearchDisabled
if v := d.Get("semantic_search_sku").(string); v != "" {
semanticSearchSku = services.SearchSemanticSearch(v)
}
// NOTE: Semantic Search SKU cannot be set if the SKU is 'free'
if pointer.From(model.Sku.Name) == services.SkuNameFree && semanticSearchSku != services.SearchSemanticSearchDisabled {
return fmt.Errorf("`semantic_search_sku` can only be specified when `sku` is not set to %q string(services.SkuNameFree)
}
model.Properties.SemanticSearch = pointer.To(semanticSearchSku)

To allow this property to be removed/marked as null in the configuration to remove this field

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -233,6 +245,11 @@ func resourceSearchServiceCreate(d *pluginsdk.ResourceData, meta interface{}) er
return fmt.Errorf("%q SKUs in %q mode can have a maximum of 3 partitions, got %d", string(services.SkuNameStandardThree), string(services.HostingModeHighDensity), partitionCount)
}

// NOTE: Semantic Search SKU cannot be set if the SKU is 'free'
if skuName == services.SkuNameFree && semanticSearchSku != string(services.SearchSemanticSearchDisabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

which would mean we don't need to cast this one:

Suggested change
if skuName == services.SkuNameFree && semanticSearchSku != string(services.SearchSemanticSearchDisabled) {
if skuName == services.SkuNameFree && semanticSearchSku != services.SearchSemanticSearchDisabled {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 558 to 560
if props.SemanticSearch != nil {
semanticSearchSku = string(pointer.From(props.SemanticSearch))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

to map the None behaviour this can become:

Suggested change
if props.SemanticSearch != nil {
semanticSearchSku = string(pointer.From(props.SemanticSearch))
}
semanticSearchSku := ""
if props.SemanticSearch != nil && pointer.From(props.SemanticSearch) != services.SearchSemanticSearchDisabled {
semanticSearchSku = string(pointer.From(props.SemanticSearch))
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@@ -108,6 +108,10 @@ The following arguments are supported:

* `replica_count` - (Optional) Specifies the number of Replica's which should be created for this Search Service. This field cannot be set when using a `free` sku ([see the Microsoft documentation](https://learn.microsoft.com/azure/search/search-sku-tier)).

* `semantic_search_sku` - (Optional) The Semantic Search SKU which should be used for this Search Service. Possible values include `disabled`, `free` and `standard`. Defaults to `disabled`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Updated to remove the disabled value, but typically these are prefixed with Specifies:

Suggested change
* `semantic_search_sku` - (Optional) The Semantic Search SKU which should be used for this Search Service. Possible values include `disabled`, `free` and `standard`. Defaults to `disabled`.
* `semantic_search_sku` - (Optional) Specifies the Semantic Search SKU which should be used for this Search Service. Possible values include `free` and `standard`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@WodansSon
Copy link
Collaborator Author

image

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.

LGTM 🔎

@WodansSon WodansSon merged commit cd601ba into main Oct 27, 2023
23 checks passed
@WodansSon WodansSon deleted the e_semantic_search branch October 27, 2023 00:30
@github-actions github-actions bot added this to the v3.78.0 milestone Oct 27, 2023
WodansSon added a commit that referenced this pull request Oct 27, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2024
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