ADR-0001: Style guide #191

mikealfare · 2024-05-03T21:53:08Z

Description

establish a style guide
setup basic ADR framework to support this decision and future decisions

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue

emmyoop

First pass. Definitely a few things to discuss 😄

emmyoop · 2024-05-10T14:20:14Z

docs/arch/adr-0001-style-guide.md

+- 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


Is the expectation here that whatever utilizes the workflows works off main or off the calver tag? Would also be useful to link to calver docs and define the specific format we should be using.

Yes, and agreed; I'll update this with the link and format.

Folks can use main if they're looking for something to do down the line, or they can use the calver tag and use dependabot to test updates.

emmyoop · 2024-05-10T14:21:02Z

docs/arch/adr-0001-style-guide.md

+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.


We should split this into workflows and actions. They're different.

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?

emmyoop · 2024-05-10T14:24:05Z

docs/arch/adr-0001-style-guide.md

+
+### Workflows
+- workflows need to live in `.github/workflows`
+- we prefer actions to workflows


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.

I struggle with building new workflows off of other workflows for a few reasons:

reusable workflows are forced to be their own jobs in the calling workflow

I can't embed them in a job

I can't pass environment variables to workflows, which is especially a problem when those change by repo (e.g. connection credentials)

I need to create a bunch of inputs to pass to them, to get a bunch of outputs from them, to pass those outputs to another job, in many cases to perform one or two additional steps

any reusable workflow that needs access to the code requires adding repo and branch/ref as arguments

with an action, you do the checkout in the calling workflow and the called action can focus on the task

reusable workflows tend to take on too much scope, making them difficult to combine

for example, we currently run tests twice during our release for each adapter because both the bumpversion/changelog workflow and build/test workflow runs tests; ideally there is an action for bumpversion, an action for changelog generation, an action for build, and these can all be selected al a carte as needed

stated another way, actions should be created to perform a specific task, which should minimize maintenance for a particular action, similar to how python functions should be built

emmyoop · 2024-05-10T14:24:15Z

docs/arch/adr-0001-style-guide.md

+- workflows need to live in `.github/workflows`
+- we prefer actions to workflows
+    - actions can be inserted into other actions and as part of workflows
+    - actions can inherit environment variables


So do workflows?

Workflows do not inherit environment variables: https://docs.github.com/en/enterprise-cloud@latest/actions/using-workflows/reusing-workflows#limitations

emmyoop · 2024-05-10T14:28:03Z

docs/arch/adr-0001-style-guide.md

+### Workflows
+- workflows need to live in `.github/workflows`
+- we prefer actions to workflows
+    - actions can be inserted into other actions and as part of workflows


The difference here being that workflows can't be part of actions?

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.

emmyoop · 2024-05-10T14:28:36Z

docs/arch/adr-0001-style-guide.md

+
+## Proposal
+
+### Actions


What types of actions should we prefer - why?

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).

emmyoop · 2024-05-10T14:34:44Z

docs/arch/adr-0001-style-guide.md

+    - `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 }}`


We should have a scripts section. When should we write a script vs just doing it inline? Where should scripts live? etc

Yeah, that probably makes sense. To initially respond to the few examples you gave:

I am moving more and more towards scripts always; so basically if it's more than a line, make it a script

scripts should live alongside the action.yml which calls them, multiple actions should not depend on the same script

emmyoop · 2024-05-10T14:35:04Z

docs/arch/adr-0001-style-guide.md

+- 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


Alternatives?

Alternatives to the proposed linters and formatters

Oooh. Should we set up pre-commit in this repo?

emmyoop · 2024-05-10T14:35:42Z

docs/arch/adr-0001-style-guide.md

+
+Alternatives should support `pre-commit`: https://pre-commit.com/hooks.html
+
+### Logging


What does logging mean?

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.

I think it'll be useful to explicitly distinguish between when to "echo" vs using the GitHub notification that show in the summary.

emmyoop · 2024-05-10T18:04:44Z

docs/arch/adr-0001-style-guide.md

+- 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


You want an action that just does the notify that's built into github?

first draft adr for actions style guide

2d2dcfc

mikealfare self-assigned this May 3, 2024

mikealfare mentioned this pull request May 3, 2024

[Release Improvements] Migrate actions from dbt-adapters #173

Open

add style guide to the PR template

4b2859f

emmyoop reviewed May 10, 2024

View reviewed changes

Merge branch 'main' into adr-0001

673ad24

emmyoop reviewed May 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR-0001: Style guide #191

ADR-0001: Style guide #191

mikealfare commented May 3, 2024 •

edited

Loading

emmyoop left a comment

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

mikealfare May 10, 2024

emmyoop May 10, 2024

emmyoop May 10, 2024

		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.


		Alternatives should support `pre-commit`: https://pre-commit.com/hooks.html

		### Logging

ADR-0001: Style guide #191

Are you sure you want to change the base?

ADR-0001: Style guide #191

Conversation

mikealfare commented May 3, 2024 • edited Loading

Description

Checklist

emmyoop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikealfare commented May 3, 2024 •

edited

Loading