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

azure_cognitive_deployment - Changes model.version property from Required to Optional. #24264

Merged
merged 8 commits into from
Jan 23, 2024

Conversation

liuwuliuyun
Copy link
Contributor

@liuwuliuyun liuwuliuyun commented Dec 18, 2023

azure_cognitive_deployment

Testing results:

GOROOT=C:\Program Files\Go #gosetup
GOPATH=C:\Users\yunliu1\go #gosetup
"C:\Program Files\Go\bin\go.exe" test -c -o C:\Users\yunliu1\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___TestAccCognitiveDeploymentSequential_in_github_com_hashicorp_terraform_provider_azurerm_internal_services_cognitive.test.exe github.com/hashicorp/terraform-provider-azurerm/internal/services/cognitive #gosetup
"C:\Program Files\Go\bin\go.exe" tool test2json -t C:\Users\yunliu1\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___TestAccCognitiveDeploymentSequential_in_github_com_hashicorp_terraform_provider_azurerm_internal_services_cognitive.test.exe -test.v -test.paniconexit0 -test.run ^\QTestAccCognitiveDeploymentSequential\E$ #gosetup
=== RUN   TestAccCognitiveDeploymentSequential
--- PASS: TestAccCognitiveDeploymentSequential (802.33s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment
    --- PASS: TestAccCognitiveDeploymentSequential/deployment (802.33s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/requiresImport
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/requiresImport (193.04s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/complete
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/complete (187.35s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/update
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/update (235.89s)
=== RUN   TestAccCognitiveDeploymentSequential/deployment/basic
        --- PASS: TestAccCognitiveDeploymentSequential/deployment/basic (186.05s)
PASS

Process finished with the exit code 0

The source property requires a standalone ARM resource id rather than the buildin model and we could not create self trained model for every subscription used for acctests. Therefore I didn't to add source to acctests for now. But I think this is a valid property for any user who owns a self trained model and we should include this property in azurerm_cognitive_deployment.

@liuwuliuyun liuwuliuyun changed the title azure_cognitive_deployment - Support source property and change version property to optional azure_cognitive_deployment - Update model block Dec 18, 2023
@liuwuliuyun liuwuliuyun marked this pull request as ready for review December 22, 2023 09:04
@@ -98,9 +100,15 @@ func (r CognitiveDeploymentResource) Arguments() map[string]*pluginsdk.Schema {
ValidateFunc: validation.StringIsNotEmpty,
},

"source": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as this is a resource id wouldn't it make more sense to have it be

Suggested change
"source": {
"source_resource_id": {

@@ -66,7 +66,9 @@ A `model` block supports the following:

* `name` - (Required) The name of the Cognitive Services Account Deployment model. Changing this forces a new resource to be created.

* `version` - (Required) The version of Cognitive Services Account Deployment model.
* `source` - (Optional) Deployment model source ARM resource ID.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is a valid resource here? could we expand on the docs a bit more

@@ -142,21 +142,21 @@ resource "azurerm_cognitive_account" "test" {
func (r CognitiveDeploymentTestResource) basic(data acceptance.TestData) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a test for the new property?

@liuwuliuyun liuwuliuyun changed the title azure_cognitive_deployment - Update model block azure_cognitive_deployment - Changes model.version property from Required to Optional. Jan 9, 2024
@liuwuliuyun
Copy link
Contributor Author

@katbyte , Thanks for the comment. I removed the model.source property from this PR since we are not able to test this property for now. The reason is that we dont have a self-trained model in our subscription therefore we dont have a valid ARM resource id for source property to test.

@liuwuliuyun
Copy link
Contributor Author

Latest test evidence:

GOROOT=C:\Program Files\Go #gosetup
GOPATH=C:\Users\yunliu1\go #gosetup
"C:\Program Files\Go\bin\go.exe" test -c -o C:\Users\yunliu1\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___TestAccCognitiveDeployment_basic_in_github_com_hashicorp_terraform_provider_azurerm_internal_services_cognitive__1_.test.exe github.com/hashicorp/terraform-provider-azurerm/internal/services/cognitive #gosetup
"C:\Program Files\Go\bin\go.exe" tool test2json -t C:\Users\yunliu1\AppData\Local\JetBrains\GoLand2023.3\tmp\GoLand\___TestAccCognitiveDeployment_basic_in_github_com_hashicorp_terraform_provider_azurerm_internal_services_cognitive__1_.test.exe -test.v -test.paniconexit0 -test.run ^\QTestAccCognitiveDeployment_basic\E$ #gosetup
=== RUN   TestAccCognitiveDeployment_basic

--- PASS: TestAccCognitiveDeployment_basic (200.76s)
PASS


Process finished with the exit code 0

@liuwuliuyun liuwuliuyun requested a review from katbyte January 9, 2024 07:13
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 @liuwuliuyun. Could you fix up the minor comment on the docs? Then this should be good to go.

website/docs/r/cognitive_deployment.html.markdown Outdated Show resolved Hide resolved
Co-authored-by: stephybun <steph@hashicorp.com>
@liuwuliuyun
Copy link
Contributor Author

Hi @stephybun, thanks for the review. I have updated the doc as you suggested.

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 @liuwuliuyun LGTM 👍

@stephybun stephybun merged commit 04c253b into hashicorp:main Jan 23, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.89.0 milestone Jan 23, 2024
Copy link

This functionality has been released in v3.89.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!

dduportal added a commit to jenkins-infra/azure that referenced this pull request Jan 25, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.88.0&#34; to &#34;3.89.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.89.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.89.0&#xA;FEATURES:&#xA;&#xA;*
New Data Source: `azurerm_data_factory_trigger_schedule`
([#24572](hashicorp/terraform-provider-azurerm#24572
New Data Source: `azurerm_data_factory_trigger_schedules`
([#24572](hashicorp/terraform-provider-azurerm#24572
New Data Source: `azurerm_ip_groups`
([#24540](hashicorp/terraform-provider-azurerm#24540
New Data Source: `azurerm_nginx_certificate`
([#24577](hashicorp/terraform-provider-azurerm#24577
New Resource: `azurerm_chaos_studio_target`
([#24580](hashicorp/terraform-provider-azurerm#24580
New Resource: `azurerm_elastic_san_volume_group`
([#24166](hashicorp/terraform-provider-azurerm#24166
New Resource: `azurerm_netapp_account_encryption`
([#23733](hashicorp/terraform-provider-azurerm#23733
New Resource: `azurerm_redhat_openshift_cluster`
([#24375](https://github.com/hashicorp/terraform-provider-azurerm/issues/24375))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.66.1` of
`github.com/hashicorp/go-azure-helpers`
([#24561](hashicorp/terraform-provider-azurerm#24561
dependencies: updating to `v0.20240124.1115501` of
`github.com/hashicorp/go-azure-sdk`
([#24619](hashicorp/terraform-provider-azurerm#24619
`bot`: updating to API Version `2021-05-01-preview`
([#24555](hashicorp/terraform-provider-azurerm#24555
`containerservice`: the SDK Clients now support logging
([#24564](hashicorp/terraform-provider-azurerm#24564
`cosmosdb`: updating to API Version `2023-04-15`
([#24541](hashicorp/terraform-provider-azurerm#24541
`loadtestservice`: updating to use the base layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest` (and support
logging)
([#24578](hashicorp/terraform-provider-azurerm#24578
`managedidentity`: updating to use the base layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest` (and support
logging)
([#24578](hashicorp/terraform-provider-azurerm#24578
`azurerm_api_management_api` - change the `id` format so specific
`revision`s can be managed by Terraform
([#23031](hashicorp/terraform-provider-azurerm#23031
`azurerm_data_protection_backup_vault` - the `redundancy` propety can
now be set to `ZoneRedundant`
([#24556](hashicorp/terraform-provider-azurerm#24556
`azurerm_data_factory_integration_runtime_azure_ssis` - support for the
`credential_name` property
([#24458](hashicorp/terraform-provider-azurerm#24458
`azurerm_orchestrated_virtual_machine_scale_set` - support
`2022-datacenter-azure-edition-hotpatch` and
`2022-datacenter-azure-edition-hotpatch-smalldisk` hotpatching images
([#23500](hashicorp/terraform-provider-azurerm#23500
`azurerm_stream_analytics_job` - support for the `sku_name` property
([#24554](https://github.com/hashicorp/terraform-provider-azurerm/issues/24554))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_app_service` - parsing the API
Response for `app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
Data Source: `azurerm_function_app` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_app_configuration_key` - the value for the `value` property can
now be removed/emptied
([#24582](https://github.com/hashicorp/terraform-provider-azurerm/issues/24582))&#xA;&#xA;*
`azurerm_app_service` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_app_service_plan` - fix casing in `serverFarms` due to ID
update
([#24562](hashicorp/terraform-provider-azurerm#24562
`azurerm_app_service_slot` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_automation_schedule` - only one `monthly_occurence` block can
now be specified
([#24614](hashicorp/terraform-provider-azurerm#24614
`azurerm_cognitive_deployment` - the `model.version` property is no
longer required
([#24264](hashicorp/terraform-provider-azurerm#24264
`azurerm_container_app` - multiple `custom_scale_rule` can not be
updated
([#24509](hashicorp/terraform-provider-azurerm#24509
`azurerm_container_registry_task_schedule_run_now` - prevent issue where
the incorrect scheduled run in tracked if there have been multiple
([#24592](hashicorp/terraform-provider-azurerm#24592
`azurerm_function_app` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_function_app_slot` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_logic_app_standard` - now will parse the app service ID
insensitively
([#24562](hashicorp/terraform-provider-azurerm#24562
`azurerm_logic_app_workflow` - the `workflow_parameters` will now
correctly handle information specified by `$connections`
([#24141](hashicorp/terraform-provider-azurerm#24141
`azurerm_mssql_managed_instance_security_alert_policy` - can not update
empty storage attributes
([#24553](hashicorp/terraform-provider-azurerm#24553
`azurerm_network_interface` - the `ip_configuration` properties are no
longer added to a Load Balancer Backend if one of those
`ip_configurations` is associated with a backend
([#24470](https://github.com/hashicorp/terraform-provider-azurerm/issues/24470))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/1052/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
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 Apr 27, 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.

Azure Cognitive Deployment Resource Requires Version Field Incorrectly
3 participants