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_cognitive_account - support for the qna_runtime_endpoint property #5778

Merged
merged 17 commits into from Mar 25, 2020
Merged

azurerm_cognitive_account - support for the qna_runtime_endpoint property #5778

merged 17 commits into from Mar 25, 2020

Conversation

RJPearson94
Copy link
Contributor

@RJPearson94 RJPearson94 commented Feb 16, 2020

This commit creates a new optional qna_runtime_endpoint property at the top level of the cognitive account resource

Previously 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 allow QnAMaker cognitive service accounts to be created via Terraform.

I have added 2 Acceptance Tests for setting the qna_runtime_endpoint with QnAMaker 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 the qna_runtime_endpoint on a non QnAMaker 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 error ImportStateVerify 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

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.

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.

@ghost ghost added size/XXL and removed size/L labels Mar 10, 2020
@RJPearson94
Copy link
Contributor Author

RJPearson94 commented Mar 10, 2020

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

@ghost ghost removed the waiting-response label Mar 10, 2020
@RJPearson94 RJPearson94 reopened this Mar 10, 2020
@RJPearson94 RJPearson94 changed the base branch from master to r/domain-registration March 10, 2020 03:35
@RJPearson94 RJPearson94 changed the base branch from r/domain-registration to master March 10, 2020 03:35
@RJPearson94
Copy link
Contributor Author

Thanks for the feedback @katbyte. I have updated the PR with the changes that you recommended

@ghost ghost added size/L and removed size/XXL labels Mar 11, 2020
@RJPearson94
Copy link
Contributor Author

Hi @katbyte, thanks for looking at the PR is it possible to get these updates reviewed please

@RJPearson94
Copy link
Contributor Author

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.

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.

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)

@katbyte katbyte added this to the v2.3.0 milestone Mar 21, 2020
@RJPearson94
Copy link
Contributor Author

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

@ghost ghost removed the waiting-response label Mar 21, 2020
@katbyte katbyte changed the title Add support for AccountProperties on cognitive service accounts azurerm_cognitive_account - support for the qna_runtime_endpoint property Mar 25, 2020
@ghost ghost added size/M and removed size/L labels Mar 25, 2020
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.

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! 👍

@katbyte katbyte merged commit 3cbceda into hashicorp:master Mar 25, 2020
katbyte added a commit that referenced this pull request Mar 25, 2020
@RJPearson94
Copy link
Contributor Author

Thanks for making the changes and merging this @katbyte, greatly appreciated 👍

@RJPearson94 RJPearson94 deleted the add_account_properties_to_cognitive_services_account branch March 25, 2020 06:26
@ghost
Copy link

ghost commented Mar 27, 2020

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 ...

@ghost
Copy link

ghost commented Apr 24, 2020

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Apr 24, 2020
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.

azurerm_cognitive_account QnAMaker missing api-properties
2 participants