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

features update feature flag roll_instances_when_required for virtual_machine_scale_set to be Optional #22976

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

myc2h6o
Copy link
Contributor

@myc2h6o myc2h6o commented Aug 16, 2023

Fixing the schema according to document

* `roll_instances_when_required` - (Optional) Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources automatically roll the instances in the Scale Set when Required (for example when updating the Sku/Image). Defaults to `true`.

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 @myc2h6o - Thanks for this.

I feel the docs need correcting there, not the schema changing to match? The schema change would represent a breaking behavioural change, whereas the docs would be a correction the documented behaviour.

Thanks

@myc2h6o myc2h6o force-pushed the vmss_roll_instance branch from 9ae4ba3 to 670148e Compare August 16, 2023 08:51
@myc2h6o myc2h6o changed the title features fix schema of feature flag roll_instances_when_required for virtual_machine_scale_set features fix documentof feature flag roll_instances_when_required for virtual_machine_scale_set Aug 16, 2023
@myc2h6o myc2h6o changed the title features fix documentof feature flag roll_instances_when_required for virtual_machine_scale_set features fix document of feature flag roll_instances_when_required for virtual_machine_scale_set Aug 16, 2023
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Aug 16, 2023

sure, have updated the document instead of the schema.

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.

Thanks for the change @myc2h6o - Just one additional comment and then I think we're good to go.

@@ -213,6 +213,6 @@ The `virtual_machine_scale_set` block supports the following:

~> **Note:** Support for Force Delete is in an opt-in Preview.

* `roll_instances_when_required` - (Optional) Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources automatically roll the instances in the Scale Set when Required (for example when updating the Sku/Image). Defaults to `true`.
* `roll_instances_when_required` - (Required) Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources automatically roll the instances in the Scale Set when Required (for example when updating the Sku/Image).
Copy link
Member

Choose a reason for hiding this comment

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

Can we include the default value here to be consistent?

Suggested change
* `roll_instances_when_required` - (Required) Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources automatically roll the instances in the Scale Set when Required (for example when updating the Sku/Image).
* `roll_instances_when_required` - (Required) Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources automatically roll the instances in the Scale Set when Required (for example when updating the Sku/Image). Defaults to `false`.

(since bools are false if not explicitly set to true)

Copy link
Contributor Author

@myc2h6o myc2h6o Aug 17, 2023

Choose a reason for hiding this comment

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

I've taken another thought when seeing this default. Usually, a Required property is not supposed to have a default value. And the default value of this is already set in features/defaults.go:

RollInstancesWhenRequired: true,

So I think we should still fix the schema instead. By changing a Required prop to Optional, user won't be seeing a breaking change. No matter the old config explicitly set this feature flag or not, it will work as the same with this change.

@@ -1136,7 +1136,6 @@ provider "azurerm" {
features {
virtual_machine_scale_set {
force_delete = true
roll_instances_when_required = true
Copy link
Contributor Author

@myc2h6o myc2h6o Aug 17, 2023

Choose a reason for hiding this comment

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

removing this as it defaults to true with this change, this acc test is testing the functionality of force_delete

Copy link
Contributor

@manicminer manicminer 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, I think on balance it should be ok to change this feature flag from required to optional - it should not affect any existing configurations and the behavior should be the same in the absence of this block.

@manicminer
Copy link
Contributor

Test results

Screenshot 2023-10-12 at 10 27 55

manicminer added a commit that referenced this pull request Oct 12, 2023
@myc2h6o
Copy link
Contributor Author

myc2h6o commented Oct 14, 2023

@manicminer thanks for comfirming that. It seems like the changelog has been updated to include this change but it's forgot to be merged.

@myc2h6o myc2h6o changed the title features fix document of feature flag roll_instances_when_required for virtual_machine_scale_set features update feature flag roll_instances_when_required for virtual_machine_scale_set to be Optional Oct 14, 2023
@manicminer
Copy link
Contributor

@myc2h6o Sorry about that, not sure how that happened. I'll fix the changelog and merge it for next release.

manicminer added a commit that referenced this pull request Oct 15, 2023
@manicminer manicminer merged commit 07fe412 into hashicorp:main Oct 15, 2023
1 check passed
@manicminer manicminer added this to the v3.77.0 milestone Oct 15, 2023
@myc2h6o myc2h6o deleted the vmss_roll_instance branch October 16, 2023 01:51
dduportal added a commit to jenkins-infra/azure that referenced this pull request Oct 16, 2023
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.74.0&#34; to
&#34;3.75.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.75.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.75.0&#xA;FEATURES:&#xA;&#xA;*
New Resource: `azurerm_application_load_balancer`
([#22517](hashicorp/terraform-provider-azurerm#22517
New Resource: `azurerm_resource_management_private_link`
([#23098](https://github.com/hashicorp/terraform-provider-azurerm/issues/23098))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: `firewall` migrated to `hashicorp/go-azure-sdk`
([#22863](hashicorp/terraform-provider-azurerm#22863
`azurerm_bot_service_azure_bot` - add support for the `icon_url`
property
([#23114](hashicorp/terraform-provider-azurerm#23114
`azurerm_cognitive_deployment` - `capacity` property is now updateable
([#23251](hashicorp/terraform-provider-azurerm#23251
`azurerm_container_group` - added support for
`key_vault_user_identity_id`
([#23332](hashicorp/terraform-provider-azurerm#23332
`azurerm_data_factory` - added support for the `publish_enabled`
property
([#2334](hashicorp/terraform-provider-azurerm#2334
`azurerm_firewall_policy_rule_collection_group` - add support for the
`description` property
([#23354](hashicorp/terraform-provider-azurerm#23354
`azurerm_kubernetes_cluster` - `network_profile.network_policy` can be
migrated to `cilium`
([#23342](hashicorp/terraform-provider-azurerm#23342
`azurerm_log_analytics_workspace` - add support for the
`data_collection_rule_id` property
([#23347](hashicorp/terraform-provider-azurerm#23347
`azurerm_mysql_flexible_server` - add support for the
`io_scaling_enabled` property
([#23329](https://github.com/hashicorp/terraform-provider-azurerm/issues/23329))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_api_management_api` - fix importing `openapi`
format content file issue
([#23348](hashicorp/terraform-provider-azurerm#23348
`azurerm_cdn_frontdoor_rule` - allow a `cache_duration` of `00:00:00`
([#23384](hashicorp/terraform-provider-azurerm#23384
`azurerm_cosmosdb_cassandra_datacenter` - `sku_name` is now updatable
([#23419](hashicorp/terraform-provider-azurerm#23419
`azurerm_key_vault_certificate` - fix a bug that prevented soft-deleted
certificates from being recovered
([#23204](hashicorp/terraform-provider-azurerm#23204
`azurerm_log_analytics_solution` - fix create and update lifecycle of
resource by splitting methods
([#23333](hashicorp/terraform-provider-azurerm#23333
`azurerm_management_group_subscription_association` - mark resource as
gone correctly if not found when retrieving
([#23335](hashicorp/terraform-provider-azurerm#23335
`azurerm_management_lock` - add polling after create and delete to check
for RP propagation
([#23345](hashicorp/terraform-provider-azurerm#23345
`azurerm_monitor_diagnostic_setting` - added validation to ensure at
least one of `category` or `category_group` is supplied
([#23308](hashicorp/terraform-provider-azurerm#23308
`azurerm_palo_alto_local_rulestack_prefix_list` - fix rulestack not
being committed on delete
([#23362](hashicorp/terraform-provider-azurerm#23362
`azurerm_palo_alto_local_rulestack_fqdn_list` - fix rulestack not being
committed on delete
([#23362](hashicorp/terraform-provider-azurerm#23362
`security_center_subscription_pricing_resource` - disabled extensions
logic now works as expected
([#22997](https://github.com/hashicorp/terraform-provider-azurerm/issues/22997))&#xA;&#xA;&#xA;&#xA;</pre>
            </details>
            <details>
                <summary>3.76.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.76.0&#xA;FEATURES:&#xA;&#xA;*
New Resource: `azurerm_security_center_storage_defender`
([#23242](hashicorp/terraform-provider-azurerm#23242
New Resource:
`azurerm_spring_cloud_application_insights_application_performance_monitoring`
([#23107](https://github.com/hashicorp/terraform-provider-azurerm/issues/23107))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
provider: updating to build using Go `1.21.3`
([#23514](hashicorp/terraform-provider-azurerm#23514
provider: the `roll_instances_when_required` provider feature in the
`virtual_machine_scale_set` block is now optional
([#22976](hashicorp/terraform-provider-azurerm#22976
dependencies: updating to `v0.20231012.1141427` of
`github.com/hashicorp/go-azure-sdk`
([#23534](hashicorp/terraform-provider-azurerm#23534
Data Source: `azurerm_application_gateway` - support for
`backend_http_settings`, `global`, `gateway_ip_configuration` and
additional attributes
([#23318](hashicorp/terraform-provider-azurerm#23318
Data Source: `azurerm_network_service_tags` - export the `name`
attribute
([#23382](hashicorp/terraform-provider-azurerm#23382
`azurerm_cosmosdb_postgresql_cluster` - add support for `sql_version` of
`16` and `citus_version` of `12.1`
([#23476](hashicorp/terraform-provider-azurerm#23476
`azurerm_palo_alto_local_rulestack` - correctly normalize the `location`
property
([#23483](hashicorp/terraform-provider-azurerm#23483
`azurerm_static_site` - add support for `app_settings`
([#23421](https://github.com/hashicorp/terraform-provider-azurerm/issues/23421))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_automation_schedule` - fix a bug when
updating `start_time`
([#23494](hashicorp/terraform-provider-azurerm#23494
`azurerm_eventhub` - remove ForceNew and check `partition_count` is not
decreased
([#23499](hashicorp/terraform-provider-azurerm#23499
`azurerm_managed_lustre_file_system` - update validation for
`storage_capacity_in_tb` according to `sku_name` in use
([#23428](hashicorp/terraform-provider-azurerm#23428
`azurerm_virtual_machine` - fix a crash when the API response for the
`os_profile` block contains nil properties
([#23535](https://github.com/hashicorp/terraform-provider-azurerm/issues/23535))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </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>
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Oct 20, 2023
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.76.0&#34; to
&#34;3.77.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.77.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.77.0&#xA;FEATURES:&#xA;&#xA;*
New Resources: `azurerm_application_load_balancer_frontend`
([#23411](hashicorp/terraform-provider-azurerm#23411
New Resources: `azurerm_dev_center`
([#23538](hashicorp/terraform-provider-azurerm#23538
New Resources: `azurerm_dev_center_project`
([#23538](https://github.com/hashicorp/terraform-provider-azurerm/issues/23538))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.62.0` of
`github.com/hashicorp/go-azure-helpers`
([#23581](hashicorp/terraform-provider-azurerm#23581
dependencies: updating Kusto SDK from `2023-05-02` to `2023-08-15`
([#23598](hashicorp/terraform-provider-azurerm#23598
dependencies: updating nginx from `2022-08-01` to `2023-04-01`
([#23583](hashicorp/terraform-provider-azurerm#23583
`netapp`: updating to use API Version `2023-05-01`
([#23576](hashicorp/terraform-provider-azurerm#23576
`springcloud`: updating to use API Version `2023-09-01-preview`
([#23544](hashicorp/terraform-provider-azurerm#23544
`storage`: updating to use API Version `2023-01-01`
([#23543](hashicorp/terraform-provider-azurerm#23543
`internal/sdk`: fixing an issue where struct fields containing
`removedInNextMajorVersion` wouldn&#39;t be decoded correctly
([#23564](hashicorp/terraform-provider-azurerm#23564
`internal/sdk`: struct tag parsing is now handled consistently during
both encoding and decoding
([#23568](hashicorp/terraform-provider-azurerm#23568
provider: the `roll_instances_when_required` provider feature in the
`virtual_machine_scale_set` block is now optional
([#22976](hashicorp/terraform-provider-azurerm#22976
Data Source: `azurerm_automation_account`: refactoring the remaining
usage of `Azure/azure-sdk-for-go` to use `hashicorp/go-azure-sdk`
([#23555](hashicorp/terraform-provider-azurerm#23555
`azurerm_automation_account`: refactoring the remaining usage of
`Azure/azure-sdk-for-go` to use `hashicorp/go-azure-sdk`
([#23555](hashicorp/terraform-provider-azurerm#23555
`azurerm_resource_deployment_script_azure_cli` - improve validation for
the `version` property to support newer versions
([#23370](hashicorp/terraform-provider-azurerm#23370
`azurerm_resource_deployment_script_azure_power_shell` - improve
validation for the `version` property to support newer versions
([#23370](hashicorp/terraform-provider-azurerm#23370
`azurerm_nginx_deployment` - support for the `capacity` and `email`
properties
([#23596](https://github.com/hashicorp/terraform-provider-azurerm/issues/23596))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_virtual_hub_connection` - export
the `inbound_route_map_id`, `outbound_route_map_id`, and
`static_vnet_local_route_override_criteria` attributes in the `routing`
block, and fix a bug where these attributes could not be set
([#23491](hashicorp/terraform-provider-azurerm#23491
`azurerm_cdn_frontdoor_rule` - the `url_filename_condition` properties
`match_values` is now optional if `operator` is set to `Any`
([#23541](hashicorp/terraform-provider-azurerm#23541
`azurerm_shared_image_gallery` - added the `Private` and `Groups`
options for the `sharing.permission` property
([#23570](hashicorp/terraform-provider-azurerm#23570
`azurerm_redis_cache` - fixed incorrect ssl values for
`redis_primary_connection_string` and `secondary_connection_string`
([#23575](hashicorp/terraform-provider-azurerm#23575
`azurerm_monitor_activity_log_alert` - the `recommend_category` property
now can be set to `HighAvailability`
([#23605](hashicorp/terraform-provider-azurerm#23605
`azurerm_recovery_services_vault` - the `encryption` property can now be
used with the `cross_region_restore_enabled` property
([#23618](hashicorp/terraform-provider-azurerm#23618
`azurerm_storage_account_customer_managed_key` - prevent a panic when
the keyvault id is empty
([#23599](https://github.com/hashicorp/terraform-provider-azurerm/issues/23599))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </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>
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 May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants