Thanks everyone! Responses below: On Mon, Oct 16, 2023 at 7:37 AM Uwe Schindler <u...@thetaphi.de> wrote:
> this seems to be a safety feature and is also enabled in general for > Github. I found no options in asf.yaml to enable/disable it: > OK, thanks for checking Uwe. > Nevertheless, I see no problem for pressing the button. When I quickly > review a PR, I generally press the button. > I press it as well, but this is just a "best effort" and leaves many runs unapproved for quite some time. When I check a few days ago, there were 52 pending runs (I think for 26 PRs, seems to be 2 runs per unapproved PR), ranging up to 19 days in age: https://github.com/apache/lucene/actions?query=is%3Aaction_required (I have since approved all of them). We committers are not consistent in checking all pending runs ... > For safety reasons this is required in most projects I was contributing, > too (not only ASF). > But this is a silly way to achieve safety -- it's "assume guilty until proven innocent" of our newest contributors, when past evidence that I've seen shows it's 100% the opposite: all of our new contributors opening PRs are not bad agents. Can't we instead assume innocent until proven guilty of our newest contributors? Sure, those of us with the karma to push the "Approve and run" don't see a problem: we long ago lost the fresh eyes / Shoshin that new contributors bring and experience. Uwe, put yourself in the shoes of a new contributor: you see a small issue, you know how to make PRs so you make one, submit it, and then no response. You see that other contributors' PRs quickly get this nice GitHub action catching problems, but for some reason yours does not (maybe for 19 days). (I think this "Approve and run" button is only seen by committers.) You're not sure what you did wrong, what you should do next. You feel like this community doesn't listen to new people's PRs. Some random time later, say 12 days, the jobs run, and now you see you made some silly mistake and you fix it and push to your PR. And, again, nothing happens to confirm you did fix the problems from the first run, for maybe another 6 days. You wonder why you had to wait 12 days to see the first silly mistake and another 6 to see the next... We should constantly strive to make the new contributor experience as wonderful / frictionless / responsive as we can, not the opposite (this approval step). Such brave new people is how our community grows. And we old timer committers are blind to the pains they feel. (Separately, we have another problem: gradually growing number of still-open PRs: https://home.apache.org/~mikemccand/lucenebench/github_pr_counts.html) > What's the problem in pressing the button? Of course you take > responsibility when the crypto miner starts, but if there is a huuuuuge PR > by an external contributor, I would first ask if they could split it into > smaller pieces. At some point we have to review it, and most external > people creating huge PRs did bad stuff like pressing the format button in > their IDE. > > I think running "./gradlew precommit" is a must for new contributors. The > online checks on Github are more for me as reviewer/committer, to make sure > all is fine before I press the merge button (for many PRs I don't even > checkout the code after review). So it is fine to not trigger it by > end-users. > New contributors don't necessarily know everything that we old-timers expect/know, like running precommit (they are not committers, yet!), or tidy. That's what's great about our Github actions -- they run that for the contributor an the contributor can quickly see what went wrong. Inserting committer approval there just gums up that whole nice feedback loop for a new contributor (up to 19 days!). > -1 to ask INFRA to enable this. > OK, I won't ask INFRA to change anything! On Mon, Oct 16, 2023 at 7:53 AM Robert Muir <rcm...@gmail.com> wrote: > I think running the builds with a timeout is a good thing to do > anyway, for any CI build. I'm sure github actions has some fancy yaml > for that, but you can just do "timeout -k 1m 1h ./gradlew..." instead > of "./gradlew" too. +1, that seems like a much better way to achieve safety without harming the new contributor onboarding experience. On Mon, Oct 16, 2023 at 11:02 AM Dawid Weiss <dawid.we...@gmail.com> wrote: > I filed a PR here - > https://github.com/apache/lucene/pull/12687 > Ooh, thank you Dawid! And it's now merged, so we now have a decent timeout protection, so if a bad actor tries to crypto mine or run some distributed LLM or whatever, at least the wasted resources are bounded by how long a "typical" legitimate run takes, plus generous buffer. So given this protection, why require the added manual approval step :) Net/net I don't think we have to do anything more here ... for now I'll try to make a periodic effort myself to approve & run these blocked jobs. Maybe that's enough to create a smoother first-contributor experience. But I still strongly disagree with intentionally harming the new contributor "first experience" process for the negligible chance that they are bad actors. That's anti-community. Mike McCandless http://blog.mikemccandless.com >
