-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore(actions): use gov repo for reused actions #7904
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
d0538ec
to
d9bab77
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7904 +/- ##
==========================================
- Coverage 73.15% 73.14% -0.02%
==========================================
Files 95 95
Lines 8355 8355
Branches 218 218
==========================================
- Hits 6112 6111 -1
- Misses 2242 2243 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
d9bab77
to
cd4dcef
Compare
Lighthouse Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes GitHub Actions and governance docs by removing local copies and reusing workflows and actions from the nodejs/web-team
repository.
- Added a link to the external Governance Document in README and removed the local GOVERNANCE.md
- Updated all workflows to use the shared
setup-environment
action or reuse workflows fromnodejs/web-team
- Cleaned up CODEOWNERS and removed the old governance-related workflows and scripts
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
README.md | Added [Governance Document] link and reference |
GOVERNANCE.md | Removed local governance file |
.github/workflows/translations-sync.yml | Replaced setup steps with shared setup-environment action |
.github/workflows/scorecard.yml | Replaced inline steps with reusable workflow invocation |
.github/workflows/publish-packages.yml | Swapped setup steps for shared environment action |
.github/workflows/playwright.yml | Swapped setup steps for shared environment action |
.github/workflows/playwright-cloudflare-open-next.yml | Swapped setup steps for shared environment action |
.github/workflows/notify-on-push.yml | Replaced Slack step with reusable notify-on-push action |
.github/workflows/lint-and-tests.yml | Swapped setup steps for shared environment action |
.github/workflows/find-inactive-collaborators.yml | Removed this workflow entirely |
.github/workflows/dependency-review.yml | Replaced inline dependency review with reusable workflow |
.github/workflows/codeql.yml | Replaced inline CodeQL setup with reusable workflow |
.github/workflows/chromatic.yml | Swapped setup steps for shared environment action |
.github/workflows/build.yml | Swapped setup steps for shared environment action |
.github/scripts/report-inactive-collaborators.mjs | Removed reporting script |
.github/CODEOWNERS | Removed GOVERNANCE.md ownership entry |
Comments suppressed due to low confidence (3)
.github/workflows/scorecard.yml:26
- When invoking a reusable workflow, you must include a ref (e.g.,
@main
) to pin to a specific version:uses: nodejs/web-team/.github/workflows/scorecard.yml@main
.
uses: nodejs/web-team/.github/workflows/scorecard.yml
.github/workflows/dependency-review.yml:18
- Add a ref to the reusable workflow usage (e.g.,
@main
) so that the workflow invocation is versioned and stable:uses: nodejs/web-team/.github/workflows/dependency-review.yml@main
.
uses: nodejs/web-team/.github/workflows/dependency-review.yml
.github/workflows/codeql.yml:17
- Include a specific ref on the reusable workflow invocation (e.g.,
@main
) and verify that the target file supports reusable workflow inputs.
uses: nodejs/web-team/.github/workflows/codeql.yml
|
||
- name: Setup Node.js | ||
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 | ||
- uses: nodejs/web-team/actions/setup-environment@main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use hashes here or since it is ours it is safe enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like, since we control that repository, we can assume that it's safe. The only people with write
permissions to it also have write
permissions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure then that also there are branch protection rules and the same rules for only allowing certain actions rules, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cannot assume another repo is safe. this is a huge vulnerability
- malicious upstream
- disgruntled contributor
- compromised developer machine
https://blog.rafaelgss.dev/why-you-should-pin-actions-by-commit-hash
Typically, that's the case, but I feel like since we are scoping the permissions almost identically to the ones here, the risk of compromising that repository is identical to the risk of compromising this repository. If an attacker somehow gained permissions on that repo via a leaked access token, for example, they would already have access in this repository. |
points taken, but that doesn't necessarily make me feel much better. the indirection of this repo and setup in the name of simplification and reuse only serves to make our environment harder to understand, and IMO, less secure in the long run. but now i am dredging up past disagreements and should self-moderate. i will lift my block if the majority approves of this approach... @RafaelGSS - I invoked your blog post before - curious if you have any wisdom that could help us here |
I'll add the explicit commits. We can always change it the future. |
cd4dcef
to
7164365
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d recommend leaving this as a fallback and mentioning in this file that the governance file has been moved elsewhere, to avoid a 404 error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this file has many (any?) backlinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 We now have
nodejs/web-team
, which'll contain our re-used actions.This should make our lives a bit easier, as we don't need to update a thousand workflows whenever something changes.