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

chore: improve CI by making it a workflow graph #4959

Merged
merged 4 commits into from
May 18, 2022
Merged

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented May 11, 2022

While working on #4952 I split the linting job into multiple jobs.
I figured that we can do the same for all parts of our CI. With each step being its own job it means:

  • Each will be independently shown in the signals box. This means if one fails, it's immediately obvious which job failed without digging into logs.
    • This is useful on mobile where the GH app pushes you into the mobile browser to view action details.
  • One failure doesn't fail everything else - allowing you to see all passes/fails regardless of a single failure.
    • E.g. bad spelling shouldn't block the format checker, and one package's test shouldn't block another's.
  • All jobs are run in parallel on separate machines - automatically by GH.
    • This should save us a lot of time in the testing (hopefully around 6 mins).
    • This also means that you can see the results of smaller jobs earlier, without waiting for slower steps to finish.
      • E.g. format check is fast (~1m total), but linting is slow (~4m) - and seeing that signal earlier is nice.
  • We can block the build earlier by creating a hierarchy of jobs.
    • Right now if yarn is having issues then all jobs fail at once. Or sometimes some jobs might fail whilst others don't. Separating the install + cache into its own step will mean just the install job fails clearly and blocks everything. Similarly if there's a build failure - all jobs will fail.

This PR actions the above to create a workflow for our CI:

  1. Run the install and cache the install artefacts
    • This is the same as before, however now we do it as its own job so it's guaranteed to be done exactly once - there's no race condition where 2 jobs might do the install if they both run before the cache is filled.
  2. Build and (basic) Lint in Parallel
    a) Run the build and cache the output
    b) In parallel we also any steps that don't need the build:
    a) Prettier
    b) Markdownlint
    c) Spellcheck
  3. Run the build-dependent steps in parallel
    a) ESLint
    b) Typecheck
    c) Integration tests
    d) Website tests
    e) Unit tests for all packages on different node versions (each package / version is run as a separate job)
  4. Upload codecov results, collected from the main node version tests only.

I also:

  • moved the preparation steps into reusable actions so they're not copy+pasted everywhere
  • ran all of the unit tests using a matrix instead of in a monolithic job. this means we get one signal per package per node version (applying all the above justifications to our tests)

Comparison:

Before:
image

After:
image

@bradzacher bradzacher added the repo maintenance things to do with maintenance of the repo, and not with code/docs label May 11, 2022
@nx-cloud
Copy link

nx-cloud bot commented May 11, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f45de7e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@netlify
Copy link

netlify bot commented May 11, 2022

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit f45de7e
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/628541e1af13e200096d561a
😎 Deploy Preview https://deploy-preview-4959--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bradzacher bradzacher force-pushed the improve-ci-2022-05-10 branch 9 times, most recently from 13709c5 to 379ccf3 Compare May 11, 2022 06:44
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #4959 (f45de7e) into main (8cbbcc3) will decrease coverage by 2.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4959      +/-   ##
==========================================
- Coverage   93.93%   91.70%   -2.23%     
==========================================
  Files         175      361     +186     
  Lines        9904    12122    +2218     
  Branches     3139     3517     +378     
==========================================
+ Hits         9303    11117    +1814     
- Misses        354      657     +303     
- Partials      247      348     +101     
Flag Coverage Δ
unittest 91.70% <ø> (-2.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/scope-manager/src/scope/ScopeType.ts 100.00% <0.00%> (ø)
...ges/scope-manager/src/definition/DefinitionType.ts 100.00% <0.00%> (ø)
...ges/utils/src/ast-utils/eslint-utils/predicates.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/lib/es2017.full.ts 100.00% <0.00%> (ø)
...ages/scope-manager/src/lib/es2018.asynciterable.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/variable/index.ts 80.00% <0.00%> (ø)
packages/type-utils/src/typeFlagUtils.ts 83.33% <0.00%> (ø)
...ckages/utils/src/ts-eslint-scope/PatternVisitor.ts 100.00% <0.00%> (ø)
packages/scope-manager/src/ScopeManager.ts 77.21% <0.00%> (ø)
.../scope-manager/src/variable/ESLintScopeVariable.ts 100.00% <0.00%> (ø)
... and 176 more

@armano2
Copy link
Member

armano2 commented May 11, 2022

q: is there a reason why we are runnign tests in amtrix and all of them at once in separate task to?

image

image

@bradzacher
Copy link
Member Author

q: is there a reason why we are runnign tests in amtrix and all of them at once in separate task to?

@armano2 - to collect and upload the coverage
but I think that I found another solution for doing this via the matrix instead of in one job - https://github.com/codecov/example-actions-bundled

@bradzacher bradzacher changed the title chore: improve CI speed by reordering the workflow chore: improve CI by making it a workflow graph May 11, 2022
@bradzacher bradzacher force-pushed the improve-ci-2022-05-10 branch 5 times, most recently from 7628a21 to ac8b43a Compare May 11, 2022 21:24
@bradzacher bradzacher force-pushed the improve-ci-2022-05-10 branch from ac8b43a to ac2b486 Compare May 11, 2022 22:44
@bradzacher bradzacher marked this pull request as ready for review May 11, 2022 23:27
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks!

.github/workflows/ci.yml Show resolved Hide resolved
- name: Use Node.js ${{ env.PRIMARY_NODE_VERSION }}
uses: actions/setup-node@v3
- name: Checkout
uses: actions/checkout@v3
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there's an actions/checkout@v3 before each prepare-install, and the only difference is the fetch-depth sometimes being 2. Could the prepare-install composite action do the checkout as well, with fetch-depth as an input?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had this originally but because prepare-install is a local reusable action - you have to do the checkout before GH action can reference the file :(

If we wanted - we could move the reusable actions to a separate repo entirely and then GH actions could reference them without a checkout - but I figured for now we can just eat the duplication and punt on that decision.

.github/workflows/ci.yml Show resolved Hide resolved
armano2
armano2 previously approved these changes May 18, 2022
Copy link
Member

@armano2 armano2 left a comment

Choose a reason for hiding this comment

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

i do like this change and it seem that is working correctly, thank you for your work 🐱

i have one question about artifacts, do we have to clean them after coverage is uploaded?

I think there is storage limit on github, and we do not need them after they where submitted

@bradzacher
Copy link
Member Author

i have one question about artifacts, do we have to clean them after coverage is uploaded?

https://github.com/actions/upload-artifact#retention-period

Looks like by default it stores the artifact for 90 days!

I changed this repos' default retention to 14 days considering that we don't ever look at old logs or artifacts. Worst case someone can re-run the workflow to get fresh artifacts.

I'll also change the config here to 1 day retention for codecov artifacts (the minimum) to make sure we're not wasting space.

@bradzacher bradzacher dismissed stale reviews from armano2 and JoshuaKGoldberg via f45de7e May 18, 2022 18:58
@bradzacher bradzacher merged commit 08ae2c4 into main May 18, 2022
@bradzacher bradzacher deleted the improve-ci-2022-05-10 branch May 18, 2022 19:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants