search

Azure / Azure-Verified-Modules

Azure Verified Modules (AVM) is an initiative to consolidate and set the standards for what a good Infrastructure-as-Code module looks like. Modules will then align to these standards, across languages (Bicep, Terraform etc.) and will then be classified as AVMs and available from their respective language specific registries.
https://aka.ms/AVM
MIT License
354 stars 83 forks source link

[Module Proposal]: `avm-res-containerregistry-registry` #38

Closed pradorodriguez closed 9 months ago

pradorodriguez commented 1 year ago

Check for previous/existing GitHub issues/module proposals

Terraform or Bicep?

Terraform

Module Classification?

Resource Module

Module Name

avm-res-containerregistry-registry

Module Details

GH repo name is terraform-azurerm-avm-res-containerregistry-registry Terraform module to build an Azure Container Registry

Do you want to be the owner of this module?

Yes

Module Owner's GitHub Username

pradorodriguez

jeanchg commented 1 year ago

@pradorodriguez, thanks for the submission! Pls confirm you agree to the below: • https://azure.github.io/Azure-Verified-Modules/specs/shared/team-definitions/ • https://azure.github.io/Azure-Verified-Modules/specs/shared/ https://azure.github.io/Azure-Verified-Modules/help-support/module-support/ (the support aspect in particular is quite important) I'll then assign it to you, thanks!!

pradorodriguez commented 1 year ago

@pradorodriguez, thanks for the submission! Pls confirm you agree to the below: • https://azure.github.io/Azure-Verified-Modules/specs/shared/team-definitions/ • https://azure.github.io/Azure-Verified-Modules/specs/shared/ https://azure.github.io/Azure-Verified-Modules/help-support/module-support/ (the support aspect in particular is quite important) I'll then assign it to you, thanks!!

Dear @jeanchg , I agree.

PmeshramPM commented 1 year ago

@pradorodriguez -Please pay attention to the module name, GH repo name that I have updated in the main description above.

kewalaka commented 1 year ago

I'm interested in the idea of AVM and would like to help. I know PRs aren't open yet, but I wanted to learn from the AVM guidance and made an example as a learning exercise here:

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

It is cloned from the AVM template, following the guidance on https://aka.ms/avm, using the keyvault from @matt-FFFFFF as inspiration.

e2e tests cover default, private-endpoint, "low cost" (basic SKU), and a geo-replication example, these are all passing.

@pradorodriguez - I'm happy to raise a PR once the properly named repository is created.

Hope this is useful, next steps?

PmeshramPM commented 1 year ago

I am checking with @pradorodriguez on the current status on this module

kewalaka commented 1 year ago

according to the spec the GH name should be https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry and the module name should be avm-res-containerregistry-registry

PmeshramPM commented 1 year ago

@kewalaka - Thank you and updated.

kewalaka commented 1 year ago

ref: https://github.com/Azure/Azure-Verified-Modules/issues/38#issuecomment-1751971828

@pradorodriguez - keen to help as per above. The module above was made specifically following AVM guidance and covers the base resource pretty well, I'm eager to get the ball rolling and see if we can make ACR the second released resource 😄.

pradorodriguez commented 1 year ago

Hi @kewalaka , would you like to take the lead of this AVM? You have already built the module. Let me know your thoughts.

kewalaka commented 1 year ago

@pradorodriguez happy to be co-owner - I can't be an owner as I'm not Microsoft. I'm happy to walk you through the module too if you'd like, my github profile has contact details.

mbilalamjad commented 12 months ago

Hey @pradorodriguez just checking in to confirm if there any any further updates on this, have you decided on how to work with @kewalaka going forward, you would have to own this module as Microsoft FTE and can have @kewalaka as co-owner and leverage the great work he's already done for this proposed module CC @PmeshramPM

pradorodriguez commented 11 months ago

Hi @mbilalamjad. I am trying to create the Github Repo but I am getting an "Failed request: (401)" error when configuring the Azure org and conduct business review. This is the repo I created: https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry

