search

Azure / terraform-azurerm-avm-res-authorization-roleassignment

AVM Terraform module for role assignments
https://registry.terraform.io/modules/Azure/avm-res-authorization-roleassignment
MIT License
4 stars 2 forks source link

First iteration of role assignments module #4

Closed jaredfholgate closed 5 months ago

jaredfholgate commented 7 months ago

Implementation of https://github.com/Azure/Azure-Verified-Modules/issues/311

This module includes functionality to lookup principals by various attributes apply roles at various levels.

It supports the following scopes:

It supports the follow principal types:

The E2E test validates all the different combinations of these and more.

Will add an issue to support role assignment conditions, but we have enough for iteration 1 here.

jaredfholgate commented 6 months ago

Please note there is still an outstanding issue with the new linting implementation that I am waiting on resolution of in this PR: https://github.com/Azure/tfmod-scaffold/pull/18

jchancellor-ms commented 5 months ago

@jaredfholgate - The code looks good. Consider making the following changes to be compliant with the code spec:

  1. Include examples for each variable per tfnfr1
  2. Per TFNFR15 order variable definitions in alphabetical order
  3. Per TFNFR25 and TFNFR26 set the terraform and providers to upgrade only within a major version to avoid breaking changes
  4. Per TFNFR26 order outputs alphabetically
  5. Per TFNFR32 order locals alphabetically
matt-FFFFFF commented 5 months ago

@jaredfholgate lookat the avmfix tool, which is in the container image.

It will perform all of the chores in terms of style and formatting.

jaredfholgate commented 5 months ago

@matt-FFFFFF For https://azure.github.io/Azure-Verified-Modules/specs/terraform/#id-tfnfr25---category-code-style---all-verified-modules-must-have-terraformtf-file. Is it acceptable to use ~> #.# instead of using the >= #.#, < #.#? It has the same effect, but less verbose.

jaredfholgate commented 5 months ago

@jaredfholgate - The code looks good. Consider making the following changes to be compliant with the code spec:

  1. Include examples for each variable per tfnfr1
  2. Per TFNFR15 order variable definitions in alphabetical order
  3. Per TFNFR25 and TFNFR26 set the terraform and providers to upgrade only within a major version to avoid breaking changes
  4. Per TFNFR26 order outputs alphabetically
  5. Per TFNFR32 order locals alphabetically

@jchancellor-ms I have made these fixes, thanks for spotting them. Ready to review again.

matt-FFFFFF commented 5 months ago

@matt-FFFFFF For https://azure.github.io/Azure-Verified-Modules/specs/terraform/#id-tfnfr25---category-code-style---all-verified-modules-must-have-terraformtf-file. Is it acceptable to use ~> #.# instead of using the >= #.#, < #.#? It has the same effect, but less verbose.

~> allows only patch version increments, where as the >= x.y.z, <a.b.c allows minors?

https://developer.hashicorp.com/terraform/language/expressions/version-constraints#-3

jaredfholgate commented 5 months ago

@matt-FFFFFF

        Matt White
        FTE For https://azure.github.io/Azure-Verified-Modules/specs/terraform/#id-tfnfr25---category-code-style---all-verified-modules-must-have-terraformtf-file. Is it acceptable to use `~> #.#` instead of using the `>= #.#, < #.#`? It has the same effect, but less verbose.

~> allows only patch version increments, where as the >= x.y.z, <a.b.c allows minors?

https://developer.hashicorp.com/terraform/language/expressions/version-constraints#-3

It is the rightmost number, so if you only specify the minor it will increment the minor too. For example:

Current release: 1.4.7

Next release: 1.4.8

Next release: 1.5.0

Next release: 2.0.0

From the docs (see **):

[~>]: Allows only the rightmost version component to increment. This format is referred to as the pessimistic constraint operator. For example, to allow new patch releases within a specific minor release, use the full version number:

[~> 1.0.4]: Allows Terraform to install 1.0.5 and 1.0.10 but not 1.1.0.
[~> 1.1]: Allows Terraform to install 1.2 and 1.10 but not 2.0. **
matt-FFFFFF commented 5 months ago

Ahh cool. I didn't know that.

Happy to allow both formats and update the spec!

jchancellor-ms commented 5 months ago

@jaredfholgate - I've approved, sorry for the delay.

jaredfholgate commented 5 months ago

@Azure/avm-core-team-technical Please can I get an approval on this PR? Many thanks.

jaredfholgate commented 5 months ago

Ahh cool. I didn't know that.

Happy to allow both formats and update the spec!

Just to confirm for anybody reading this thread. The spec was updated to include this and provide more clarity in this PR: https://github.com/Azure/Azure-Verified-Modules/pull/535

matt-FFFFFF commented 5 months ago

Only comment is that we should add nullable = false to all collection values