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

r\cognitive_account: Add support for custom_question_answering_search_service_id #15804

Merged
merged 4 commits into from
Mar 29, 2022

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Mar 11, 2022

Fix #14011 and #9271

@myc2h6o myc2h6o changed the title r\cognitive_account: Add support for qna_service_endpoint_id r\cognitive_account: Add support for qna_search_endpoint_id Mar 11, 2022
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Mar 11, 2022

image

Copy link
Member

@stephybun stephybun 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 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": {
Copy link
Member

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`")
Copy link
Member

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

Suggested change
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)
Copy link
Member

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

Suggested change
-> **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)

@myc2h6o myc2h6o changed the title r\cognitive_account: Add support for qna_search_endpoint_id r\cognitive_account: Add support for custom_question_answering_search_service_id Mar 15, 2022
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Mar 15, 2022

Hey @stephybun Thanks for reviewing the change! I've renamed the property to custom_question_answering_search_service_id, please take a look.

The failed test case TestAccCognitiveAccount_networkAcls seems to be failing on main as well from some time back, probably not because of this change.

@stephybun
Copy link
Member

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!

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Mar 18, 2022

Sure let me investigate the test failure as well.

@myc2h6o
Copy link
Contributor Author

myc2h6o commented Mar 22, 2022

I've created a new pr #15932 to fix the test

@geremy42
Copy link

@myc2h6o Thanks for the job! I'm also waiting for this update. Do you have any idea when the PR could be merged ?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @myc2h6o, LGTM 🚀

@stephybun stephybun merged commit 8bc1c36 into hashicorp:main Mar 29, 2022
@github-actions github-actions bot added this to the v3.1.0 milestone Mar 29, 2022
stephybun added a commit that referenced this pull request Mar 29, 2022
@myc2h6o myc2h6o deleted the cognitive_qna branch March 30, 2022 04:53
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

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!

@github-actions
Copy link

github-actions bot commented May 9, 2022

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 9, 2022
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.

Support for [Azure Question answering feature]
4 participants