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

fix(ci): run GH action on external PRs as well #112

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Dec 31, 2024

Summary

Add pull_request to GHA on config

Details

#110 and #111 are not running CI because this was missing per #109 (comment)

@agilgur5
Copy link
Owner Author

agilgur5 commented Dec 31, 2024

I figured the old Node versions might fail; for reference Node 14 fails to install:

Run actions/setup-node@v3
Attempting to download 14.x...
Not found in manifest. Falling back to download directly from Node
Error: Unable to find Node version '14.x' for platform darwin and architecture arm64.

I'd like to keep Node 16 for now, although it is EoL, to ensure continuous testing throughout the range. Will bump that later too.

Using 16.x and 18.x though it looks like node-canvas is again having install failures 😕 . From 16.x on macOS.

Run npm ci
[...]
npm ERR! node-pre-gyp info using [email protected] | darwin | arm64
npm ERR! node-pre-gyp info check checked for "/Users/runner/work/react-signature-canvas/react-signature-canvas/node_modules/canvas/build/Release/canvas.node" (not found)
npm ERR! node-pre-gyp http GET https://github.com/Automattic/node-canvas/releases/download/v2.9.1/canvas-v2.9.1-node-v93-darwin-unknown-arm64.tar.gz
npm ERR! node-pre-gyp ERR! install response status 404 Not Found on https://github.com/Automattic/node-canvas/releases/download/v2.9.1/canvas-v2.9.1-node-v93-darwin-unknown-arm64.tar.gz 
npm ERR! node-pre-gyp WARN Pre-built binaries not installable for [email protected] and [email protected] (node-v93 ABI, unknown) (falling back to source compile with node-gyp) 
npm ERR! node-pre-gyp WARN Hit error response status 404 Not Found on https://github.com/Automattic/node-canvas/releases/download/v2.9.1/canvas-v2.9.1-node-v93-darwin-unknown-arm64.tar.gz

And with 18.x on Ubuntu:

npm error node-pre-gyp info using [email protected] | linux | x64
npm error node-pre-gyp info check checked for "/home/runner/work/react-signature-canvas/react-signature-canvas/node_modules/canvas/build/Release/canvas.node" (not found)
npm error node-pre-gyp http GET https://github.com/Automattic/node-canvas/releases/download/v2.9.1/canvas-v2.9.1-node-v108-linux-glibc-x64.tar.gz
npm error node-pre-gyp ERR! install response status 404 Not Found on https://github.com/Automattic/node-canvas/releases/download/v2.9.1/canvas-v2.9.1-node-v108-linux-glibc-x64.tar.gz 

Afaict, per Automattic/node-canvas#2025 (comment), this likely requires updating node-canvas.

As a less intrusive change, I'd like to try 16.x on Ubuntu, since that worked on a retry.
Can update Node + node-canvas to newer versions in a future PR then after that passes (for continuous range testing purposes, per above)

agilgur5 added a commit that referenced this pull request Dec 31, 2024
@agilgur5 agilgur5 added the kind: internal Changes only affect the internals and not the API or usage label Dec 31, 2024
@agilgur5
Copy link
Owner Author

As a less intrusive change, I'd like to try 16.x on Ubuntu, since that worked on a retry.

Ok this is working for now. Codecov status check isn't reporting seemingly b/c the old main report doesn't exist anymore due to cache age (Missing Head Commit), but this change is CI-only (no source code) and can manually confirm that 100% test coverage remains

@agilgur5 agilgur5 changed the title fix(ci): run GH action on PRs as well fix(ci): run GH action on external PRs. temp pin Node 16 on Ubuntu Dec 31, 2024
@agilgur5 agilgur5 changed the title fix(ci): run GH action on external PRs. temp pin Node 16 on Ubuntu fix(ci): run GH action on external PRs + temp pin Node 16 on Ubuntu Dec 31, 2024
@agilgur5 agilgur5 changed the title fix(ci): run GH action on external PRs + temp pin Node 16 on Ubuntu fix(ci): run GH action on external PRs as well Dec 31, 2024
@agilgur5
Copy link
Owner Author

I split off the Node/OS version pinning to a separate PR #113 as it's independent of GH actions running on external PRs.

@agilgur5
Copy link
Owner Author

Codecov status check isn't reporting seemingly b/c the old main report doesn't exist anymore due to cache age (Missing Head Commit)

Hmm, this is still happening even with #113 being merged and CI running on the merged commit.
Codecov isn't listing the latest commit either, although it is seeing the PRs.

I think this is because Codecov seemingly no longer supports tokenless coverage report uploads? The docs and GH Action both require a token now?

EDIT: looks like there's some requirements for tokenless now

@agilgur5
Copy link
Owner Author

EDIT: looks like there's some requirements for tokenless now

Split out another PR to get this working: #114

- `push` works for all PRs from my own branches, but not for PRs from forks from external contributors
  - see also https://github.com/agilgur5/mst-persist/blob/4f8b9f116d1645112ac2eb790d41f007916b9ff0/.github/workflows/ci.yml#L2
Copy link

codecov bot commented Dec 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (599f371) to head (925daf9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #112   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           75        75           
  Branches         9         9           
=========================================
  Hits            75        75           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

✅ CI checks all green now, see split PRs #113 and #114 for more details on the fixes that were necessary

@agilgur5 agilgur5 merged commit ec4e365 into main Dec 31, 2024
4 checks passed
@agilgur5 agilgur5 deleted the fix-ci-for-prs branch December 31, 2024 21:09
agilgur5 added a commit that referenced this pull request Jan 3, 2025
fix(ci): run GH action on PRs as well

- `push` works for all PRs from my own branches, but not for PRs from forks from external contributors
  - see also https://github.com/agilgur5/mst-persist/blob/4f8b9f116d1645112ac2eb790d41f007916b9ff0/.github/workflows/ci.yml#L2

(cherry picked from commit ec4e365)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: internal Changes only affect the internals and not the API or usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant