-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
r\cognitive_account
: Add support for custom_question_answering_search_service_id
#15804
Conversation
r\cognitive_account
: Add support for qna_service_endpoint_id
r\cognitive_account
: Add support for qna_search_endpoint_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @myc2h6o. I left a few suggestions in-line. There is also one test failure
------- Stdout: -------
=== RUN TestAccCognitiveAccount_networkAcls
=== PAUSE TestAccCognitiveAccount_networkAcls
=== CONT TestAccCognitiveAccount_networkAcls
testcase.go:110: Step 3/4 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
stdout
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# azurerm_cognitive_account.test will be updated in-place
~ resource "azurerm_cognitive_account" "test" {
id = "/subscriptions/*******/resourceGroups/acctestRG-cognitive-220314105613118558/providers/Microsoft.CognitiveServices/accounts/acctestcogacc-220314105613118558"
name = "acctestcogacc-220314105613118558"
tags = {}
# (12 unchanged attributes hidden)
~ network_acls {
# (2 unchanged attributes hidden)
+ virtual_network_rules {
+ ignore_missing_vnet_service_endpoint = false
+ subnet_id = "/subscriptions/*******/resourceGroups/acctestRG-cognitive-220314105613118558/providers/Microsoft.Network/virtualNetworks/acctestvirtnet220314105613118558/subnets/acctestsubneta220314105613118558"
}
+ virtual_network_rules {
+ ignore_missing_vnet_service_endpoint = false
+ subnet_id = "/subscriptions/*******/resourceGroups/acctestRG-cognitive-220314105613118558/providers/Microsoft.Network/virtualNetworks/acctestvirtnet220314105613118558/subnets/acctestsubnetb220314105613118558"
}
}
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccCognitiveAccount_networkAcls (990.54s)
FAIL
Whilst it may not be related to your changes it'd be great if you could fix that up as well!
@@ -807,6 +816,12 @@ func resourceCognitiveAccountSchema() map[string]*pluginsdk.Schema { | |||
ValidateFunc: validation.IsURLWithHTTPorHTTPS, | |||
}, | |||
|
|||
"qna_search_endpoint_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this perhaps be better named custom_question_answering_search_service_id
?
From the link added in the docs below QnAMaker has been renamed to custom question answering, and in the Portal and Docs it looks like it is also referred to as the Search Service and not an Endpoint.
if kind == "TextAnalytics" { | ||
props.QnaAzureSearchEndpointId = utils.String(v.(string)) | ||
} else { | ||
return nil, fmt.Errorf("qna_search_endpoint_id can only used set when kind is set to `TextAnalytics`") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can make this more consistent with the error message above
return nil, fmt.Errorf("qna_search_endpoint_id can only used set when kind is set to `TextAnalytics`") | |
return nil, fmt.Errorf("the Search Service ID `custom_question_answering_search_service_id` can only be set when kind is set to `TextAnalytics`") |
|
||
* `qna_search_endpoint_id` - If `kind` is `TextAnalytics` the ID to the Search service. | ||
|
||
-> **NOTE:** `qna_search_endpoint_id` is used for [the new version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/custom-question-answering), while `qna_runtime_endpoint` is used for [the old version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/overview/overview) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might benefit from a bit more information
-> **NOTE:** `qna_search_endpoint_id` is used for [the new version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/custom-question-answering), while `qna_runtime_endpoint` is used for [the old version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/overview/overview) | |
-> **NOTE:** `custom_question_answering_search_service_id` is used for [Custom Question Answering, the renamed version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/custom-question-answering), while `qna_runtime_endpoint` is used for [the old version of QnA Maker](https://docs.microsoft.com/azure/cognitive-services/qnamaker/overview/overview) |
r\cognitive_account
: Add support for qna_search_endpoint_id
r\cognitive_account
: Add support for custom_question_answering_search_service_id
Hey @stephybun Thanks for reviewing the change! I've renamed the property to The failed test case |
Thanks for making those changes @myc2h6o. I'm aware the test failure is not related to the changes made in this PR, but it should be fixed anyhow. Since you're already working on this resource and have the context you're well suited to taking a look into this and to help us fix it. Thanks again! |
Sure let me investigate the test failure as well. |
I've created a new pr #15932 to fix the test |
@myc2h6o Thanks for the job! I'm also waiting for this update. Do you have any idea when the PR could be merged ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @myc2h6o, LGTM 🚀
This functionality has been released in v3.1.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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. |
Fix #14011 and #9271