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_linux_web_app_slot - fix ignored worker_count argument in site_config block #24515

Merged
merged 2 commits into from
Feb 9, 2024
Merged

azurerm_linux_web_app_slot - fix ignored worker_count argument in site_config block #24515

merged 2 commits into from
Feb 9, 2024

Conversation

dkanbier
Copy link
Contributor

@dkanbier dkanbier commented Jan 16, 2024

Currently the worker_count argument in the site_config block on azurerm_linux_web_app_slot seems to get ignored when the resource is created:

resource "azurerm_linux_web_app_slot" "example" {
  name           = "example-slot"
  app_service_id = azurerm_linux_web_app.example.id

  site_config {
    worker_count = 2
  }
}

Running terraform apply on this code will show no issues and finish successfully. The resulting slot will have a worker_count of 1 however, which you can check via the JSON View of the slot in the Azure Portal.

Re-running a terraform plan directly after the terraform apply will show differences:

      ~ site_config {
          ~ worker_count                            = 1 -> 2

While debugging I discovered the NumberOfWorkers argument is simply missing from the AzureRM API call to Azure when the azurerm_linux_web_app_slot resource gets created.

I think this is because it is never added to the expanded arguments.

The goal of this PR is to remedy this, by adding the NumberOfWorkers to the expanded arguments. This causes the resulting slot to have the correct number of workers.

Unit tests:

=== RUN   TestAccLinuxWebAppSlot_complete
=== PAUSE TestAccLinuxWebAppSlot_complete
=== RUN   TestAccLinuxWebAppSlot_completeUpdate
=== PAUSE TestAccLinuxWebAppSlot_completeUpdate
=== CONT  TestAccLinuxWebAppSlot_complete
=== CONT  TestAccLinuxWebAppSlot_completeUpdate
--- PASS: TestAccLinuxWebAppSlot_complete (152.51s)
--- PASS: TestAccLinuxWebAppSlot_completeUpdate (221.91s)
PASS

fixes #24678

@dkanbier dkanbier changed the title azurerm_linux_web_app_slot - fix ignored worker_count argument in site_config block azurerm_linux_web_app_slot - fix ignored worker_count argument in site_config block Jan 18, 2024
@dkanbier
Copy link
Contributor Author

dkanbier commented Jan 29, 2024

Added issue #24678 with detailed debug logs and steps to reproduce the issue.

@sven5
Copy link

sven5 commented Feb 1, 2024

When will this one be merged?

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @dkanbier - Thanks for this PR, and for the catch on the missing property. Due to a recent change in the SDK, this property is now an int64 type, so no longer requires casting. If you can update, we can get the tests run and hopefully get this merged when they pass.

Thanks!

@dkanbier
Copy link
Contributor Author

dkanbier commented Feb 5, 2024

Hi @dkanbier - Thanks for this PR, and for the catch on the missing property. Due to a recent change in the SDK, this property is now an int64 type, so no longer requires casting. If you can update, we can get the tests run and hopefully get this merged when they pass.

Thanks!

Hi @jackofallops , I've applied your suggested changes and tried rebuilding the module but it fails:

internal/services/appservice/helpers/web_app_slot_schema.go:670:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment
internal/services/appservice/helpers/web_app_slot_schema.go:823:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment

I think we would have to change the type of NumberOfWorkers.

If we do that I think we also need to update a lot more code to remove the int32 casting which is currently used on WorkerCount throughout the code.

Please let me know if you want to pursue this or not so I can make changes accordingly. If we do the PR will become quite a bit bigger in my mind.

@jackofallops
Copy link
Member

Hi @dkanbier - Thanks for this PR, and for the catch on the missing property. Due to a recent change in the SDK, this property is now an int64 type, so no longer requires casting. If you can update, we can get the tests run and hopefully get this merged when they pass.
Thanks!

Hi @jackofallops , I've applied your suggested changes and tried rebuilding the module but it fails:

internal/services/appservice/helpers/web_app_slot_schema.go:670:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment
internal/services/appservice/helpers/web_app_slot_schema.go:823:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment

I think we would have to change the type of NumberOfWorkers.

If we do that I think we also need to update a lot more code to remove the int32 casting which is currently used on WorkerCount throughout the code.

Please let me know if you want to pursue this or not so I can make changes accordingly. If we do the PR will become quite a bit bigger in my mind.

Hi @dkanbier - To pick up the change in main to build successfully you'd need to update your fork's main branch and rebase your PR branch on top of that. (you should then end up with: https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/appservice/helpers/web_app_slot_schema.go#L46) in your local.) It's not absolutely necessary for the PR however, since the merge branch appears to be handling it just fine. Now you've made the change, I can run workflows and the testing on our CI server for this.

@dkanbier
Copy link
Contributor Author

dkanbier commented Feb 5, 2024

Hi @dkanbier - Thanks for this PR, and for the catch on the missing property. Due to a recent change in the SDK, this property is now an int64 type, so no longer requires casting. If you can update, we can get the tests run and hopefully get this merged when they pass.
Thanks!

Hi @jackofallops , I've applied your suggested changes and tried rebuilding the module but it fails:

internal/services/appservice/helpers/web_app_slot_schema.go:670:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment
internal/services/appservice/helpers/web_app_slot_schema.go:823:30: cannot use pointer.To(s.WorkerCount) (value of type *int) as *int32 value in assignment

I think we would have to change the type of NumberOfWorkers.
If we do that I think we also need to update a lot more code to remove the int32 casting which is currently used on WorkerCount throughout the code.
Please let me know if you want to pursue this or not so I can make changes accordingly. If we do the PR will become quite a bit bigger in my mind.

Hi @dkanbier - To pick up the change in main to build successfully you'd need to update your fork's main branch and rebase your PR branch on top of that. (you should then end up with: https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/appservice/helpers/web_app_slot_schema.go#L46) in your local.) It's not absolutely necessary for the PR however, since the merge branch appears to be handling it just fine. Now you've made the change, I can run workflows and the testing on our CI server for this.

@jackofallops Ah yes you are right, I did the rebase but forgot I was on my fork. Sorry!

@jackofallops
Copy link
Member

@jackofallops Ah yes you are right, I did the rebase but forgot I was on my fork. Sorry!

No harm done, tests are running now, one of the team will get this merged assuming everything is ✅

Thanks again for the contribution!

@dkanbier
Copy link
Contributor Author

dkanbier commented Feb 7, 2024

Hi @jackofallops , any news on the tests maybe?

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this @dkanbier

@mbfrahry mbfrahry added this to the v3.92.0 milestone Feb 9, 2024
@mbfrahry mbfrahry merged commit 838a854 into hashicorp:main Feb 9, 2024
32 checks passed
mbfrahry added a commit that referenced this pull request Feb 9, 2024
@dkanbier dkanbier deleted the bugfix/linux_web_app_slot branch February 10, 2024 16:13
dduportal added a commit to jenkins-infra/azure that referenced this pull request Feb 19, 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.91.0&#34; to &#34;3.92.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.92.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.92.0&#xA;FEATURES:&#xA;&#xA;*
**New Data Source**: `azurerm_virtual_desktop_application_group`
([#24771](https://github.com/hashicorp/terraform-provider-azurerm/issues/24771))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
provider: support for the feature flag
`postgresql_flexible_server.restart_server_on_configuration_value_change
property`
([#23811](hashicorp/terraform-provider-azurerm#23811
dependencies: updating to v0.20240214.1142753 of
`github.com/hashicorp/go-azure-sdk`
([#24889](hashicorp/terraform-provider-azurerm#24889
`automation`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24858](hashicorp/terraform-provider-azurerm#24858
`maintenance`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24819](hashicorp/terraform-provider-azurerm#24819
`containerapps`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24862](hashicorp/terraform-provider-azurerm#24862
`containerservices`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24872](hashicorp/terraform-provider-azurerm#24872
`timeseriesinsights`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24889](hashicorp/terraform-provider-azurerm#24889
`azurerm_container_app_environment`: support for the
`infrastructure_resource_group_name` property
([#24361](hashicorp/terraform-provider-azurerm#24361
`azurerm_cost_anomaly_alert` - support for the `subscription_id`
property
([#24258](hashicorp/terraform-provider-azurerm#24258
`azurerm_cosmosdb_account` - add default values for the
`consistency_policy` code block
([#24830](hashicorp/terraform-provider-azurerm#24830
`azurerm_dashboard_grafana` - support for the `smtp` block
([#24717](hashicorp/terraform-provider-azurerm#24717
`azurerm_key_vault_certificates` - support for the `tags` property
([#24857](hashicorp/terraform-provider-azurerm#24857
`azurerm_key_vault_secrets` - support for the `tags` property
([#24857](hashicorp/terraform-provider-azurerm#24857
`azurerm_orchestrated_virtual_machine_scale_set` - support for the
`additional_unattend_content` block
([#24292](hashicorp/terraform-provider-azurerm#24292
`azurerm_virtual_desktop_host_pool` - support for the `vm_template`
property
([#24369](https://github.com/hashicorp/terraform-provider-azurerm/issues/24369))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_container_app_environment`: avoid unwanted
changes when updating and using `log_analytics_workspace_id`
([#24303](hashicorp/terraform-provider-azurerm#24303
`azurerm_cosmosdb_account` - fixed regression in the `backup` code block
([#24830](hashicorp/terraform-provider-azurerm#24830
`azurerm_data_factory` - allow the `git_url` property to be blank/empty
([#24879](hashicorp/terraform-provider-azurerm#24879
`azurerm_linux_web_app_slot` - the `worker_count` property now works
correctly in the `site_config` block
([#24515](hashicorp/terraform-provider-azurerm#24515
`azurerm_linux_web_app` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_linux_web_app_slot` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_private_endpoint` - fixing an issue where updating the Private
Endpoint would remove any Application Security Group Association
([#24846](hashicorp/terraform-provider-azurerm#24846
`azurerm_search_service` - fixed the update function to adjust for
changed API behaviour
([#24837](hashicorp/terraform-provider-azurerm#24837
`azurerm_search_service` - fixed the update function to adjust for
changed API behaviour
([#24903](hashicorp/terraform-provider-azurerm#24903
`azurerm_windows_web_app` - support `off` for the `file_system_level`
property
([#24877](hashicorp/terraform-provider-azurerm#24877
`azurerm_windows_web_app_slot` - support `off` for the
`file_system_level` property
([#24877](https://github.com/hashicorp/terraform-provider-azurerm/issues/24877))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/updatecli/job/azure/job/main/3/">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) <[email protected]>
Co-authored-by: Damien Duportal <[email protected]>
rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 29, 2024
rizkybiz pushed a commit to rizkybiz/terraform-provider-azurerm that referenced this pull request Feb 29, 2024
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 24, 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.

azurerm_linux_web_app_slot ignores worker_count parameter in site_config block
4 participants