Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bradzacher merged 4 commits intomainfromimprove-ci-2022-05-10
May 18, 2022

Conversation

bradzacher
Copy link
Member

@bradzacherbradzacher commentedMay 11, 2022
edited
Loading

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 obviouswhich 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 sometimessome 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

JoshuaKGoldberg, Josh-Cena, and armano2 reacted with rocket emoji
@bradzacherbradzacher added the repo maintenancethings to do with maintenance of the repo, and not with code/docs labelMay 11, 2022
@nx-cloud
Copy link

nx-cloudbot commentedMay 11, 2022
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitf45de7e. 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 💌 fromNxCloud.

@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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@netlify
Copy link

netlifybot commentedMay 11, 2022
edited
Loading

Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitf45de7e
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/628541e1af13e200096d561a
😎 Deploy Previewhttps://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 yourNetlify site settings.

@bradzacherbradzacherforce-pushed theimprove-ci-2022-05-10 branch 9 times, most recently from13709c5 to379ccf3CompareMay 11, 2022 06:44
@codecov
Copy link

codecovbot commentedMay 11, 2022
edited
Loading

Codecov Report

Merging#4959 (f45de7e) intomain (8cbbcc3) willdecrease coverage by2.22%.
The diff coverage isn/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
FlagCoverage Δ
unittest91.70% <ø> (-2.23%)⬇️

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

Impacted FilesCoverage Δ
packages/scope-manager/src/scope/ScopeType.ts100.00% <0.00%> (ø)
...ges/scope-manager/src/definition/DefinitionType.ts100.00% <0.00%> (ø)
...ges/utils/src/ast-utils/eslint-utils/predicates.ts100.00% <0.00%> (ø)
packages/scope-manager/src/lib/es2017.full.ts100.00% <0.00%> (ø)
...ages/scope-manager/src/lib/es2018.asynciterable.ts100.00% <0.00%> (ø)
packages/scope-manager/src/variable/index.ts80.00% <0.00%> (ø)
packages/type-utils/src/typeFlagUtils.ts83.33% <0.00%> (ø)
...ckages/utils/src/ts-eslint-scope/PatternVisitor.ts100.00% <0.00%> (ø)
packages/scope-manager/src/ScopeManager.ts77.21% <0.00%> (ø)
.../scope-manager/src/variable/ESLintScopeVariable.ts100.00% <0.00%> (ø)
... and176 more

@armano2
Copy link
Collaborator

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
MemberAuthor

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 Ithink that I found another solution for doing this via the matrix instead of in one job -https://github.com/codecov/example-actions-bundled

armano2 reacted with hooray emoji

@bradzacherbradzacher changed the titlechore: improve CI speed by reordering the workflowchore: improve CI by making it a workflow graphMay 11, 2022
@bradzacherbradzacherforce-pushed theimprove-ci-2022-05-10 branch 5 times, most recently from7628a21 toac8b43aCompareMay 11, 2022 21:24
@bradzacherbradzacher marked this pull request as ready for reviewMay 11, 2022 23:27
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesMay 18, 2022
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg 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!

- name: Use Node.js ${{ env.PRIMARY_NODE_VERSION }}
uses: actions/setup-node@v3
- name: Checkout
uses: actions/checkout@v3

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 anactions/checkout@v3 before eachprepare-install, and the only difference is thefetch-depth sometimes being2. Could theprepare-install composite action do the checkout as well, withfetch-depth as aninput?

bradzacher reacted with thumbs up emoji
Copy link
MemberAuthor

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

JoshuaKGoldberg reacted with thumbs up emoji
armano2
armano2 previously approved these changesMay 18, 2022
Copy link
Collaborator

@armano2armano2 left a comment
edited
Loading

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 reacted with thumbs up emoji
@bradzacher
Copy link
MemberAuthor

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.

armano2 and JoshuaKGoldberg reacted with thumbs up emoji

@bradzacherbradzacher merged commit08ae2c4 intomainMay 18, 2022
@bradzacherbradzacher deleted the improve-ci-2022-05-10 branchMay 18, 2022 19:46
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsJun 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@armano2armano2armano2 approved these changes

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

@JamesHenryJamesHenryAwaiting requested review from JamesHenry

Assignees
No one assigned
Labels
repo maintenancethings to do with maintenance of the repo, and not with code/docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@bradzacher@armano2@JoshuaKGoldberg

[8]ページ先頭

©2009-2025 Movatter.jp