Skip to content

chore(ci): make cost-saving changes#8714

Open
aduh95 wants to merge 6 commits intonodejs:mainfrom
aduh95:cost-saving-ci
Open

chore(ci): make cost-saving changes#8714
aduh95 wants to merge 6 commits intonodejs:mainfrom
aduh95:cost-saving-ci

Conversation

@aduh95
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 commented Mar 11, 2026

Description

IIUC we don't need full blown runners for most jobs, let's see if the slim ones do the job

Refs: https://docs.github.com/en/billing/reference/actions-runner-pricing

Validation

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@aduh95 aduh95 requested a review from a team as a code owner March 11, 2026 09:39
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nodejs-org Ready Ready Preview Mar 11, 2026 11:58am

Request Review

@github-actions
Copy link
Copy Markdown
Contributor

👋 Codeowner Review Request

The following codeowners have been identified for the changed files:

Team reviewers: @nodejs/web-infra

Please review the changes when you have a chance. Thank you! 🙏

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.07%. Comparing base (ec16468) to head (552ed8b).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8714      +/-   ##
==========================================
- Coverage   75.10%   75.07%   -0.04%     
==========================================
  Files         104      104              
  Lines        9167     9167              
  Branches      315      316       +1     
==========================================
- Hits         6885     6882       -3     
- Misses       2280     2283       +3     
  Partials        2        2              

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why has this been split into two separate jobs, rather than a matrix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you look at the commit history, you'll see I've started with that exact approach, and gave up as I wasn't able to make it work. Open to suggestions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it looks like the runs failed with Error when evaluating 'strategy' for job 'build'. .github/workflows/build.yml (Line: 41, Col: 21): Unexpected value 'pull_request', .github/workflows/build.yml (Line: 42, Col: 15): Unexpected value 'nodejs/nodejs.org', which seems like it was just a syntax error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Like I said, open to suggestions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd probably start with keeping it as the matrix it was before, and switching to the slim runner? Then if that works, look at adding the other changes you made that exclude certain runs etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe that's just a me-thing, but your use of question mark on sentences that are clearly not questions is... frustrating. It reads to me as the equivalent of writing if(console.log('Hello')); instead of console.log('hello');.
We can discuss decoupling (and if someone wants to open an alternative PR, that's great), but the more pressing problems is that it looks like lots of jobs are not compatible with the slim runners.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They are questions. I am asking if that change can be done, as I would not like to land this as-is with the jobs being split out of what was a working matrix before, creating duplication.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any update here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I still need help to unblock the Error: EACCES: permission denied, open '/home/agent/post_event.json' failure

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a bit concerned about using the lightweight runners here, given we've struggled with long build times, and recently needed to switch to more powerful Vercel runners as well?

@bmuenzenmeyer
Copy link
Copy Markdown
Contributor

this looks interesting to me, more from a speed perspective than costs savings. pulling a smaller image should be faster in most circumstances.

@aduh95 i am naive to this - but Node.js or OpenJS Foundation doesn't actually pay GitHub for our CI costs, right?

@aduh95
Copy link
Copy Markdown
Contributor Author

aduh95 commented Apr 10, 2026

Node.js or OpenJS Foundation doesn't actually pay GitHub for our CI costs, right?

We do on the private fork used to prepare security releases – GH only lets public repo use GHA for free.

@bmuenzenmeyer
Copy link
Copy Markdown
Contributor

We do on the private fork used to prepare security releases – GH only lets public repo use GHA for free.

Yikes. That seems like a tax on security. I did some digging and found this explicitly mentioned on a Discussion Ticket: Investing in the security advisory experience on GitHub

Enabling CI on advisory workspace private forks

One of the longest-standing requests we hear is the inability to run GitHub Actions on the temporary private forks created for security advisories. Right now, maintainers can't run their CI pipeline against a security fix before merging, which means you're either merging untested patches or maintaining a separate private fork workflow outside of our advisory tooling.

We're actively working through the security model required to enable Actions on these workspaces safely. The core challenge is ensuring that embargoed vulnerability details can't leak through webhook payloads, third-party app integrations, or untrusted workflow execution. This is complex work with real security implications, and we're being deliberate about getting it right - the integrity of the embargo model is foundational to the trust that makes coordinated disclosure work.

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.

5 participants