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 (option 1) #2346

Closed
wants to merge 20 commits into from

Conversation

beaesguerra
Copy link
Member

@beaesguerra beaesguerra commented Oct 15, 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 to use composite actions so that both node-ci-push and node-ci-pr can use the same config

See #2348 for an alternative where we use reusable workflows instead to share configuration.

Pros:

  • shared configuration for node-ci-push and node-ci-pr
  • no changes in names so the checks don't need to be updated
    Screenshot 2024-10-17 at 3 40 43 PM

Cons:

  • Since the actions are called within a step, the steps of the lint/test actions are not as clearly broken down in the logs
    Screenshot 2024-10-17 at 3 44 01 PM

Test Plan

  • Confirm linting and tests run in CI

…bled on feature branches (this is so feature branches can be updated with main)
@beaesguerra beaesguerra self-assigned this Oct 15, 2024
Copy link

changeset-bot bot commented Oct 15, 2024

⚠️ No Changeset found

Latest commit: e2a1fc2

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

Copy link
Contributor

github-actions bot commented Oct 15, 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 15, 2024

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-lzwkaoxtyl.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

@@ -68,3 +68,100 @@ jobs:
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: ./coverage/coverage-final.json

Copy link
Member Author

@beaesguerra beaesguerra Oct 15, 2024

Choose a reason for hiding this comment

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

These changes are based on chatting with Lilli on how to work with Intermediate Integration Branches (thanks for the help @lillialexis!). These changes will make it so main will include these checks and we can include commits from main into feature branches. Please let me know if there's another way to address this so that we're able to merge the latest changes from main into feature branches!

@beaesguerra beaesguerra marked this pull request as ready for review October 15, 2024 21:08
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Oct 15, 2024

Gerald

Required Reviewers
  • @Khan/wonder-blocks for changes to .github/workflows/node-ci-pr.yml, .github/workflows/node-ci-push.yml, .github/actions/lint/action.yml, .github/actions/test/action.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 15, 2024 21:08
Copy link
Contributor

github-actions bot commented Oct 15, 2024

npm Snapshot: NOT Published

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

@jandrade
Copy link
Member

@beaesguerra as we discussed offline. It would be worth exploring using reusable workflows to avoid repeating the jobs in two separate workflows.

@beaesguerra beaesguerra marked this pull request as draft October 17, 2024 16:12
@beaesguerra beaesguerra removed the request for review from a team October 17, 2024 18:52
@beaesguerra beaesguerra force-pushed the add-test-lint-checks-for-feature-branches branch from d8f84a1 to b956751 Compare October 17, 2024 19:19
@beaesguerra beaesguerra force-pushed the add-test-lint-checks-for-feature-branches branch from b956751 to 85e3439 Compare October 17, 2024 19:22
@beaesguerra beaesguerra force-pushed the add-test-lint-checks-for-feature-branches branch from 08a819b to c0a0a52 Compare October 17, 2024 19:38
- name: Typecheck
if: always() # always run this check until we update the eslint config
# if: steps.js-ts-files.outputs.filtered != '[]' || steps.typecheck-reset.outputs.filtered != '[]'
shell: bash
Copy link
Member Author

Choose a reason for hiding this comment

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

The action.yml files contain the same steps as before with the addition of shell: bash on the steps that had errors around requiring the shell config

- uses: actions/checkout@v4
- uses: ./.github/actions/test
env:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
Copy link
Member Author

Choose a reason for hiding this comment

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

One other difference between composite actions and reusing workflows is that secrets can only be accessed by actions using an input or environment variable, while reusable workflows can define secrets

Here, we pass the codecov token using an env variable. Let me know if there are any concerns with this!

@beaesguerra beaesguerra changed the title Include lint and test jobs for node-ci-push workflow Include lint and test jobs for node-ci-push workflow (option 1) Oct 17, 2024
@@ -35,36 +36,29 @@ jobs:
with:
node-version: ${{ matrix.node-version }}


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.

The coverage job can be removed since the test action covers test coverage too!

@beaesguerra beaesguerra marked this pull request as ready for review October 17, 2024 21:55
@khan-actions-bot khan-actions-bot requested a review from a team October 17, 2024 21:55
@beaesguerra
Copy link
Member Author

We will be moving forward with option 2 using reusable workflows. See #2348 for more details!

beaesguerra added a commit that referenced this pull request Oct 18, 2024
…workflow) (#2348)

## Summary:
- Lint and test jobs from [node-ci-pr.yml](https://github.com/Khan/wonder-blocks/blob/a32b0779acc9ed8dc742955160cc7748052296e8/.github/workflows/node-ci-pr.yml#L65-L160) 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](https://docs.github.com/en/actions/sharing-automations/reusing-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](https://github.com/Khan/wonder-blocks/actions/runs/11393035774/job/31700470290?pr=2348) for the different steps:
![Screenshot 2024-10-17 at 3 21 04 PM](https://github.com/user-attachments/assets/effeca62-4326-4626-bee0-02f43c984d19)


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](https://github.com/user-attachments/assets/baee2f1b-78a8-479c-a416-d07cc76972c9)

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](https://github.com/user-attachments/assets/e6749cc3-5b85-4e6d-933b-8217e8acf7cf)

Author: beaesguerra

Reviewers: beaesguerra, jandrade

Required Reviewers:

Approved By: jandrade

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), 🚫 Chromatic - Get results on regular PRs, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⌛ undefined, ⌛ undefined

Pull Request URL: #2348
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