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

Update azurerm_linux|windows_virtual_machine_scale_set - Support platform_fault_domain_count, disk_iops_read_write and disk_mbps_read_write #9262

Merged
merged 14 commits into from Nov 19, 2020

Conversation

ArcturusZhang
Copy link
Contributor

No description provided.

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 @ArcturusZhang - have a could questions i've left inline

@@ -264,6 +266,10 @@ A `data_disk` block supports the following:

~> **NOTE:** Disk Encryption Sets are in Public Preview in a limited set of regions

* `disk_iops_read_write` - (Optional) Specifies the Read-Write IOPS for this Data Disk. Only settable for UltraSSD disks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this make more sense as

Suggested change
* `disk_iops_read_write` - (Optional) Specifies the Read-Write IOPS for this Data Disk. Only settable for UltraSSD disks.
* `ultra_ssd_disk_iops_read_write` - (Optional) Specifies the Read-Write IOPS for this Data Disk. Only settable for UltraSSD disks.

to make it clear it only applies to ultra ssds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I would love to do this change, but these two attributes already exist in the azurerm_managed_disk resource, would this cause any incosistency?

Copy link
Member

Choose a reason for hiding this comment

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

We should leave these consistent with elsewhere in the provider, even though they only have meaning in one circumstance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should eventually change both to ultra_ssd then - i'll just add a comment

@jackofallops jackofallops added enhancement service/vmss Virtual Machine Scale Sets labels Nov 18, 2020
@ArcturusZhang
Copy link
Contributor Author

Hi @katbyte I have resolved the comments, please have a look, thanks

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 @ArcturusZhang - we should eventually change these properties to have ultra_ssd in them but that can be a 3.0 task - LGTM 👍

@katbyte katbyte merged commit 8c3357c into hashicorp:master Nov 19, 2020
@katbyte katbyte added this to the v2.37.0 milestone Nov 19, 2020
katbyte added a commit that referenced this pull request Nov 19, 2020
@ArcturusZhang ArcturusZhang deleted the vmss-enhancement branch November 19, 2020 05:26
@ghost
Copy link

ghost commented Nov 20, 2020

This has been released in version 2.37.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.37.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Dec 19, 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 as resolved and limited conversation to collaborators Dec 19, 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.

None yet

3 participants