TSC
TSC copied to clipboard
nodejs
`needs-ci` label is confusing
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 https://github.com/nodejs/TSC/issues/1039#issuecomment-866973522
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 https://github.com/nodejs/TSC/issues/1039#issuecomment-866998284
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
How useful is the label still? ncu can tell if Jenkins CI is needed or not now.
The idea was motivated by two things:
- help collaborators tell if a PR requires CI to land (without to have to run ncu).
- give the ability to collaborators to override ncu judgement (e.g., if a PR only changes a comment in a file, they could remove the label to notify that the PR doesn't actually need a full CI run).
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)
and reports the CI status when it is required?
then it needs to re-run and update the status once Jenkins CI has been run for the PR?
@aduh95 is this issue still useful? Only asking because it's been a few years since the last discussion.
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 added ci-passed after a passing CI, and removed it if additional commits get pushed.
I think we can just remove it. The CI requirement is already shown at the button of the GitHub UI.
The CI requirement is already shown at the button of the GitHub UI.
@joyeecheung Could you clarify what UI element you refer to? I must have never noticed that.
@joyeecheung Could you clarify what UI element you refer to? I must have never noticed that.
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.
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.
Oh wait, someone can still run arbitrary code via a PR so it should not be request-ci automatically. 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.
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
I just noticed that needs-ci is used by git node land to recognize PRs that needs full CI runs: https://github.com/nodejs/node/pull/37308#issuecomment-777088445
Why do we need this label at all, though? We require contributors to land PRs via git node or commit-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-ci could 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.
Why do we need this label at all, though?
It was explained above https://github.com/nodejs/TSC/issues/1044#issuecomment-867073408 though I'll say I've never seen anyone overriding the ncu judgement..
It was explained above #1044 (comment) though I’ll say I’ve never seen anyone overriding the ncu judgement..
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-unnecessary on 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?
I'll say I've never seen anyone overriding the ncu judgement..
I've done it myself numerous times, for typings PRs for example, the bot is incapable of knowing it's only changing comments.
I've done it myself numerous times, for typings PRs for example, the bot is incapable of knowing it's only changing comments.
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
this.pr.labels.nodes.some((l) => l.name === 'needs-ci') ||
this.requiresJenkinsRun() // Checks directories
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.
the bot is incapable of knowing it's only changing comments.
Also I hope that this is only talking about typings. Not comments in lib/, src/ or test/. 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).
Hum I've done it for all kinds of comments, is that real that a comment could affect code? Do you have an example?
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.
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).
Hum I've done it for all kinds of comments, is that real that a comment could affect code? Do you have an example?
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
So can we just invert it? CI is assumed to be required by default, and the ci-unnecessary label 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.
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)