Improve CI setup #1904

mehulkar · 2022-09-10T01:36:58Z

Changes the CI setup to:

Extract Go and node setup to a "composite" action

This sets up the environment consistently. For example, the golangci-lint was
building turbo with Go 1.17, while the Go unit and e2e tests were building with Go 1.18.
Parallelize some jobs for faster feedback

Separates Go unit and e2e tests, and JS lint and tests into separate workflows.
The tradeoff is that each Github Action runner will need to install Go and Node and
build turbo multiple times, but this is fairly fast, and these steps are cacheable.
Improve the names of Workflows, Jobs, and Steps to be easier to debug

Some of the naming changes are for consistency, but some make it easier to
scan the list of PR checks and the Action summaries to see what failed and where.
In some cases, the names of Steps are removed, as the command itself is descriptive
enough, that we do not need to name it.

Review Notes

I'm open to bike-shedding the naming changes¹ and the level of abstraction (e.g. separating Go and Node setup), so if you have suggestions let's hear them!
Look at the Checks tab and the Checks status at the bottom of the PR (by the merge button) to see if it visually helps make sense of what is passing/failing.
The location of the "composite action" doesn't have to be at ./github/actions, but I put it there because it made sense. As far as I can tell, it doesn't collide with any Github features, but I also have 0 experience with GH actions before today.

"Workflows", "Jobs", and "Steps" can all have names, but at least Jobs and Steps aren't required to have names. Job IDs (the yaml key defining the job) can be the identifier). And Steps that just run a single command, can just use the command as its identifier -- in many cases that's much more clear than a name. ↩

vercel · 2022-09-10T01:37:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Sep 15, 2022 at 6:47AM (UTC)

gsoltis · 2022-09-12T18:07:09Z

golangci-lint was building turbo with Go 1.17

This is intentional, a bunch of the linters crash with Go 1.18. See golangci/golangci-lint#2649

gsoltis · 2022-09-12T18:46:43Z

Actually, I think as long as the the golangci-lint config isn't changed, it will still run in go-1.17 mode? So this is probably ok.

mehulkar · 2022-09-12T18:48:56Z

a bunch of the linters crash with Go 1.18. See golangci/golangci-lint#2649

Interesting... I assumed that passing lint meant that it worked fine, but the crashing linters are disabled:

golangci-lint disable those linters automatically with go1.18

I've reverted the linter to 1.17 with this note, but if it's not throwing errors, is it valuable to do this? The remaining "broken" lint rules are: bodyclose, rowserrcheck, sqlclosecheck, wastedassign, go-critic. Is this valuable enough to use inconsistent versions?

mehulkar · 2022-09-12T18:49:31Z

Actually, I think as long as the the golangci-lint config isn't changed, it will still run in go-1.17 mode? So this is probably ok.

Oh hah, I didn't see there's another config there setting the version! In that case, it seems fine? 🤔

mehulkar · 2022-09-12T19:06:33Z

A future possible improvement here is to use a a matrix for the e2e tests for each package manager, so they can be parallelized (rather than running in a loop).

gsoltis · 2022-09-12T22:57:16Z

I'll defer to @nathanhammond since he's done a bunch of the work around this too, but LGTM.

nathanhammond · 2022-09-14T09:16:45Z

.github/workflows/pr-go-lint.yml

        with:
-          go-version: 1.17


You set up go-version as an input but never use it. It would be this task that should set it, yeah?

Yeah, it was customized before for golangci-lint, but then then customization wasn't needed. I kept the feature though, for some imaginary future.

.github/actions/setup-deps/action.yml

.github/workflows/benchmark.yml

.github/workflows/pr-go-run-examples.yml

.github/workflows/pr-go-e2e.yml

.github/workflows/pr-go-run-examples.yml

nathanhammond · 2022-09-14T09:58:43Z

.github/workflows/pr-go-lint.yml

-  #     - v*
-  #   branches:
-  #     - master
-  #     - main
  pull_request:


We can add file-based filters on this (and everywhere else) now.

maybe future PR

nathanhammond

Answering questions.

.github/workflows/pr-go-run-examples.yml

.github/workflows/release.yml

mehulkar · 2022-09-15T06:29:37Z

This PR may be interfering with other PRs to merge? It seems #1932 is showing these new workflows as pending, so it marks the PR unmergeable. Closing this temporarily.

