chore: address issue 1836 action items#1840
Conversation
✅ Deploy Preview for fdc3 canceled.
|
39d40b2 to
74ad9bc
Compare
|
/easycla |
- Update Node test matrix to [22, 24, 25] in cve-scanning and coverage workflows - Pin GitHub Actions dependencies - Add CodeCov coverage badge to README.md - Add CodeQL workflow for OpenSSF scorecard
|
/easycla |
4aa7dc7 to
f408c80
Compare
|
Hi @kriswest I've opened this PR to address the remaining action items from the April 8th maintainers meeting (#1836). Specifically, this PR covers: Updated the Node.js test matrix to include Node 22, 24, and 25 (deprecating Node 20) README.md |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
kriswest
left a comment
There was a problem hiding this comment.
Thanks for having a go at this. There are a couple of small adjustments to make + we'll need to figure out why the coverage and CVE scanning jobs are failing.
- Remove '// trigger' comment from webpack.config.js - Pin codeql-action steps to commit SHA - Simplify cve-scanning to use node 22 only (no matrix)
|
Hi @kriswest, thank you for the feedback! I've addressed all the requested changes:
Could you also let me know what's failing in the coverage and CVE scanning jobs so I can help investigate? Happy to make any further adjustments. |
kriswest
left a comment
There was a problem hiding this comment.
This looks good to me now - we just need to figure out why the workflows are failing...
|
@bingenito this PR is handling a couple of the action items you and I were going to look at after the maintainers meeting (CodeQL, dependency pinning etc., node test matrix). However, its failing the coverage and CVE scanning checks on what looks like token permissions issues? Is that something you could take a look at and advise on (as that was the remaining item you'd offered to look at - and probably understand a lot better than I)? |
|
Thanks @kriswest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1840 +/- ##
==========================================
+ Coverage 95.65% 95.68% +0.02%
==========================================
Files 69 69
Lines 4631 4631
Branches 714 807 +93
==========================================
+ Hits 4430 4431 +1
+ Misses 201 200 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 |
There was a problem hiding this comment.
These got downgraded. We really should add a dependabot config at least for actions if we don't want to do npm yet. I suspect this older version will also not work with oidc.
| - name: Codecov | ||
| uses: codecov/codecov-action@v6 | ||
| with: | ||
| use_oidc: true |
There was a problem hiding this comment.
We lost this. It has to be sent.
| uses: codecov/codecov-action@v6 | ||
| with: | ||
| use_oidc: true | ||
| uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4 |
There was a problem hiding this comment.
Since we have a build matrix, we will now have an issue that it will upload coverage for each entry of the build matrix overwriting the last. One fix for this is to upload with a to codecov that includes the matrix variable in it.
|
|
||
| permissions: | ||
| contents: read | ||
| id-token: write |
There was a problem hiding this comment.
This is required when adding back the oidc which was removed erroneously. It can be put at a lower level, but it has to be here in combination with oidc for codecov upload to work without tokens.
| uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 | ||
| with: | ||
| node-version: 20 | ||
| node-version: 22 |
There was a problem hiding this comment.
If we change this to 24 (the publishing node version), we can remote the NODE_AUTH_TOKEN from the npm publish env variables and it will use oidc and trusted publishing for free. Trusted publishing requires a new version of npm with node 24.
There was a problem hiding this comment.
I'm definitely OK with moving forward to node 24 (and trusted publishing) sooner rather than later.
Resolves #1826
Resolves #1827