-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
VM/VMSS: support for StandardSSD_LRS
managed disk type
#1901
VM/VMSS: support for StandardSSD_LRS
managed disk type
#1901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ira-bryndzei,
Thank you for this contribution. One of the test is failing (TestAccAzureRMManagedDisk_update) I believe because a step was accidentally added.
Additionally, could we get some tests to verify the new storage type works?
@@ -131,6 +131,17 @@ func TestAccAzureRMManagedDisk_update(t *testing.T) { | |||
Providers: testAccProviders, | |||
CheckDestroy: testCheckAzureRMManagedDiskDestroy, | |||
Steps: []resource.TestStep{ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this test step is in error, it causes a failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katbyte thanks for your reply. I'll take a look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katbyte Is it possible to run acceptance test authenticating using the Azure CLІ with user account as I have no ability to create service principal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, currently you need a SP to run them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to give some additional information here, this test is failing because the configuration in the preConfig
is being used for multiple entries (with different storage account types) - whereas the configuration actually creates a disk of the storage account type compute.StorageAccountTypesStandardLRS
. It should be possible to add another configuration which includes the different storage account type to allow this to pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ira-bryndzei it's probably easiest to revert the additional test block in this file (but leave the updated constant/enum name) for the moment (we can then push a new test for this new disk type in a follow up commit) - what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombuildsstuff that's work for me. I'll revert that block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombuildsstuff I deleted that block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks - I've pushed a couple of commits (one to add the new values to the docs, which I'd missed before; and another to add the tests as discussed above) - I hope you don't mind!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @ira-bryndzei
Thanks for pushing those changes - this now LGTM 👍
StandardSSD_LRS
managed disk type
c75c08d
to
bb4792f
Compare
When can we expect this to be available to use? |
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! |
There is new StandardSSDLRS disk type in Azure cloud and Go Azure SDK now supports StandardSSDLRS disk type so I updated provider to use it in arm_managed_disk arm_virtual_machine and arm_virtual_machine_scale_set resources. And also Go SDK now returns StorageAccountTypes* values for storage account so I updated them too.
(fixes #1432)