-
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
azurerm_cognitive_account - support for the qna_runtime_endpoint
property
#5778
azurerm_cognitive_account - support for the qna_runtime_endpoint
property
#5778
Conversation
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.
Hi @RJPearson94,
Thank you for fixing the resource, i've given this a quick review and left a couple comments inline. Mainly we need to remove the properties block as we don't' include it on other resources.
azurerm/internal/services/cognitive/resource_arm_cognitive_account.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/cognitive/tests/resource_arm_cognitive_account_test.go
Outdated
Show resolved
Hide resolved
…tive_services_account' into fix-account-props
Closing PR due to issues of additional commit being included. i will reopen a new one. Update: Decided to reopen this PR and switched the base branch to another branch and back onto master to force Github to re-compare the file changes |
Thanks for the feedback @katbyte. I have updated the PR with the changes that you recommended |
Hi @katbyte, thanks for looking at the PR is it possible to get these updates reviewed please |
Hey @katbyte, @mbfrahry & @tombuildsstuff, is it possible to get this change reviewed as it would be great to get this out as part of the 2.3.0 release. |
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 the updates @RJPearson94,
There are just a couple comments and 1 question i've left inline, but the real blocker is the tests are failing on our side:
------- Stdout: -------
=== RUN TestAccAzureRMCognitiveAccount_updateAccountApiProperties
=== PAUSE TestAccAzureRMCognitiveAccount_updateAccountApiProperties
=== CONT TestAccAzureRMCognitiveAccount_updateAccountApiProperties
--- FAIL: TestAccAzureRMCognitiveAccount_updateAccountApiProperties (89.92s)
testing.go:640: Step 0 error: errors during apply:
Error: Error creating Cognitive Services Account "acctestcogacc-200321002604096733" (Resource Group "acctestRG-200321002604096733"): cognitiveservices.AccountsClient#Create: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidApiSetId" Message="The account type 'QnAMaker' is either invalid or unavailable in given region."
What regions is qna available in? we might have to hard code it if it's not in one of our test locations: primary (west europe), secondary (east us 2), Ternary (francecentral)
azurerm/internal/services/cognitive/resource_arm_cognitive_account.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/cognitive/resource_arm_cognitive_account.go
Outdated
Show resolved
Hide resolved
Hi @katbyte, thanks for the feedback. I have made the changes that you recommended. The Cognitive Service Account for QnAMaker isn't available in all regions (I can't find any documentation stating where these Cognitive Service Accounts can be provisioned). I have been testing with West US (and this is what the Azure Portal defaults to) so I have hardcoded this location in the tests |
AccountProperties
on cognitive service accountsqna_runtime_endpoint
property
…ies_to_cognitive_services_account
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.
Hey @RJPearson94,
Thanks for the revisions, i hope you don't mind but i pushed a couple changes to get this merged today and tidy up the tests and remove one as they are expensive to run nightly. LGTM now! 👍
Thanks for making the changes and merging this @katbyte, greatly appreciated 👍 |
This has been released in version 2.3.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.3.0"
}
# ... other configuration ... |
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! |
This commit creates a new optional
qna_runtime_endpoint
property at the top level of the cognitive account resourcePreviously if the kind of a cognitive service account was set to
QnAMaker
an error would be thrown from the Microsoft API (see issue for more details).This issue was blocked due to missing functionality in the Azure SDK however support to allow the Account Properties and Account API Properties to be set was added in V25. This change allows the
qna_runtime_endpoint
to be set and will allowQnAMaker
cognitive service accounts to be created via Terraform.I have added 2 Acceptance Tests for setting the
qna_runtime_endpoint
withQnAMaker
set as the kind, 1 as a new resource and the 2nd with the url updated. I tried to add an acceptance test for setting theqna_runtime_endpoint
on a nonQnAMaker
kind cognitive service account i.e.Face
however the Microsoft API ignores this property when it is supplied so it is not returned in the response. As a result the tests failed with the following errorImportStateVerify attributes not equivalent
. I decided remove this test.The website docs have been updated to match the current functionality.
Fixes #4338
I am still quite new to Go and this is my first commit to the Terraform Providers so any feedback will be greatly appreciated