Repository navigation
needs-ci label is confusing #1044
Description
This may start to be getting off topic, but I suspect we also need to explain the needs-ci label (or get rid of it or rename it). Specifically, I'm under the impression it should remain after the CI is run and green because it's not a label that means "this needs a Jenkins run in the future" but rather "this needs a Jenkins run for it to land because it modifies executable code". I think a lot of people think it means that first thing and remove it after Jenkins starts or after it's green.
It may be too late to change this, but I wonder if in general, labels that are primarily for the bot/Actions should be prefixed with something so that it's clearer.
Originally posted by @Trott in #1039 (comment)
I'm fairly certain that label was repurposed and it was originally meant to be the latter before the contribution process changed to only requiring GitHub action runs for doc-only PRs.
Originally posted by @richardlau in #1039 (comment)
I agree, we should either rename it to requires-Jenkins-CI or similar, or have some kind of prefixing. Opening a new issue as I think it deserves its own discussion
Activity
mmarchini commentedon Jun 23, 2021
How useful is the label still?
ncucan tell if Jenkins CI is needed or not now.aduh95 commentedon Jun 23, 2021
The idea was motivated by two things:
ljharb commentedon Jun 23, 2021
Couldn't that be achieved by a github status, which auto-succeeds if CI isn't required, and reports the CI status when it is required? (i guess that wouldn't cover the ability to override)
legendecas commentedon Jun 24, 2021
then it needs to re-run and update the status once Jenkins CI has been run for the PR?
ljharb commentedon Jun 24, 2021
The CI process itself can update the status on completion.
mhdawson commentedon Jul 4, 2023
@aduh95 is this issue still useful? Only asking because it's been a few years since the last discussion.
GeoffreyBooth commentedon Sep 6, 2023
Someone just triggered a fresh CI job on a PR of mine that had a passing CI, that I was waiting for the time limit to expire in order to land. Now we’re going to go into another cycle of “resume job” runs for the next several hours, because a collaborator misinterpreted “needs CI” to mean, well, that the PR needs CI to be run for it. We need to rename this terrible label ASAP.
Perhaps
ci-must-pass-to-land; and it would be nice if a bot automatically addedci-passedafter a passing CI, and removed it if additional commits get pushed.joyeecheung commentedon Sep 8, 2023
I think we can just remove it. The CI requirement is already shown at the button of the GitHub UI.
tniessen commentedon Sep 8, 2023
@joyeecheung Could you clarify what UI element you refer to? I must have never noticed that.
GeoffreyBooth commentedon Sep 8, 2023
Perhaps Joyee means the box above the merge button, that lists all the results of the GitHub Actions jobs? As in, if Actions jobs run, then by definition this PR needs CI.
joyeecheung commentedon Sep 8, 2023
Yes, although I now realize that there would just be GitHub actions. We could also just..switch it to
request-ci. Probably going to do more good than harm.joyeecheung commentedon Sep 8, 2023
Oh wait, someone can still run arbitrary code via a PR so it should not be
request-ciautomatically. Maybe we can instead modify the bot to say something smarter instead, it already comments to ping the reviewers anyway...we can even make it smarter, if the test only touches documentation, it can say this only requires doc + linter tests. If the test touches anything else it can say this requires full test. If the test touches deps/v8 it can say this requires v8-commit tests.joyeecheung commentedon Sep 8, 2023
Oh, if we can install this https://plugins.jenkins.io/build-with-parameters/ we can even let the bot generate a URL that fills out the build parameters automatically
joyeecheung commentedon Sep 8, 2023
I just noticed that needs-ci is used by
git node landto recognize PRs that needs full CI runs: nodejs/node#37308 (comment)GeoffreyBooth commentedon Sep 8, 2023
Why do we need this label at all, though? We require contributors to land PRs via
git nodeorcommit-queue, right, and both of those check that CI has run and passed before landing. So what use is this label—is it just to inform the PR author “hey, make sure you get a passing CI before you try to land this”?If that’s the case then maybe the bot that currently adds
needs-cicould instead post a comment “This PR will require passing CI to land” and ideally provide a link via https://plugins.jenkins.io/build-with-parameters/ to run CI with the proper parameters pre-filled.joyeecheung commentedon Sep 8, 2023
It was explained above #1044 (comment) though I'll say I've never seen anyone overriding the ncu judgement..
GeoffreyBooth commentedon Sep 8, 2023
Okay, so if the only goal we need to preserve is the other one, “help collaborators tell if a PR requires CI to land” then what if we flipped the label?
ci-unnecessaryon PRs that don’t need a CI run in order to land, and the default is to assume that every PR needs passing CI to land?aduh95 commentedon Sep 8, 2023
I've done it myself numerous times, for
typingsPRs for example, the bot is incapable of knowing it's only changing comments.joyeecheung commentedon Sep 12, 2023
Isn't the overwrite only in place to tell ncu that it NEEDS CI when ncu thinks it doesn't? The condition is written as
What the bot does is the other way around, it thinks more PRs need CI than necessary (your use case). Though I am pretty sure both cases can be made redundant if we just update the path rules both in ncu and the bot.
joyeecheung commentedon Sep 12, 2023
Also I hope that this is only talking about typings. Not comments in
lib/,src/ortest/. We had some pretty funny bugs before that could be "fixed by a comment" (usually GC-related so the repro just subtly depends on a comment, but it shows that a comment can affect flakiness of a test/expose a bug so they should need a full CI IMO).aduh95 commentedon Sep 12, 2023
Hum I've done it for all kinds of comments, is that real that a comment could affect code? Do you have an example?
The CQ will still refuse to land the PR if the path is one where a full CI run is usually mandatory. ncu will also warn that the PR doesn't look ready. So I guess it's true that removing the label is never actually useful, but it does communicate to other reviewers “please do not start a CI, it's not necessary, I'll land this manually“ (although this message is rarely understood, so…)
Adding the label to a PR that isn't on a path where the bot thinks it deserves a full CI run does communicate (to both bots and humans) that you think the PR should not land without it. If we remove the label, we remove that feature (but I reckon it's rarely used).
joyeecheung commentedon Sep 12, 2023
I present to you this magic bug that was witnessed by a table of Node.js TSC members and V8 team members (we were having a Node.js TSC & V8 dinner and thought hey why not fix some flakes as dessert) https://x.com/joyeecheung/status/1002281495505989632?s=46
GeoffreyBooth commentedon Sep 12, 2023
So can we just invert it? CI is assumed to be required by default, and the
ci-unnecessarylabel can be added if we want to override the script to tell it to not bother checking Jenkins?Or if CI is just always required, then I guess we also wouldn’t need the label. It would be nice if a more limited CI could be required for docs-only changes, but that can be a future improvement.
joyeecheung commentedon Sep 12, 2023
I think ci-unnecessary is a good idea. We can also use it for some kind of exceptional circumstances (like reverting X cause some test to fail mysteriously but we need it to fix a large regression ASAP? I hope that doesn’t happen but you never know)
mhdawson commentedon Oct 15, 2024
At this point its been over a year since the last discussion on this issue. I'm going to go ahead and close. Please let me know if that was not the right thing to do or feel free to reopen.