ADR-0001: Style guide #191
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Architectural Design Records (ADRs) | ||
|
|
||
| For any architectural/engineering decisions we make, we will create an ADR (Architectural Design Record) | ||
| to keep track of what decision we made and why. This allows us to refer back to decisions in the future | ||
| and see if the reasons we made a choice still holds true. This also allows for others to more easily understand the code. | ||
| ADRs will follow this process: | ||
|
|
||
| - They will live in the repo, under a directory `docs/arch` | ||
| - They will be written in markdown | ||
| - They will follow the naming convention `adr-NNNN-<decision-title>.md` | ||
| - `NNNN` will just be a counter starting at `0001` and will allow us easily keep the records in chronological order. | ||
| - The common sections that each ADR should have are: | ||
| - Title | ||
| - Context | ||
| - Options | ||
| - Decision | ||
| - Consequences | ||
| - Use this article as a reference: [https://cognitect.com/blog/2011/11/15/documenting-architecture-decisions](https://cognitect.com/blog/2011/11/15/documenting-architecture-decisions) | ||
|
|
||
| ## Contents | ||
|
|
||
| - [ADR-0001: Style guide](ard-0001-style-guide.md) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Style Guide | ||
|
|
||
|
|
||
| ## Context | ||
|
|
||
| We need a style guide for our actions repo. This structure provides the following benefits: | ||
| - developers can spend more time on solving the problem | ||
| - PR reviews are limited to discussing intent | ||
| - format related feedback can point to a collective style guide instead of personal preferences | ||
| - consumers know how to find what they're looking for | ||
| - developers know where to look when fixing something | ||
|
|
||
| ## Options | ||
|
|
||
| This style guide is a collection of individual options. Instead of providing permutations of these options, | ||
| this ADR provides a section for an initial proposal for several style guide topics and a section | ||
| that contains the agreed upon formats. This ADR should be updated to remove reference to the initial | ||
| proposal prior to merging in order to avoid confusion. | ||
|
|
||
| As an alternative, each topic could be its own ADR. Reviewers may request this approach, which will require | ||
| breaking this ADR into several ADRs and associated PRs. | ||
|
|
||
| ## Proposal | ||
|
|
||
| ### Actions | ||
|
There was a problem hiding this comment. What types of actions should we prefer - why? There was a problem hiding this comment. I don't have a strong opinion on this. I've been using composite actions since most of the actions I create are pretty small. But some of our actions are more complex and I could see how using python would be easier (like semver parsing). Using python might also be more testable in certain scenarios (again thinking about semver parsing). |
||
| - actions live at the repo root so that usage looks like `uses: dbt-labs/actions/<action-name>@<tag>` | ||
| - actions live in directories with the format `<platform/tool>/<resource>/<action>`, e.g. | ||
| - `./hatch/environment/create` | ||
| - `./pypi/release/view` | ||
| - actions have a generic scope with a focused context to support reusability | ||
| - actions perform a single task on a single object within the context of a platform/tool | ||
|
|
||
| ### Workflows | ||
| - workflows need to live in `.github/workflows` | ||
| - we prefer actions to workflows | ||
|
There was a problem hiding this comment. I don't agree with this. I want to avoid composition of the same 7 actions to accomplish one thing. Having to dig multiple levels down is also frustrating and makes it unclear what's happening. There was a problem hiding this comment. I struggle with building new workflows off of other workflows for a few reasons:
|
||
| - actions can be inserted into other actions and as part of workflows | ||
|
There was a problem hiding this comment. The difference here being that workflows can't be part of actions? There was a problem hiding this comment. Workflow can't be part of actions, and can't be part of other jobs. They need to be stand alone jobs, which is pretty limiting. |
||
| - actions can inherit environment variables | ||
|
There was a problem hiding this comment. Workflows do not inherit environment variables: https://docs.github.com/en/enterprise-cloud@latest/actions/using-workflows/reusing-workflows#limitations |
||
|
|
||
| ### Formatting | ||
| - job ids, step ids, and variables are in kebab case | ||
| - job names, step names, and description fields follow dbt docs standards (capitalize first word only) | ||
| - extra descriptors should be avoided unless required for disambiguation, e.g. | ||
| - `version-number` -> `version` | ||
| - `archive-name` -> `archive` | ||
| - yaml files use a four space tab | ||
| - scripts use environment variables in `env` instead of inline substitution like `${{ inputs.value }}` | ||
|
There was a problem hiding this comment. We should have a scripts section. When should we write a script vs just doing it inline? Where should scripts live? etc There was a problem hiding this comment. Yeah, that probably makes sense. To initially respond to the few examples you gave:
|
||
|
|
||
| ### Tools | ||
| - formatter: `pretty-format-yaml` @ https://github.com/macisamuele/language-formatters-pre-commit-hooks | ||
| - `yamlfmt` @ https://github.com/jumanjihouse/pre-commit-hook-yamlfmt | ||
| - `yamlfmt` @ https://github.com/google/yamlfmt (requires Go) | ||
| - linter: `check-yaml` @ https://github.com/pre-commit/pre-commit-hooks | ||
| - `yamllint` @ https://github.com/adrienverge/yamllint | ||
|
|
||
| Alternatives should support `pre-commit`: https://pre-commit.com/hooks.html | ||
|
There was a problem hiding this comment. Alternatives to the proposed linters and formatters There was a problem hiding this comment. Oooh. Should we set up pre-commit in this repo? |
||
|
|
||
| ### Logging | ||
|
There was a problem hiding this comment. I'm thinking of debug/summary notifications/etc. as logging. So when we print inputs/outputs to standard out, that's a debug log to me. And when we generate a notification that shows up in the summary section, that's an info/error/warn log. There was a problem hiding this comment. I think it'll be useful to explicitly distinguish between when to "echo" vs using the GitHub notification that show in the summary. |
||
| - the first step of an action logs non-secret inputs as a debug log | ||
| - the last step of an action logs non-secret outputs as a debug log | ||
| - any step that changes state outside of the scope of the action has an associated info log | ||
| - any step that results in predictable failure (e.g. unit tests) has an associated error log | ||
| - any step that results in a predictable skip (e.g. version bump not needed) has an associated warning log | ||
| - log types are standardized as an action in this repo | ||
|
There was a problem hiding this comment. You want an action that just does the notify that's built into github? |
||
|
|
||
| ### Docs | ||
| - all actions and workflows are discoverable in the repo `README.md` | ||
| - all workflows have a file docstring in the "what, why, when" format | ||
|
|
||
| ### Versioning | ||
| - actions and workflows are not versioned individually; instead, the entire repo is versioned, similar to the `pre-commit` hooks repo | ||
| - tags should be used to communicate new functionality | ||
| - tagging should follow calver | ||
|
Comment on lines
+70
to
+72
There was a problem hiding this comment. Is the expectation here that whatever utilizes the workflows works off There was a problem hiding this comment. Yes, and agreed; I'll update this with the link and format. Folks can use |
||
|
|
||
| ## Decision | ||
|
|
||
| ## Consequences | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # { Title } | ||
|
|
||
| ## Context | ||
|
|
||
| { A brief problem statement. Why do we need to make this decision? } | ||
|
|
||
| ## Options | ||
|
|
||
| - { Option 1 } | ||
| - ... | ||
|
|
||
| ### { Option 1 } | ||
|
|
||
| #### Pro's | ||
|
|
||
| - { Pro 1 } | ||
| - ... | ||
|
|
||
| #### Con's | ||
|
|
||
| - { Con 1 } | ||
| - ... | ||
|
|
||
| ## Decision | ||
|
|
||
| #### Selected: { Option x } | ||
|
|
||
| { Justification } | ||
|
|
||
| ## Consequences | ||
|
|
||
| - [+] { Positive consequence } | ||
| - [-] { Negative consequence } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should split this into workflows and actions. They're different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we would have one style guide for workflows and another for actions? There's a significant overlap. If we want to distinguish between actions and workflows, but reduce redundancy, can we do one shared style guide, and then an action style guide and workflow style guide that only focus on things specific to actions/workflows?