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

Include lint and test jobs for node-ci-push workflow (using reusable workflow) #2348

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

beaesguerra
Copy link
Member

@beaesguerra beaesguerra commented Oct 17, 2024

Summary:

  • Lint and test jobs from node-ci-pr.yml are also run in the node-ci-push.yml workflow
    • this is done so that feature branches can be updated with the latest changes in main (there are currently branch protection rules on feature/* branches that require these status checks to pass)
  • Lint and test jobs are refactored into reusable workflows so that both node-ci-push and node-ci-pr can use the same config

See #2346 for an alternative where we use composite actions instead to share configuration

Update: We decided to use reusable workflows so that the logs are easier to read for the steps and we can use the secrets config (which isn't supported by composite actions). The branch rules have been updated to look for Lint / Lint and Test / Test checks.

Pros:

  • shared configuration for node-ci-push and node-ci-pr
  • easy to see the logs for the different steps:
    Screenshot 2024-10-17 at 3 21 04 PM

Cons:

  • since we are using another workflow, the names of the jobs are nested (ie. Node CI (PR) / Lint / Lint instead of just Node CI (PR) / Lint ) and the github checks will need to be updated (let me know though if there's a way around this!)
    Screenshot 2024-10-17 at 3 16 53 PM

Issue: WB-1778

Test plan:

  • Confirm linting and tests run in CI
  • Confirm steps properly fail when there are linting errors, typescript errors, and failing tests
    image

@beaesguerra beaesguerra self-assigned this Oct 17, 2024
Copy link

changeset-bot bot commented Oct 17, 2024

⚠️ No Changeset found

Latest commit: 89c9872

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR


coverage:
Copy link
Member Author

Choose a reason for hiding this comment

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

This job can be removed since the Test workflow also gathers coverage!

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Size Change: 0 B

Total Size: 100 kB

ℹ️ View Unchanged
Filename Size
packages/wonder-blocks-accordion/dist/es/index.js 3.78 kB
packages/wonder-blocks-banner/dist/es/index.js 1.53 kB
packages/wonder-blocks-birthday-picker/dist/es/index.js 1.77 kB
packages/wonder-blocks-breadcrumbs/dist/es/index.js 887 B
packages/wonder-blocks-button/dist/es/index.js 4.04 kB
packages/wonder-blocks-cell/dist/es/index.js 2.01 kB
packages/wonder-blocks-clickable/dist/es/index.js 3.06 kB
packages/wonder-blocks-core/dist/es/index.js 3.44 kB
packages/wonder-blocks-data/dist/es/index.js 6.24 kB
packages/wonder-blocks-dropdown/dist/es/index.js 18.2 kB
packages/wonder-blocks-form/dist/es/index.js 6.21 kB
packages/wonder-blocks-grid/dist/es/index.js 1.36 kB
packages/wonder-blocks-i18n/dist/es/index.js 4.76 kB
packages/wonder-blocks-icon-button/dist/es/index.js 3 kB
packages/wonder-blocks-icon/dist/es/index.js 828 B
packages/wonder-blocks-labeled-field/dist/es/index.js 72 B
packages/wonder-blocks-layout/dist/es/index.js 1.82 kB
packages/wonder-blocks-link/dist/es/index.js 2.28 kB
packages/wonder-blocks-modal/dist/es/index.js 5.36 kB
packages/wonder-blocks-pill/dist/es/index.js 1.65 kB
packages/wonder-blocks-popover/dist/es/index.js 4.87 kB
packages/wonder-blocks-progress-spinner/dist/es/index.js 1.52 kB
packages/wonder-blocks-search-field/dist/es/index.js 1.3 kB
packages/wonder-blocks-switch/dist/es/index.js 1.94 kB
packages/wonder-blocks-testing-core/dist/es/index.js 3.74 kB
packages/wonder-blocks-testing/dist/es/index.js 1.07 kB
packages/wonder-blocks-theming/dist/es/index.js 693 B
packages/wonder-blocks-timing/dist/es/index.js 1.8 kB
packages/wonder-blocks-tokens/dist/es/index.js 2.1 kB
packages/wonder-blocks-toolbar/dist/es/index.js 827 B
packages/wonder-blocks-tooltip/dist/es/index.js 7.08 kB
packages/wonder-blocks-typography/dist/es/index.js 1.23 kB

compressed-size-action

Copy link
Contributor

github-actions bot commented Oct 17, 2024

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-lthespwsvf.chromatic.com/

Chromatic results:

Metric Total
Captured snapshots 7
Tests with visual changes 0
Total stories 497
Inherited (not captured) snapshots [TurboSnap] 365
Tests on the build 372

token: ${{ secrets.CODECOV_TOKEN }}
files: ./coverage/coverage-final.json
uses: ./.github/workflows/node-ci-test.yml
secrets:
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, we are able to pass tokens using secrets

@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Oct 17, 2024

Gerald

Required Reviewers
  • @Khan/wonder-blocks for changes to .github/workflows/node-ci-lint.yml, .github/workflows/node-ci-pr.yml, .github/workflows/node-ci-push.yml, .github/workflows/node-ci-test.yml

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team October 17, 2024 21:55
Copy link
Contributor

github-actions bot commented Oct 17, 2024

npm Snapshot: NOT Published

🤕 Oh noes!! We couldn't find any changesets in this PR (edb0ac0). As a result, we did not publish an npm snapshot for you.

@beaesguerra beaesguerra changed the title Include lint and test jobs for node-cipush workflow (option 2) Include lint and test jobs for node-ci-push workflow (using reusable workflow) Oct 18, 2024
Copy link
Member

@jandrade jandrade left a comment

Choose a reason for hiding this comment

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

This is a great improvement! thanks for the detailed description and iterating on the problem 👏 🚀

@beaesguerra beaesguerra merged commit bb7f7cf into main Oct 18, 2024
65 of 67 checks passed
@beaesguerra beaesguerra deleted the try-workflow branch October 18, 2024 20:32
@beaesguerra
Copy link
Member Author

Added some notes around these findings here: https://khanacademy.atlassian.net/wiki/x/AYGUyQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants