Node: Discuss: should we require Travis and GitHub Actions to succeed before landing PRs

[mmarchini]

According to our current CI policy, before landing a Pull Request, only Jenkins jobs are required to succeed. While I imagine most of us are careful to not land a PR when Travis/Actions are red, our policy or tooling doesn't reflect that practice. Now that we have Travis and Actions enabled in the repository for a while, the question is: should we consider those as official CI options and consider them as well in our policy?

On one hand, adding more CI results to consider can make our policy more complex. On the other hand, if we treat Actions as an official CI, we can move some tests from Jenkins to here, simplifying our Jenkins infrastructure (for example, tests involving different ./configure options). We would probably still need to test on every platform on Jenkins though.

I don't have strong opinions either way, but I'd like to hear what others think.

[richardlau]

I personally liked the previous simplicity of only having one thing to check (the Jenkins CI run) but we've since allowed Travis CI for doc-only changes (https://github.com/nodejs/node/pull/30330). Our tooling hasn't caught up (which is why https://github.com/nodejs/node/issues/29770 is still open).

There are advantages to having things in Travis/Actions:

• It is much easier for collaborators to change the behaviour of a Travis/Actions job (doesn't require having permissions in Jenkins) and such changes have an audit trail (i.e. they are PR'ed like any other change)
• Run automatically on pull requests (don't need to be started by collaborators)
• Actions are better integrated with GitHub, allowing things like annotations (we're doing that now for any JavaScript lint failures)

The main downside is Travis/Actions doesn't cover all of our supported platforms.

I'm leaning towards doing less in Travis as it is more restrictive than Actions in terms of things like job running time limit (50 mins on Travis vs 6 hours on Actions) and concurrency (5 concurrent jobs for open source on Travis vs 20 on Actions).

[sam-github]

While I imagine most of us are careful to not land a PR when Travis/Actions are red

I rely heavily on git node land... when it says its good, I land it, I don't even look at github, so whatever our policy ends up being, +1000 on it being what the tooling checks!

[mhdawson]

I think if we have actions/travis jobs that run automatically for each PR then they should pass and be a blocker for landing, otherwise why are we running them?

[mmarchini]

I'm leaning towards doing less in Travis as it is more restrictive than Actions in terms of things like job running time limit (50 mins on Travis vs 6 hours on Actions) and concurrency (5 concurrent jobs for open source on Travis vs 20 on Actions).

Agreed. Also, keeping only one should make maintenance simpler (in theory at least).

I rely heavily on git node land... when it says its good, I land it, I don't even look at github, so whatever our policy ends up being, +1000 on it being what the tooling checks!

We already have the building blocks to check GitHub CI status on ncu, just need to make some changes so that it's possible to check both Jenkins and GitHub CI (we might need to fix something on github-bot first though).

[gengjiawen]

I am +1 on require those CI.

But travis sometimes being flaky like those https://travis-ci.com/github/nodejs/node/jobs/299678389, I see it mulit times.

[mmarchini]

I'll probably send a PR to remove Travis :eyes:

Just need to double-check that everything we have on Travis is covered on Actions.

[Trott]

I rely heavily on git node land... when it says its good, I land it, I don't even look at github, so whatever our policy ends up being, +1000 on it being what the tooling checks!

If I'm not mistaken, git node land checks that a CI was run, but does not check that it was green. So, uh, that's at least one thing you should check manually. :-D

[mmarchini]

Not sure if it should, but it could require the last CI to be successful before landing.

[targos]

IIRC, it checks that there is a CI: URL kind of command in the comments. What we could (and I think should) add to git node land is a verification using the GitHub checks API, with a prompt to allow landing anyway if everything is not green.

[Trott]

IIRC, it checks that there is a CI: URL kind of command in the comments. What we could (and I think should) add to git node land is a verification using the GitHub checks API, with a prompt to allow landing anyway if everything is not green.

It might be more robust to only check the green-or-not status of node-test-pull-request. It's often the case that a subtask (e.g., node-test-commit-linux-containered) still shows as red even after a newer run has passed.

[sam-github]

If I'm not mistaken, git node land checks that a CI was run, but does not check that it was green. So, uh, that's at least one thing you should check manually. :-D

git node land's green checkmarks strongly, strongly suggests that it has done that check, it should probably get fixed to make that clear. Its a mystery to me how I haven't landed anything incorrectly yet, perhaps I do tend to look for green in CI before dropping to the shell to land things.

[mmarchini]

git node land's green checkmarks strongly, strongly suggests that it has done that check, it should probably get fixed to make that clear

It's a blue i icon, indicating it's informative.

What we could (and I think should) add to git node land is a verification using the GitHub checks API

This is already implemented on ncu, but we need to check that a Jenkins CI was run (today we only check if a GitHub Checks is present). Also, bugs like this can hinder the results collected by ncu.

[mmarchini]

Latest node-core-utils version will have a red X if the last Jenkins CI failed (thanks @codebytere for adding it a few months ago!) and once https://github.com/nodejs/node-core-utils/pull/469 lands we'll also check for Actions, which should cover everything raised in this issue :)