nathanhammond · 2022-09-15T06:34:42Z

Not this PR, it needs to land first. It's the branch protection rules which I just reconfigured for 1904.

Changes the CI setup to: - Extract Go and node setup to a "composite" action, allowing us to setup all CI jobs consistently. For example, the golangci-lint was building turbo with Go 1.17, while the Go unit and e2e tests were building with Go 1.18. - Parallelize some jobs for faster feedback Separates Go unit and e2e tests, and JS lint and tests into separate workflows. The tradeoff is that each Github Action runner will need to install Go and Node and build turbo multiple times, but this is fairly fast, and these steps are highly cacheable. - Improve the names of Workflows, Jobs, and Steps to be more debuggable Some of the naming changes are for consistency, but some make it easier to scan the list of PR checks and the Action summaries to see what failed and where. In some cases, the names of Steps are removed, as the command itself is descriptive enough, that we do not need to name it. Co-authored-by: Nathan Hammond <nathan.hammond@vercel.com>

vercel bot deployed to Preview September 10, 2022 01:37 View deployment

mehulkar force-pushed the mk/tests branch from 89cd3fc to fc3cfe2 Compare September 10, 2022 06:28

vercel bot deployed to Preview September 10, 2022 06:29 View deployment

mehulkar changed the title ~~Split up tests and rename workflow files~~ Improve CI setup Sep 10, 2022

mehulkar force-pushed the mk/tests branch from fc3cfe2 to 789b26d Compare September 10, 2022 06:35

vercel bot deployed to Preview September 10, 2022 06:36 View deployment

mehulkar marked this pull request as ready for review September 10, 2022 07:15

mehulkar requested a review from a team as a code owner September 10, 2022 07:15

mehulkar requested review from nathanhammond and tknickman September 10, 2022 07:15

vercel bot deployed to Preview September 12, 2022 16:33 View deployment

mehulkar added the pr: chore A task that needs to be done (neither feature or bug) label Sep 12, 2022

nathanhammond suggested changes Sep 14, 2022

View reviewed changes

nathanhammond mentioned this pull request Sep 14, 2022

[PR to mk/tests] CI iteration #1961

Merged

nathanhammond reviewed Sep 14, 2022

View reviewed changes

.github/workflows/pr-go-run-examples.yml Outdated Show resolved Hide resolved

.github/workflows/pr-go-run-examples.yml Outdated Show resolved Hide resolved

.github/workflows/release.yml Outdated Show resolved Hide resolved

mehulkar requested a review from nathanhammond September 14, 2022 15:41

vercel bot deployed to Preview September 14, 2022 15:42 View deployment

nathanhammond approved these changes Sep 15, 2022

View reviewed changes

nathanhammond force-pushed the mk/tests branch from 8f6aa5b to 1fa8f81 Compare September 15, 2022 06:27

mehulkar closed this Sep 15, 2022

nathanhammond reopened this Sep 15, 2022

nathanhammond added the pr: automerge Kodiak will merge these automatically after checks pass label Sep 15, 2022

mehulkar force-pushed the mk/tests branch from 1fa8f81 to 1c08899 Compare September 15, 2022 06:46

vercel bot deployed to Preview September 15, 2022 06:47 View deployment

nathanhammond merged commit 231fcf9 into main Sep 15, 2022

nathanhammond deleted the mk/tests branch September 15, 2022 06:54

Improve CI setup #1904

Improve CI setup #1904

Uh oh!

Conversation

mehulkar commented Sep 10, 2022 • edited by nathanhammond Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Notes

Footnotes

Uh oh!

vercel bot commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsoltis commented Sep 12, 2022

Uh oh!

gsoltis commented Sep 12, 2022

Uh oh!

mehulkar commented Sep 12, 2022

Uh oh!

mehulkar commented Sep 12, 2022

Uh oh!

mehulkar commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsoltis commented Sep 12, 2022

Uh oh!

nathanhammond Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

mehulkar Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nathanhammond Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

mehulkar Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mehulkar commented Sep 15, 2022

Uh oh!

nathanhammond commented Sep 15, 2022

Uh oh!

Uh oh!

mehulkar commented Sep 10, 2022 •

edited by nathanhammond

Loading

vercel bot commented Sep 10, 2022 •

edited

Loading

mehulkar commented Sep 12, 2022 •

edited

Loading