CompositionalIT / farmer

Repeatable Azure deployments with ARM templates - made easy!
https://compositionalit.github.io/farmer
MIT License
527 stars 159 forks source link

Fix diagnostics storage account reference for existing storage account #945

Closed ninjarobot closed 2 years ago

ninjarobot commented 2 years ago

This PR closes #939

The changes in this PR are as follows:

I have read the contributing guidelines and have completed the following:

If I haven't completed any of the tasks above, I include the reasons why here:

Bugfix only, no doc to update. Uses existing unit test.

ninjarobot commented 2 years ago

@Vishal7Sharma thank you for reporting #939. Can you please review this fix? I see the issue was that the [reference] depended on the resource being deployed in the same template, which isn't the case when deploying a VM using an existing storage account for boot diagnostics.

Vishal7Sharma commented 2 years ago

Hi @ninjarobot thanks for looking into this, yes, the above PR solves the issue and generates the correct arm template.

Vishal7Sharma commented 2 years ago

Hi @ninjarobot, a minor issue though, now if we pass the parameter "diagnostics_support" without existing storage account, still it generates the arm expression like below. So, I think we have to handle default storage account scenario differently.

"diagnosticsProfile": {
          "bootDiagnostics": {
            "enabled": true,
            "storageUri": "[reference(resourceId('Microsoft.Storage/storageAccounts', 'abcdstorage'), '2019-06-01').primaryEndpoints.blob]"
          }
        },
ninjarobot commented 2 years ago

Thanks @Vishal7Sharma - I must be missing it - what is wrong with that expression?

Vishal7Sharma commented 2 years ago

Hi @ninjarobot, my bad I thought arm expression has to be different for new and existing storage account. I just tested the above changes with new storage account as well and it worked. So, the above changes are correct for both use cases (existing and new). Thanks

ninjarobot commented 2 years ago

Thanks for verifying on your end as well. I'll go ahead and merge to get the fix out for you.