d/azurerm_builtin_role_definition - loading available role definitions from Azure

[jansepke]

and fix a small typo.

should we remove the validation list so we do not need to add new roles?

[paultyng]

Are the API names the same as the ones already listed? In all cases? We may need some mapping for legacy names if not. I'll have to run some acceptance testing.

[jansepke]

I made a diff check with this result:

new in the API:

• Lab Creator
• Resource Policy Contributor (Preview)
• Storage Blob Data Contributor (Preview)
• Storage Blob Data Reader (Preview)
• Storage Queue Data Contributor (Preview)
• Storage Queue Data Reader (Preview)
• Virtual Machine Contributor

removed in the API:

• Auditor
• CloudAware Collector Storage Account Keys Access
• DB Admin
• Lab Accounts User
• Network Security Operator
• VirtualMachineContributor

for the VirtualMachineContributor role we could do a mapping to Virtual Machine Contributor.
for all other removed roles: could this depend on the subscription or something user or region specific?

[paultyng]

I'll look in to this and reply back here when I track down an answer

[tombuildsstuff]

@jansepke @paultyng

for all other removed roles: could this depend on the subscription or something user or region specific?

My guess would be that's more likely to be subscription specific (given resources are enabled by Resource Provider)

Storage Queue Data Contributor (Preview)

We need to proceed carefully here, since the (Preview) suffix will be renamed/removed during the Preview cycle - and users code will stop working since Terraform won't be able to find the value in the API. It may be able to work around this by placing a (yellow) info block in the documentation (see an example below) - but we may also want to see if it's possible to return roles which are stable?

~> **NOTE:** Azure roles with the suffix `(Preview)` are not necessarily stable and may change at any time.

[jansepke]

@tombuildsstuff I added a validation that will error on preview roles

[tombuildsstuff]

hey @jansepke

Thanks for pushing those updates (and apologies for the delayed re-review!) - this mostly LGTM now, if we can add a migration path for users using the existing VirtualMachineContributor name this otherwise looks good to merge 👍

Thanks!

[tombuildsstuff]

in order to keep compatibility with folks using the VirtualMachineContributor key (e.g. so that we don't need to bump the major version number of the Provider) - can we add an if statement above this, which we can remove in a future version of Terraform? e.g.

if name == "VirtualMachineContributor" {
  name = "Virtual Machine Contributor"
}
filter := fmt.Sprintf("roleName eq '%s'", name)

[jansepke]

@tombuildsstuff I added the if statement and also removed the validation, because this will now happen on the azure API side

[tombuildsstuff]

hey @jansepke

Thanks for pushing the latest changes, I've taken a look through and this now LGTM :)

iI'll kick off the acceptance tests now..

Thanks!

[tombuildsstuff]

Tests pass:

[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!