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

Change the directory structure of the src/bicep folder #561

Closed
brooke-hamilton opened this issue Dec 7, 2021 · 12 comments · Fixed by #699
Closed

Change the directory structure of the src/bicep folder #561

brooke-hamilton opened this issue Dec 7, 2021 · 12 comments · Fixed by #699
Assignees
Labels
bicep Related to Bicep code core New feature or request

Comments

@brooke-hamilton
Copy link
Contributor

brooke-hamilton commented Dec 7, 2021

Benefit/Result/Outcome

Improved folder structure for the src/bicep folder.

Description

This is based on a discussion in #532.

Acceptance Criteria

  • Put modules in the core folder that are scoped to MLZ concepts, like spoke network, and call modules in the modules folder. Generally, modules in this folder will execute modules in the modules folder.
  • Put modules in the modules folder that are scoped to Azure concepts, like storage accounts. Generally, a module in this folder will not execute other modules.
  • Add a README.md to the core folder explaining the purpose of the templates in that folder.
  • Add a README.md to the modules folder explaining the purpose of the templates in that folder.
  • Update the naming convention for all Bicep files to use all lower case letters and add dashes to separate words (making the names consistent with the Terraform files.)
  • Change the name of newWorkload to tier3 to match the name of the Terraform template.
  • Change the Bicep directory structure to match the following (NOTE: the folder structure is what is specified. The exact file names may not match what is below.):
├── add-ons
│   ├── remote-access
│   │   ├── README.md
│   │   ├── main.json
│   │   └── remote-access.bicep
│   ├── tier3
│   │   ├── modules
│   │   │   └── hub-network-peering.bicep
│   │   ├── README.md
│   │   └── tier3.bicep
├── core
│   │── hub-network.bicep
│   │── hub-network-peerings.bicep
│   │── README.md
│   │── remote-access.bicep
│   │── spoke-network.bicep
│   └── spoke-network-peering.bicep
├── examples
│   ├── app-service-plan
│   │   ├── modules
│   │   │   ├── appServicePlan.bicep
│   │   │   └── appServiceSettings.bicep
│   │   ├── README.md
│   │   ├── app-service-plan.bicep
│   │   └── app-service-plan.json
│   ├── container-registry
│   │   ├── modules
│   │   │   └── container-registry.bicep
│   │   ├── README.md
│   │   └── container-registry.bicep
│   ├── inherit-tags
│   │   ├── README.md
│   │   └── inherit-tags.bicep
│   ├── keyvault
│   │   ├── modules
│   │   │   └── key-vault.bicep
│   │   ├── README.md
│   │   ├── key-vault.bicep
│   │   └── key-vault.json
│   ├── sentinel
│   │   ├── README.md
│   │   └── sentinel.tf
│   └── README.md
├── form
│   └── mlz.portal.json
├── modules
│   ├── policies
│   │   ├── CMMC-policyAssignmentParameters.json
│   │   ├── IL5-policyAssignmentParameters.json
│   │   └── NIST-policyAssignmentParameters.json
│   ├── bastion-host.bicep
│   ├── central-logging.bicep
│   ├── firewall.bicep
│   ├── linux-virtual-machine.bicep
│   ├── log-analytics-diagnostic-logging.bicep
│   ├── log-analytics-workspace.bicep
│   ├── network-interface.bicep
│   ├── network-security-group.bicep
│   ├── policy-assignment.bicep
│   ├── private-link.bicep
│   ├── public-ip-address.bicep
│   ├── README.md
│   ├── resource-group.bicep
│   ├── role-assignment.bicep
│   ├── route-table.bicep
│   ├── security-center.bicep
│   ├── storage-account.bicep
│   ├── subnet.bicep
│   ├── virtual-network-peering.bicep
│   ├── virtual-network.bicep
│   └── windows-virtual-machine.bicep
├── README.md
├── bicepconfig.json
├── mlz.bicep
├── mlz.json

@brooke-hamilton
Copy link
Contributor Author

brooke-hamilton commented Dec 13, 2021

Before implementing this, we need to do a couple of things with the proposed structure:

  • Update the names of the folders called 'core' and 'modules' to make sure the relationships are clear, i.e., things in core call things in modules, and modules are broadly reusable.
  • Make sure the initial experience of viewing this structure makes the purpose of the files and the folders obvious.

@brooke-hamilton
Copy link
Contributor Author

brooke-hamilton commented Jan 10, 2022

Another input on this: we had feedback related to auto-generating documentation that each template should be in its own folder so that a README.md document can be generated within the same folder to show parameter docs. That would be in alignment with the Terraform templates.

@brooke-hamilton
Copy link
Contributor Author

Based on discussions above, here's another proposal for the directory structure in src/bicep:

  • All Bicep files are lower case to be consistent with the Terraform files and to avoid casing issues between platforms in Bicep.
  • Added core folder to contain Bicep code that calls individual modules.
  • Put each core item into its own folder and added a README.md document.
  • Renamed newWorkload to tier3 to be consistent with the Terraform template.
  • Removed the build folder.
