doc: improve the process for landing a PR

[jakecastelli]

Even though incident could still happen like #54554 (which was partially my fault and I learned the lesson), I wanted to share my thoughts and hope to reduce the potential breakage in the future. As I noticed the other PR in net landed and I should've taken a closer look, or even just re-trigger the CI when in doubt, both approaches would help me to catch the test failure earlier.

Wording is not my expertise, happy for any suggestions 👍 or even just any thoughts on this potentially ongoing issue.

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[aduh95]

I’m not sure it’s worth adding this, more often than not the CQ label is added to the PR hours before it actually lands because of our wait time requirements, and checking the commits at the time won’t tell you more info than the CI already does. We already have a requirement for CI to not be older than 2 weeks, that’s the requirement we should amend if anything – and IMO we don’t need to change anything, it’s not like broken CI on main is a recurring thing

[marco-ippolito]

I agree there is probably no need to take action since its very rare

[jakecastelli]

Thanks for reviewing, I agree with you. Since this is rarely going to happen and the recovery is not costly (either revert or fast track a fix), I think it is probably not worthing the extra burden to do the check.

[targos]

Instead, I think we should look into using GitHub's commit queue system (which I think can ensure that tests pass after applying the commits in order).

[jakecastelli]

Instead, I think we should look into using GitHub's commit queue system (which I think can ensure that tests pass after applying the commits in order).

That's not a bad suggestion! We could exclude subsystems e.g. doc, meta etc. to make things slightly faster.

[aduh95]

Instead, I think we should look into using GitHub's commit queue system (which I think can ensure that tests pass after applying the commits in order).

Or run tests as part of our own CQ system. I don't think this can reasonably work on GHA runners, we'd need to use a runner that can cache build results so most build take instants instead of about one hour currently, plus another hour for running the tests.

[tniessen]

Instead, I think we should look into using GitHub's commit queue system (which I think can ensure that tests pass after applying the commits in order).

That was the original design idea for the commit queue, many years ago. The main problem is how slow and flaky our CI has been.

[avivkeller]

Suggested change

	6. Ideally, check the recent commits on `main` to evaluate any potential
	6. Ideally, check the recent commits on the base branch to evaluate any potential

[jakecastelli]

main and the base branch in this context are synonym to me, I will treat this as a nit if that's ok

[avivkeller]

Sure! FWIW the base branch refers to whatever branch the PR is targeting, so I think it makes more sense, as the PR could also be targeting vX.x-staging, etc.

[mcollina]

I don't think this is feasible. It's already hard to land something as is.

[jakecastelli]

The main problem is how slow and flaky our CI has been

A full CI run would be rough, but a quick test run (on windows, macOS, Linux) hopefully would be sufficient.

[aduh95]

@jakecastelli do you still want to pursue this? There seems to be quite a few concerns with this PR:

• it sets unrealistic standards for landing a PR.
• it won't be very effective (the CQ label is sometimes added hours or even days before a PR actually lands).
• there might be better solutions (e.g. have the CQ run the test suites when PR is labelled with needs-ci).
• the current situation is "good enough" IMO (at least I know that I will 100% of the time take the risk of not doing that optional step you're adding, and I suspect most folks would do the same, so what's the point of having it in our docs I wonder).

Note that this PR is technically ready to land if you decide to ignore those concerns (there's at least one approving review, and no one is blocking it), so it's up to you.

[jakecastelli]

Thanks for summarising the concerns and feedback @aduh95, I have decided not to proceed with landing, I left it open for a bit just to see if there were other folks thoughts or feedback. I think I am in a position to close it as:

1. I agree this would add extra burden to land a PR with very little benefit.
2. We can always revert a faulty commit, maybe it would block some other PRs from landing for day or two, it won't be catastrophic.
3. There are potentially better solutions with automated toolings.

Appreciated all the reviews and feedback 🙏