Conversation
sean-navapbc
left a comment
sean-navapbc
left a comment
There was a problem hiding this comment.
Approve with minor changes - The documentation is valuable and well-structured. Just a few typos to fix before merging.
| - Layer: Corresponds to root modules and actual resources. | ||
|
|
||
|  |
There was a problem hiding this comment.
The visualization is hosted on lucid.app. Consider:
- Is the image publicly accessible long-term?
- Should there be a fallback or the image committed to the repo?
- What if the Lucidchart account changes ownership?
There was a problem hiding this comment.
This is the general approach we've taken before, like on the system architecture diagram (and that image in the README.md). It is a bit strange if non-Nava staff are using the template, as they can't easily actually make a copy for themselves, but that's not a new problem.
Is the image publicly accessible long-term?
Yes.
Should there be a fallback or the image committed to the repo?
I'm generally against committing a copy, but we could.
What if the Lucidchart account changes ownership?
Nava will have a huge migration effort if as an org we move off of Lucidchart, so this would just need to be a part of those plans.
lorenyu
left a comment
lorenyu
left a comment
There was a problem hiding this comment.
Overall looks good and I support merging as is. The slice concept for me doesn't really quite gel, maybe we can user test this a bit with some engineers?
There was a problem hiding this comment.
Should we link to this doc somewhere from infra/README.md or somewhere?
| - Slice: Conceptual or organizational grouping, it does not directly correspond | ||
| to infrastructure resources itself, though may be expressed in the file | ||
| structure. |
There was a problem hiding this comment.
I don't know the right solution, but it feels like the definition of Slice can be a little more precise and narrow. Conceptual grouping feels like it can potentially refer to a lot of things, so until you see examples there isn't really a rigorous definition of what it means.
Also, I think "app environment" as a slice seems analogous to different accounts or different networks as a slice e.g. "nonprod network" or "prod network", but that doesn't seem captured here. Is it because "env-config" is in a folder but network configs are all in one file?
I wonder if there's a concept like "layer instance" to represent the difference between different sets of deployed resources within a layer backed by a different tfbackend file. Would that be helpful? It doesn't seem like the same as "slice" but maybe also a useful concept.
re: Configs, conceptually, network configs could have been structured the same as env-config, or vice versa, we could have put all env configs in a big file, but for now I think network configs are in one file because so far it's simple enough to fit in one file.
I think the way I think about it logically is:
| config | depends on | |
|---|---|---|
| project config | all layers | |
| # there are no account level configs per account because we treat all accounts the same | ||
| network configs per network | project config | network layer |
| app configs per app | network config, project config | build repository layer, database layer, service layer |
| environment config per app environment | app config, network config, project config | database layer, service layer, and network layer |
note: the network layer actually references both app config and app env configs in these lines
|  |
There was a problem hiding this comment.
Could be something we address later, but the items in the diagram are spread out which causes the text to be small, so if there's a way we can make the diagram more space efficient it can be more readable
| TL;DR for the why: group resources with a similar lifecycle and scope, reduce | ||
| code duplication and drift between environments, help streamline receiving | ||
| updates from upstream | ||
|
|
||
| It's important to understand most of the actual resource code [lives in | ||
| re-usable modules under `/infra/modules/`](/docs/infra/module-architecture.md). | ||
| Everything else largely exists to call that re-usable code with the correct | ||
| parameters at the correct time, [in a well structured | ||
| way](/docs/infra/module-dependencies.md). That "everything else" can be broken | ||
| down into three high-level "slices" (some with further internal divisions). | ||
|
|
||
| One last bit of exposition, as yet more new terminology has been introduced with | ||
| "slice". If you've read other documentation already you'll have likely seen | ||
| references to infrastructure "layers", but there are some conceptual | ||
| pseudo-layers that, for clarity, that this document calls a "slice". Some slices | ||
| directly correspond to a layer, other slices correspond to multiple layers. In | ||
| summary: |
There was a problem hiding this comment.
nit: This part feels just a bit too conversational in tone? I wonder if we could formalize it just a tad.
3 participants
It has felt like there is a bit of a missing middle ground in the docs for conceptualizing and then applying the concepts with the infra template. This doc is an attempt to fill the void. Trying to strike a balance between having enough detail to serve the purpose, but not too much to overwhelm and duplicate other documentation.
The visualization is the main point, with the rest of the doc capturing details that don't fit and somewhat of a basis for a script if you were to walk someone through the graphic. As well as a few other misc. tweaks and fixes to related docs.
Although this is in the AWS template repo, it's largely applicable to the current Azure template, so felt reasonable to just have a couple callouts on the differences between the AWS and Azure setup, rather than duplicate entirely.