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

VM/VMSS: support for StandardSSD_LRS managed disk type #1901

Conversation

ira-bryndzei
Copy link
Contributor

@ira-bryndzei ira-bryndzei commented Sep 9, 2018

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)

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.

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{
{
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@ira-bryndzei ira-bryndzei Sep 11, 2018

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ira-bryndzei,

No, currently you need a SP to run them.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great! thanks

@ghost ghost added size/S and removed size/M labels Sep 12, 2018
@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review September 12, 2018 12:21

updates have been pushed

@ghost ghost added size/L and removed size/S labels Sep 12, 2018
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a 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 👍

@tombuildsstuff tombuildsstuff changed the title add support for StandardSSDLRS disk type VM/VMSS: support for StandardSSDLRS disk type Sep 12, 2018
@tombuildsstuff tombuildsstuff changed the title VM/VMSS: support for StandardSSDLRS disk type VM/VMSS: support for StandardSSD_LRS managed disk type Sep 12, 2018
@tombuildsstuff tombuildsstuff force-pushed the support_StorageAccountTypesStandardSSDLRS branch from c75c08d to bb4792f Compare September 12, 2018 13:13
@ghost ghost added the size/L label Sep 12, 2018
@tombuildsstuff
Copy link
Contributor

Ignoring a temporary transitory issue the tests pass:

screenshot 2018-09-12 at 16 41 09

@tombuildsstuff tombuildsstuff merged commit 3373882 into hashicorp:master Sep 12, 2018
tombuildsstuff added a commit that referenced this pull request Sep 12, 2018
@DYNSOL
Copy link

DYNSOL commented Sep 13, 2018

When can we expect this to be available to use?

@ghost
Copy link

ghost commented Mar 6, 2019

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
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.

Standard SSD Managed Disk Type option not available to set in terraform
4 participants