kewalaka commented 11 months ago

@pradorodriguez the above repo doesn't seem to be created from the AVM template - does this step come later?

https://github.com/Azure/terraform-azurerm-avm-template

i.e. should look like this:

image

..which you get by creating the GH repo from the above link:

image

pradorodriguez commented 11 months ago

Dear @mbilalamjad and @kewalaka , I was able to create the github repo. I executed the following tasks:

  1. GH repo created
  2. Labels applied
  3. Permissions assigned
  4. Cloned @kewalaka repo to my local PC (https://github.com/kewalaka/terraform-azurerm-avm-res-containerregistry-registry)
  5. Pushed the content of @kewalaka 's repo to the newly created GH repo (https://github.com/pradorodriguez/terraform-azurerm-avm-res-resources-resourcegroups)

Next steps:

  1. Validate standard and policies defined in the AVM site (https://azure.github.io/Azure-Verified-Modules/)
kewalaka commented 11 months ago

As per the screenshot in my previous comment, this repo it isn't created from the template (but the version from my repo is), I don't know if that is a "must" requirement? This has been raised previously in relation to the storage account, can we get feedback from the AVM team on this, please @mbilalamjad ?

The version from my repo was created from an earlier version of the AVM template, so the file contents will match the AVM template at the point this was created, I can PR in changes since.

the GH label will need an update, it mentions this is the module for a public IP.

the _header.md needs amending now this is an official module repo.

Validate standard and policies defined in the AVM site (https://azure.github.io/Azure-Verified-Modules/)

expecting there to be a few suggestions! I can fix up a few things a little later today, based on my learnings over the last few weeks 🙂, there is additional feature coverage & examples to add too.

Cool to see it up there, thanks @pradorodriguez !

mbilalamjad commented 11 months ago

As per the screenshot in my previous comment, this repo it isn't created from the template (but the version from my repo is), I don't know if that is a "must" requirement? This has been raised previously in relation to the storage account, can we get feedback from the AVM team on this, please @mbilalamjad Bilal Amjad FTE ?

The version from my repo was created from an earlier version of the AVM template, so the file contents will match the AVM template at the point this was created, I can PR in changes since.

the GH label will need an update, it mentions this is the module for a public IP.

the _header.md needs amending now this is an official module repo.

Validate standard and policies defined in the AVM site (https://azure.github.io/Azure-Verified-Modules/)

expecting there to be a few suggestions! I can fix up a few things a little later today, based on my learnings over the last few weeks 🙂, there is additional feature coverage & examples to add too.

Cool to see it up there, thanks @pradorodriguez Marco Prado FTE !

@pradorodriguez based on @kewalaka feedback above, the repo mentioned in step 5 doesn't seem to be the right repo as that's for resource group, could you please share the repo created in the Azure github org with him which should be created based on the repo template for AVM

kewalaka commented 11 months ago

repo is here; https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry

has been shared & i've done the first PR to align with recent AVM template changes.

Question remaining for @Azure/avm-core-team-technical - must the repo say "generated by the AVM template"?

i.e. like this example:

286711342-52c8ef32-7b94-4026-ac19-122bf3ac96e2

Personally, I don't think it matters in this case - unlike the storage example this repo has the right structure & CI... just don't want to go too far & then have to re-create & lose issue tracking/history🙂

pradorodriguez commented 11 months ago

Dear @mbilalamjad , The repo was forked from the template by @kewalaka , and recently he updated the version to match the latest updates. You can validate this by tracking the history of the PR and forks:

  1. Latest PR: https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry/pull/2
  2. Origin Repo: https://github.com/kewalaka/terraform-azurerm-avm-res-containerregistry-registry/tree/main
  3. Forked Repo: https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry

Let me know if you have any other question or comment. Regards

kewalaka commented 11 months ago

hi @pradorodriguez

my comment was more regarding that the original repo (number 3 above), doesn't say it was created from the AVM template:

i.e. compare this one, which is the official Azure version: image

to this one in my repo:

image

That "generated from" line is done by starting with the AVM template:

image

Is this necessary, because my commit has aligned to this template? I'm not sure.

@matt-FFFFFF based on related discussions we've had - can you advise if where we're at is ok, or if the repo needs to be recreated so it matches the second screenshot above?

matt-FFFFFF commented 11 months ago

As long as the repo is compliant, again for this case we can make an exception.

Suggest force pushing the @kewalaka commits into the Azure repo if necessary.

We will have tooling soon to automatically validate AVM repo compliance.

mbilalamjad commented 11 months ago

Hi @pradorodriguez,

Performed a review of the module and have the following recommendations. As per the contribution process the PR needed to be reviewed by the AVM core team but since it was already merged with out a review from the AVM Core team therefore, I created an issue for tracking. https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry/issues/3

  1. terraform.lock.hcl shouldn't be in the repo per the .gitignore file.
  2. Update the support.md file.
  3. Consider following specs TFNFR31 for the local.tf file.
  4. Consider updating version to 0.1.0 as the first version that would be published into the terraform registry per spec SNFR17.
  5. Consider updating output to contain Resource Name, ID and Object per specs RMFR7 & TFFR2.
  6. Consider setting prevent_deletion_if_contains_resources to false in provider block in example code per spec TFNFR36.
  7. Consider setting a constraint on maximum major version of Provider per spec TFNFR26.
  8. The Contributor and Owner teams are not added to the repo per spec SNFR20.

Once you action the above you should be good to publish 0.1.0 to the terraform registry

CC: @matt-FFFFFF, @PmeshramPM, @segraef

kewalaka commented 11 months ago

@mbilalamjad terraform.lock.hcl was introduced to the AVM template by @matt-FFFFFF in this commit, has this review finding been superseded?

https://github.com/Azure/terraform-azurerm-avm-template/pull/40

mbilalamjad commented 11 months ago

@mbilalamjad Bilal Amjad FTE terraform.lock.hcl was introduced to the AVM template by @matt-FFFFFF Matt White FTE in this commit, has this review finding been superseded?

Azure/terraform-azurerm-avm-template#40

From the commit that you've mentioned, please see the .gitignore file in which the .terraform.lock.hcl should be ignored whereas its currently present in the repo

matt-FFFFFF commented 11 months ago

Apologies. Will sort

kewalaka commented 11 months ago

From the commit that you've mentioned, please see the .gitignore file in which the .terraform.lock.hcl should be ignored whereas its currently present in the repo

ah got what you mean, sorry! done here (https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry/pull/5)

though, i'm reading Matt's comment and my takeaway is it should be there 😅 (that's what the Terraform docs say).. if it needs to be, will put it back later!

PmeshramPM commented 10 months ago

@pradorodriguez to reach out to @mbilalamjad - he needs some help with the workflow actions

mbilalamjad commented 9 months ago

@pradorodriguez, @kewalaka I performed a second review and approved the latest PR, if there's not further updates ahead of publishing to the registry that you'd like to do than I'd suggest going ahead with publishing it

pradorodriguez commented 9 months ago

@pradorodriguez, @kewalaka I performed a second review and approved the latest PR, if there's not further updates ahead of publishing to the registry that you'd like to do than I'd suggest going ahead with publishing it

Hi @mbilalamjad , we will proceed after the revision of PR 12 (grept run): https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry/pull/12

mbilalamjad commented 9 months ago

Thank you so much @pradorodriguez and @kewalaka for your contribution.

Closing this proposal as module has been published to terraform registry below. https://registry.terraform.io/modules/Azure/avm-res-containerregistry-registry/azurerm/latest

Further discussion on this module including issues & PRs to take please on its repo below https://github.com/Azure/terraform-azurerm-avm-res-containerregistry-registry