Azure / bicep-registry-modules

Bicep registry modules
MIT License
513 stars 362 forks source link

[AVM Module Issue]: Boot Diagnostics has wrong parameter value #3713

Closed FallenHoot closed 1 day ago

FallenHoot commented 3 weeks ago

Check for previous/existing GitHub issues

Issue Type?

Bug

Module Name

avm/res/compute/virtual-machine-scale-set

(Optional) Module Version

0.4.0

Description

Unable to enable Boot Diagnostics, because parameter is configured wrong. It needs boolen, but requesting a string. After looking into it more deeply, the Microsoft best practice is to not include a storage account, but that can be optional. The learn docs state that all you need is the enabled true or false

bootDiagnostics: {
          enabled: !empty(bootDiagnosticStorageAccountName)
          storageUri: !empty(bootDiagnosticStorageAccountName)
            ? 'https://${bootDiagnosticStorageAccountName}${bootDiagnosticStorageAccountUri}'
            : null
}

Feedback: After looking into this for VMs and VMSS, I noticed this has the same issue. bootDiagnosticEnabled needs to be a param to allow the users to decide if they should use the Microsoft best practice managed storage or the optional secure standalone storage account.

Suggested fix:

@description('Optional. Enable boot diagnostics to use default managed or secure storage. Defaults to false.')
param bootDiagnosticEnabled bool = false

@description('Optional. The name of the boot diagnostic storage account. Provide this if you want to use your own storage account for security reasons instead of the recommended Microsoft Managed Storage Account.')
param bootDiagnosticStorageAccountName string?

@description('Optional. The URI of the boot diagnostic storage account.')
param bootDiagnosticStorageAccountUri string?

bootDiagnostics: {
  enabled: bootDiagnosticEnabled
  storageUri: !empty(bootDiagnosticStorageAccountName) ? 'https://${bootDiagnosticStorageAccountName}${bootDiagnosticStorageAccountUri}' : null
}

Happy to create a PR if needed.

(Optional) Correlation Id

No response

microsoft-github-policy-service[bot] commented 3 weeks ago

[!IMPORTANT] The "Needs: Triage :mag:" label must be removed once the triage process is complete!

[!TIP] For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation.

avm-team-linter[bot] commented 3 weeks ago

@FallenHoot, thanks for submitting this issue for the avm/res/compute/virtual-machine-scale-set module!

[!IMPORTANT] A member of the @Azure/avm-res-compute-virtualmachinescaleset-module-owners-bicep or @Azure/avm-res-compute-virtualmachinescaleset-module-contributors-bicep team will review it soon!

rahalan commented 2 weeks ago

@FallenHoot , thank you. Please submit a PR!