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

[no ticket] Create DOCUMENTATION.md #3231

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

coilysiren
Copy link
Collaborator

@coilysiren coilysiren commented Dec 16, 2024

docs docs docs

@coilysiren coilysiren marked this pull request as ready for review December 16, 2024 18:44
@coilysiren coilysiren changed the title Create DOCUMENTATION.md [no ticket] Create DOCUMENTATION.md Dec 16, 2024
DOCUMENTATION.md Outdated
2. All engineering documentation in its final state must live in GitHub.
3. Draft / WIP engineering documentation can live in Google Drive if desired
4. Code documenation of 3 paragraphs or longer must go into a `.md` file in the same location as the code. You must be able to find every engineering `.md` file via "walking down" links from the root README.md of the repository. Which is to say, add a link from `DEVELOPEMENT.md` or `OPERATIONS.md` or `README.md` (any of them, there are several) into your new `.md` file.
6. Code comments of 1 paragraph of longer, [or otherwise sufficiently complex](https://github.com/HHS/simpler-grants-gov/blob/02d99c055734edb3d705044de669f7856c0a8fe7/api/tests/src/api/users/test_user_route_login.py#L16-L41), must be linked to from a `.md` file, any `.md` file, pick whichever one makes the most sense.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to move this shortly, there isn't a reason to include it here. Auth was getting built quickly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's reasonable, sorry if it felt like a callout 😅. I should come up with a mock example for "sufficiently complex" or maybe leave it ambiguous. Ambiguous is fine sometimes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually appreciate having a specific example. @coilysiren feel free to use one of my more verbose docstrings as an example. Maybe this one?

I struggle because (speaking of colocation) it can be helpful to have the steps outlined directly in the method docstrings, but I also have a more detailed example in documentation/analytics/metrics/burndown.md

In the case of the examples linked above, would you recommend removing the steps listed under Notes in favor of a link to that markdown file (after moving it into the analytics/ codebase)?

11. Always prefer documentation living close to its code, rather than inside of the `documentation/` folder. Tech specs / preliminary design documentation is the exception to this. If in doubt:

- Documentation created before code is written may go inside of `documentation/`
- Documentation created after code is written should not go inside of `documentation/`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you recommend we document something that lives across a few paths?

For example, on the API side there are a few folders:

  • /src/api - where we store API endpoints
  • /src/services - where we store most of the implementation for a given endpoint (to keep the endpoint logic focused on the input/output/configurational bits)
  • /src/auth - where we implement the auth code (used by nearly everything in /src/api)

Auth in particular has a lot built into each one, and also has a lot of implicit behavior with how it might be used - following standard OAuth patterns.

For this, would it make more sense to document it somewhere in documentation, and then link to it in each of those places?


I generally prefer a centralized document hub as putting a ton of it mixed into the code requires knowing where to look. I do think there is a middleground of "document centrally, reference everywhere" to get the best of both worlds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding you right, I would imagine a doc structure like this:

/src/api/README.md
/src/services/README.md
/src/auth/README.md
/src/arbitrarily/deep/folder/README.md
/src/README.md # <== just links to all the subfolders above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of some language we can add in here to make this more obvious?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer a centralized document hub as putting a ton of it mixed into the code requires knowing where to look. I do think there is a middleground of "document centrally, reference everywhere" to get the best of both worlds.

I tend to agree with @chouinar on this one. Along the same lines as his suggestion above, I might suggest this pattern as a compromise:

  1. Root level documentation/ folder is reserved for top-level guides, onboarding docs, wiki, etc.
  2. Codebase-level docs/ folder (e.g. api/docs/) is the main hub for detailed documentation about the codebase, diagrams, etc.
  3. Sub-folder README.md (e.g. api/src/db/README.md) is used to provide short overviews about each important sub-section of the codebase with links to the relevant api/docs/ section

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's valid I agree with the general idea but I still want suggestions on how to capture it in this document!

Copy link
Collaborator

@widal001 widal001 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I might not have been clear with my response, but my suggestions would be to replace item 11 with something like:

11. Reserve the `documentation/` folder at the root of the repo for engineering docs that apply across 
    multiple codebases, such as onboarding guides, runbooks, and high-level technical overviews.
12. Use a `docs/` folder within each codebase’s root (e.g., `api/docs/`, frontend/docs/) for detailed 
    technical documentation, such as explanation of data flows, and system architecture diagrams.
13. Provide contextual summaries using README.md files in relevant subfolders of the codebase 
    (e.g., api/src/db/README.md). These files should give _short_ overviews of the subfolder's purpose 
    and its key components, but otherwise link to relevant sections in the corresponding `docs/` folder.

Copy link
Collaborator

@widal001 widal001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very exciting! Love the direction this moves us in overall.

I had a couple of specific questions/suggestions about the guidelines for where documentation lives though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this! Documentation is near and dear to my heart and having clearly documented standards around it is even better!

Comment on lines +5 to +6
1. Remember that all our documentation is public-facing.
2. All engineering documentation in its final state must live in GitHub.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree with this in spirit, though, in practice I imagine it may be a bit more nuanced?

For example, would the following be considered "engineering documentation" and if so, where do we think they should live according to the guidelines from the rest of this doc?

  1. Security documentation (i.e. SIAs, POAMs, etc.)
  2. Incident response plans
  3. Runbooks for common internal engineering tasks shared across codebases (e.g. how to trigger a step function and check its logs, how to connect to our various DBs in RDS and run SQL queries against them)
  4. Post mortems

I could see a case for most of these at least living in our public wiki (with appropriate redaction of sensitive info) but that would still put it in our documentation/wiki sub-directory instead of in one of the codebase sub-directories, which seems like it might conflict with guideline 11.

Copy link
Collaborator Author

@coilysiren coilysiren Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll clarify this line to "code (python, js) and configuration (terraform) development"

Security docs and operational docs are their own whole different category of things. Security docs shouldn't follow these rules at all. Operational docs are hit / miss.

Runbooks specifically should follow the "redact IDs, use generic URLs" (or however I say it, I'm on mobile right now) rules below, so I don't personally see a strong reason not to co-locate them with the majority of the other documentation. Moreso because I'm going to be a primary author of runbooks, and I don't personally feel compelled to push them into an internal wiki.

This speaks more to the repository organization than anything else, and that subject is WIP. Anyone due to be eventually on call (which includes me for sure, and idk who beyond that) should make this call specifically. So idk maybe @chouinar has an opinion on where runbooks should live, or maybe he never wants to be on call 😆

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having a dedicated folder in our documentation for runbooks might help? Even if some of the sections just link to other parts of the docs.

Especially in an oncall critical situation, I want a single place to start.

DOCUMENTATION.md Outdated Show resolved Hide resolved
11. Always prefer documentation living close to its code, rather than inside of the `documentation/` folder. Tech specs / preliminary design documentation is the exception to this. If in doubt:

- Documentation created before code is written may go inside of `documentation/`
- Documentation created after code is written should not go inside of `documentation/`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally prefer a centralized document hub as putting a ton of it mixed into the code requires knowing where to look. I do think there is a middleground of "document centrally, reference everywhere" to get the best of both worlds.

I tend to agree with @chouinar on this one. Along the same lines as his suggestion above, I might suggest this pattern as a compromise:

  1. Root level documentation/ folder is reserved for top-level guides, onboarding docs, wiki, etc.
  2. Codebase-level docs/ folder (e.g. api/docs/) is the main hub for detailed documentation about the codebase, diagrams, etc.
  3. Sub-folder README.md (e.g. api/src/db/README.md) is used to provide short overviews about each important sub-section of the codebase with links to the relevant api/docs/ section


12. Meta documents (such as this one) must also follow these rules.
13. These rules only apply to engineering documentation.
14. These rules should be actively enforced. Which is to say: request changes inside of PRs that don't follow these rules.
Copy link
Collaborator

@widal001 widal001 Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also be able to set up some linting to enforce a simple subset of these rules!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I can't recall seeing a linter that comes with these types of rules out of the box, and idk about using engineering time to make them. It's a good idea though!

8. Architecture diagrams go into Github. A link into the source drafting tool for the architecture diagram must be added as well, like for example Lucid Chart.
9. Github issues / PRs documenting durable information of 3 paragraphs of longer, or of sufficient complexity or value, must be linked to from a `.md` file.
10. Avoid linking into the repo's root README.md file if at all reasonable.
11. Always prefer documentation living close to its code, rather than inside of the `documentation/` folder. Tech specs / preliminary design documentation is the exception to this. If in doubt:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also curious what our stance on ADRs is?

I can imagine it being helpful to co-locate ADRs with the codebase to which they're relevant, but right now, keeping them in documentation/wiki/decisions allows us to auto-sync them with the public wiki and keeps our decisions together.

If we wanted to make the case for colocation with the relevant codebases I'd be open to it, but we should come up with a plan to preserve discoverability from the wiki page for ADRs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ADRs are such a long form and widely spanning category of content that they should go into documentation. I'll call this out specifically


7. AWS links and screenshots that only contain a generic URL (ex. https://us-east-1.console.aws.amazon.com/rds/home?region=us-east-1#databases) and generic names visible on the screen (eg. simply `api-dev` without any attached unique IDs) are generally safe. If in doubt, don't include it.
8. Architecture diagrams go into Github. A link into the source drafting tool for the architecture diagram must be added as well, like for example Lucid Chart.
9. Github issues / PRs documenting durable information of 3 paragraphs of longer, or of sufficient complexity or value, must be linked to from a `.md` file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this one! It's a good litmus test for when documentation is needed.

- anything related to IAM

7. AWS links and screenshots that only contain a generic URL (ex. https://us-east-1.console.aws.amazon.com/rds/home?region=us-east-1#databases) and generic names visible on the screen (eg. simply `api-dev` without any attached unique IDs) are generally safe. If in doubt, don't include it.
8. Architecture diagrams go into Github. A link into the source drafting tool for the architecture diagram must be added as well, like for example Lucid Chart.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be worth specifically recommending a diagramming tool whose source code can be version controlled itself to avoid permission issues? e.g. mermaid.js or graphviz

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of this, the answer for 90% of cases is mermaid

https://docs.mermaidchart.com/blog/posts/mermaid-supports-architecture-diagrams

I'll add that into the doc

DOCUMENTATION.md Outdated Show resolved Hide resolved
coilysiren and others added 2 commits December 17, 2024 07:06
Co-authored-by: Billy Daly <[email protected]>
Co-authored-by: Billy Daly <[email protected]>
@coilysiren coilysiren marked this pull request as draft December 19, 2024 22:34
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.

3 participants