Support for StorageAccountType on azurerm_shared_image_version

[cwebbtw]

Work in progress fix for #4833

Have made changes to support storage account type in:

• Data source
• Resource
• Documentation
• Acceptance test for ZRS/LRS storage account type

I've made some changes to azurerm_shared_image_version to support a storage_account_type field, however I'm running into problems running the acceptance tests, which seem to be failing against origin/master. Running tests with:

make testacc TESTARGS='-run=TestAccAzureRMSharedImageVersion'

Other people seem to be having similar issues and have active PRs open, see #4453.

--- FAIL: TestAccDataSourceAzureRMSharedImageVersion_basic (912.05s)
    testing.go:569: Step 1 error: errors during apply:
        
        Error: compute.ImagesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="InvalidParameter" Message="Required parameter 'hyperVGeneration' is missing (null)." Target="hyperVGeneration"
        
          on /var/folders/v8/cmph72_n7lvbpnbp2g5ssc0m0000gn/T/tf-test596992779/main.tf line 102:
          (source code not available)
        
        
        
        Error: Code="TargetDiskBlobAlreadyExists" Message="Blob https://accsa191113120502781918.blob.core.windows.net/vhds/myosdisk1.vhd already exists. Please provide a different blob URI as target for disk 'myosdisk1'."
        
          on /var/folders/v8/cmph72_n7lvbpnbp2g5ssc0m0000gn/T/tf-test596992779/main.tf line 64:
          (source code not available)

As such, I'm not confident the changes made will support a fix for #4833. It would be good if someone can review and make suggestions based on the current inability to run tests.

Fixes #4833

[tombuildsstuff]

hey @cwebbtw

Thanks for this PR :)

Taking a look through this is looking good - if we can fix up the minor comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

[tombuildsstuff]

@cwebbtw

I've made some changes to azurerm_shared_image_version to support a storage_account_type field, however I'm running into problems running the acceptance tests, which seem to be failing against origin/master. Running tests with:

There's a PR tracking a fix for this in #4453 but unfortunately there's a bug in it we've not been able to track down just yet - at this point this PR otherwise looks good though, so I don't think that's a blocker for this PR (although we do need to dive deeper into #4453 so we can ship that fix)

[cwebbtw]

hey @cwebbtw

Thanks for this PR :)

Taking a look through this is looking good - if we can fix up the minor comments (and the tests pass) then this otherwise LGTM 👍

Thanks!

I will make the changes and will watch the tracker for when the tests pass again on master, will merge the changes to origin to my branch and verify that the tests for adding storage_account_type pass. I'm keen on waiting for the tests to pass before we risk a PR merge, as I'm sure is your intention by the test passing comment 👍

[MarcelT-NL]

Just wanted to reach out and give you a big "THANK YOU" for the quick manner in which you guys picked my enhancement request up! I will finally be able to run my terraform script again soon :)

[MarcelT-NL]

Hi @cwebbtw,

I am a little confused about the process here - is this fix included in 1.37 and if so, where can I find the ETA of that release? Any comments on this would be appreciated!

Cheers, Marcel

[tombuildsstuff]

@cwebbtw

I will make the changes and will watch the tracker for when the tests pass again on master, will merge the changes to origin to my branch and verify that the tests for adding storage_account_type pass. I'm keen on waiting for the tests to pass before we risk a PR merge, as I'm sure is your intention by the test passing comment 👍

Since these issues are unrelated I don't think we're necessarily blocked on #4453 (albeit that needs to be fixed separately) - so providing the tests pass here we should be otherwise good - I'll take a look through now 👍

[tombuildsstuff]

hey @cwebbtw

Thanks for pushing those changes - taking a look through this now LGTM 👍

[tombuildsstuff]

hey @cwebbtw

Thanks for pushing those changes - after running the tests I've noticed a couple of issues and have left some comments inline - if we can fix those up (and the tests pass) then this should otherwise be good to merge 👍

Thanks!

[tombuildsstuff]

hey @cwebbtw

Thanks for this PR - apologies for the delayed re-review here!

Taking a look through here it appears there's a few minor things needed to get this merged; namely treating the storage_account_type field as a Computed field (since it's optional, but has a default value) and removing the assertions from the tests (since the block target_region is a Set rather than a List - thus testing this is easiest via an Import test).

Since this also requires a rebase (to pick up some changes required to make the tests pass) - I hope you don't mind but since this is from your master branch we'd rather not force-push to this (in addition there's a known bug in Github around force-pushing to a master branch with branch protection where force-pushing ends up breaking the PR).

As such whilst I'd like to thank you for this contribution, I'm going to close this in favour of #5212 - which includes your commits from this PR (so you'll still get credit for the contributions) plus the necessary changes to allow us to get this merged - I hope you don't mind!

Thanks!

[ghost]

This has been released in version 1.40.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 = "~> 1.40.0"
}
# ... other configuration ...

[ghost]

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!