Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes requested during avm team review #4

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

kewalaka
Copy link
Contributor

@kewalaka kewalaka commented Dec 18, 2023

to close #3

Review Feedback

  • terraform.lock.hcl shouldn't be in the repo per the .gitignore file.

@mbilalamjad terraform.lock.hcl was introduced to the AVM template in this PR, has this review finding been superseded?

  • Update the support.md file.

for @pradorodriguez please

  • Consider following specs TFNFR31 for the local.tf file.

What was done was following the style of the KeyVault, which in my opinion is much more readable. However, I have merged the locals block into a single block (which I think is what your ask is).

  • Consider updating version to 0.1.0 as the first version that would be published into the terraform registry per spec SNFR17.

done

  • Consider updating output to contain Resource Name, ID and Object per specs RMFR7 & TFFR2.

done, though I don't much care for the 'system assigned mi' output. What if a user assigned identity is being used?

i also note that Keyvault doesn't include the outputs in RMFR7, they seem unnecessary since you can get to them via the full resource output, but, done :)

  • Consider setting prevent_deletion_if_contains_resources to false in provider block in example code per spec TFNFR36.

done

  • Consider setting a constraint on maximum major version of Provider per spec TFNFR26.

can't find any instances of this

  • The Contributor and Owner teams are not added to the repo per spec SNFR20.

another one for @pradorodriguez , please.

@kewalaka
Copy link
Contributor Author

also I noted:

@pradorodriguez would you update the github "about" please - i don't think I have access, it currently says:

About
AVM Terraform module for public IP address

@kewalaka
Copy link
Contributor Author

kewalaka commented Dec 18, 2023

Tests pass, except an external linting issue that has been recently introduced in the template upstream, there's a PR to fix that. All E2Es pass, evidence:

https://github.com/kewalaka/terraform-azurerm-avm-res-containerregistry-registry/actions/runs/7253195647

@kewalaka kewalaka marked this pull request as draft December 18, 2023 20:22
@kewalaka
Copy link
Contributor Author

@pradorodriguez i've converted the PR to draft, if you can do the other support-related matters above please, then remove the draft status.

@pradorodriguez
Copy link
Collaborator

AVM Module Review #3

@pradorodriguez pradorodriguez merged commit 6acf239 into Azure:main Dec 18, 2023
4 of 9 checks passed
@kewalaka kewalaka mentioned this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVM Module Review
2 participants