├── add-ons
│   ├── remote-access
│   │   ├── README.md
│   │   ├── main.json
│   │   └── remote-access.bicep
│   └── tier3
│       ├── modules
│       │   └── hub-network-peering.bicep
│       ├── README.md
│       └── tier3.bicep
├── core
│   ├── hub-network
│   │   ├── README.md
│   │   └── hub-network.bicep
│   ├── hub-network-peerings
│   │   ├── README.md
│   │   └── hub-network-peerings.bicep
│   ├── remote-access
│   │   ├── README.md
│   │   └── remote-access.bicep
│   ├── spoke-network
│   │   ├── README.md
│   │   └── spoke-network.bicep
│   └── spoke-network-peering
│       ├── README.md
│       └── spoke-network-peering.bicep
├── examples
│   ├── app-service-plan
│   │   ├── modules
│   │   │   ├── appServicePlan.bicep
│   │   │   └── appServiceSettings.bicep
│   │   ├── README.md
│   │   ├── app-service-plan.bicep
│   │   └── app-service-plan.json
│   ├── container-registry
│   │   ├── modules
│   │   │   └── container-registry.bicep
│   │   ├── README.md
│   │   └── container-registry.bicep
│   ├── inherit-tags
│   │   ├── README.md
│   │   └── inherit-tags.bicep
│   ├── keyvault
│   │   ├── modules
│   │   │   └── key-vault.bicep
│   │   ├── README.md
│   │   ├── key-vault.bicep
│   │   └── key-vault.json
│   ├── sentinel
│   │   ├── README.md
│   │   └── sentinel.tf
│   ├── README.md
│   └── deploymentVariables.json
├── form
│   └── mlz.portal.json
├── modules
│   ├── policies
│   │   ├── CMMC-policyAssignmentParameters.json
│   │   ├── IL5-policyAssignmentParameters.json
│   │   └── NIST-policyAssignmentParameters.json
│   ├── bastion-host.bicep
│   ├── central-logging.bicep
│   ├── firewall.bicep
│   ├── linux-virtual-machine.bicep
│   ├── log-analytics-diagnostic-logging.bicep
│   ├── log-analytics-workspace.bicep
│   ├── network-interface.bicep
│   ├── network-security-group.bicep
│   ├── policy-assignment.bicep
│   ├── private-link.bicep
│   ├── public-ip-address.bicep
│   ├── resource-group.bicep
│   ├── role-assignment.bicep
│   ├── route-table.bicep
│   ├── security-center.bicep
│   ├── storage-account.bicep
│   ├── subnet.bicep
│   ├── virtual-network-peering.bicep
│   ├── virtual-network.bicep
│   └── windows-virtual-machine.bicep
├── README.md
├── bicepconfig.json
├── mlz.bicep
├── mlz.json

@shawngib
Copy link
Member

Seems to cross streams between opinion and none opinion in modules, also appears very busy with separate folders in core and separate read me. Can't core modules just include comments and descriptions.

@vidyambala
Copy link
Contributor

Looks really great. Out of curiosity, do we need a hub-network-peering.bicep module in tiers as we have same module in core directory?

@brooke-hamilton
Copy link
Contributor Author

Seems to cross streams between opinion and none opinion in modules, also appears very busy with separate folders in core and separate read me. Can't core modules just include comments and descriptions.

Good point that it's inconsistent to have each template in a subfolder in core and not do that in modules. We could choose one of these options:

  1. Remove the subfolders in core and not have README docs for every module--just have parameter decorators in the template.
  2. Add subfolders and README files to modules. We talked about modules being the reusable templates, so maybe those should have READMEs.
  3. Have a different folder layout between core and modules.

I'm 100% OK with any of these options. It's true that having subfolders for every module is busy and causes longer paths.

@brooke-hamilton
Copy link
Contributor Author

Looks really great. Out of curiosity, do we need a hub-network-peering.bicep module in tiers as we have same module in core directory?

Good call: those two modules appear to be nearly exact copies. Yes I think we can remove the one in tier3/modules and use the one in core instead.

@brooke-hamilton
Copy link
Contributor Author

Proposed updates to acceptance criteria and updated folder structure:

  • Add a README.md to the core folder explaining the purpose of the templates in that folder.
  • Add a README.md to the modules folder explaining the purpose of the templates in that folder.
  • Update the naming convention for all Bicep files to use all lower case letters and add dashes to separate words (making the names consistent with the Terraform files.)
  • Change the name of newWorkload to tier3 to match the name of the Terraform template.
  • Change the Bicep directory structure to match the following:
├── add-ons
│   ├── remote-access
│   │   ├── README.md
│   │   ├── main.json
│   │   └── remote-access.bicep
│   └── tier3
│       ├── modules
│       │   └── hub-network-peering.bicep
│       ├── README.md
│       └── tier3.bicep
├── core
│   │── hub-network.bicep
│   │── hub-network-peerings.bicep
│   │── README.md
│   │── remote-access.bicep
│   │── spoke-network.bicep
│   └── spoke-network-peering.bicep
├── examples
│   ├── app-service-plan
│   │   ├── modules
│   │   │   ├── appServicePlan.bicep
│   │   │   └── appServiceSettings.bicep
│   │   ├── README.md
│   │   ├── app-service-plan.bicep
│   │   └── app-service-plan.json
│   ├── container-registry
│   │   ├── modules
│   │   │   └── container-registry.bicep
│   │   ├── README.md
│   │   └── container-registry.bicep
│   ├── inherit-tags
│   │   ├── README.md
│   │   └── inherit-tags.bicep
│   ├── keyvault
│   │   ├── modules
│   │   │   └── key-vault.bicep
│   │   ├── README.md
│   │   ├── key-vault.bicep
│   │   └── key-vault.json
│   ├── sentinel
│   │   ├── README.md
│   │   └── sentinel.tf
│   ├── README.md
│   └── deploymentVariables.json
├── form
│   └── mlz.portal.json
├── modules
│   ├── policies
│   │   ├── CMMC-policyAssignmentParameters.json
│   │   ├── IL5-policyAssignmentParameters.json
│   │   └── NIST-policyAssignmentParameters.json
│   ├── bastion-host.bicep
│   ├── central-logging.bicep
│   ├── firewall.bicep
│   ├── linux-virtual-machine.bicep
│   ├── log-analytics-diagnostic-logging.bicep
│   ├── log-analytics-workspace.bicep
│   ├── network-interface.bicep
│   ├── network-security-group.bicep
│   ├── policy-assignment.bicep
│   ├── private-link.bicep
│   ├── public-ip-address.bicep
│   ├── README.md
│   ├── resource-group.bicep
│   ├── role-assignment.bicep
│   ├── route-table.bicep
│   ├── security-center.bicep
│   ├── storage-account.bicep
│   ├── subnet.bicep
│   ├── virtual-network-peering.bicep
│   ├── virtual-network.bicep
│   └── windows-virtual-machine.bicep
├── README.md
├── bicepconfig.json
├── mlz.bicep
├── mlz.json

@glennmusa
Copy link
Contributor

glennmusa commented Feb 7, 2022

I think that's a step in the right direction and is actionable today.

Glancing into the crystal ball, if there's ever intent to machine generate parameters.json or READMEs for the individual modules, the separate folder for each deployable unit might make sense:

├── add-ons
│   ├── remote-access
│   │   ├── README.md (with auto-gen'd docs)
│   │   ├── parameters.json (with auto-gen'd values)
│   │   ├── main.json
│   │   └── main.bicep
│   └── tier3
│       ├── modules
│       │   └── hub-network-peering.bicep
│       ├── README.md (with auto-gen'd docs)
│       ├── parameters.json (with auto-gen'd values)
│       ├── main.json
│       └── main.bicep
├── core
│   │── hub-network
│   │   ├── README.md (with auto-gen'd docs)
│   │   ├── parameters.json (with auto-gen'd values)
│   │   ├── main.json
│   │   └── main.bicep
│   │── hub-network-peerings
│   │   ├── README.md (with auto-gen'd docs)
│   │   ├── parameters.json (with auto-gen'd values)
│   │   ├── main.json
│   │   └── main.bicep

I want to say this is similar to the out-of-the-box directory structure required for some of the Terraform docs generations tools, but @Chambras can correct me where I'm wrong.

@brooke-hamilton
Copy link
Contributor Author

I think we need the folders for the add-ons and examples, for the reasons you said, and also because we are going to implement a standard pattern for those which provides examples for several ways to set parameters.

For the core and modules folders, will anyone call those things separately? I think we want to get to a place where the Bicep files in core are reusable elements of MLZ that can be deployed together or separately. The Bicep files in modules should also be reusable, and mostly they are.

However, the drawback of creating the folders in core and modules is the extra-long paths and nesting it creates. If nobody ever reuses those Bicep files then having the folders and generated docs/parameters files is unnecessary complexity and a pain to navigate when someone is trying to understand the relationships among the Bicep files.

We could say YAGNI for now and add the folders when we need them, which would have the benefit of being simple now and the drawback of maybe having yet another folder structure change later (better to do it all at once.)

Or we could say do it now because we think nested folders will be needed, which would have the benefit of preventing two separate refactorings and the drawback of complexity that isn't useful right away.

@brooke-hamilton
Copy link
Contributor Author

Based on team consensus we will have no subfolders in core and modules until we find a need for having subfolders.

I will update the acceptance criteria to align with this.

@brooke-hamilton brooke-hamilton self-assigned this Feb 7, 2022
@brooke-hamilton brooke-hamilton moved this from Triage to Backlog in Mission Landing Zone 2022 Feb 7, 2022
@brooke-hamilton brooke-hamilton moved this from Backlog to Deferred in Mission Landing Zone 2022 Feb 7, 2022
@brooke-hamilton
Copy link
Contributor Author

Based on team consensus we will have no subfolders in core and modules until we find a need for having subfolders.

I will update the acceptance criteria to align with this.

Done.

Repository owner moved this from In Progress to Done in Mission Landing Zone 2022 Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bicep Related to Bicep code core New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants