Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
> > 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. We can write a bot to do this. Why do it manually? https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28#approve-a-workflow-run-for-a-fork-pull-request - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
Thanks everyone! Responses below: On Mon, Oct 16, 2023 at 7:37 AM Uwe Schindler 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 huge 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 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 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
Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
I filed a PR here - https://github.com/apache/lucene/pull/12687 Dawid On Mon, Oct 16, 2023 at 7:53 PM Dawid Weiss wrote: > > It's actually as simple as adding: > > timeout-minutes: xyz > > to workflows. > > > https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes > > I use it elsewhere for jobs on Windows because they tend to hang sometimes > (for reasons unknown to me). > > Dawid > > > On Mon, Oct 16, 2023 at 4:53 PM Robert Muir 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. >> >> On Mon, Oct 16, 2023 at 9:58 AM Michael McCandless >> wrote: >> > >> > When a non-committer (I think?) opens a PR, one of the committers must >> notice it and click Approve & Run so the contributor can find out if >> something broke in our automated tests/precommit/linting. >> > >> > This seems like a waste, and a friction in the worst possible place for >> our community: new contributor onboarding experience. >> > >> > I think we have it to prevent e.g. a crypto mining bot of a PR sneaking >> in and taking tons of resources to mine dogecoin or so? >> > >> > But 1) that doesn't seem to be happening so far, 2) when I hit "Approve >> & Run" I never look closely to see if there is in fact a hidden crypto >> miner in there, and 3) can't we just put some reasonable timeout on the >> GitHub actions to block such abuse? >> > >> > Is this some sort of requirement by GitHub, or did we choose to turn on >> this silly step? >> > >> > Mike McCandless >> > >> > http://blog.mikemccandless.com >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >> For additional commands, e-mail: dev-h...@lucene.apache.org >> >>
Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
It's actually as simple as adding: timeout-minutes: xyz to workflows. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes I use it elsewhere for jobs on Windows because they tend to hang sometimes (for reasons unknown to me). Dawid On Mon, Oct 16, 2023 at 4:53 PM Robert Muir 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. > > On Mon, Oct 16, 2023 at 9:58 AM Michael McCandless > wrote: > > > > When a non-committer (I think?) opens a PR, one of the committers must > notice it and click Approve & Run so the contributor can find out if > something broke in our automated tests/precommit/linting. > > > > This seems like a waste, and a friction in the worst possible place for > our community: new contributor onboarding experience. > > > > I think we have it to prevent e.g. a crypto mining bot of a PR sneaking > in and taking tons of resources to mine dogecoin or so? > > > > But 1) that doesn't seem to be happening so far, 2) when I hit "Approve > & Run" I never look closely to see if there is in fact a hidden crypto > miner in there, and 3) can't we just put some reasonable timeout on the > GitHub actions to block such abuse? > > > > Is this some sort of requirement by GitHub, or did we choose to turn on > this silly step? > > > > Mike McCandless > > > > http://blog.mikemccandless.com > > - > To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: dev-h...@lucene.apache.org > >
Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
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. On Mon, Oct 16, 2023 at 9:58 AM Michael McCandless wrote: > > When a non-committer (I think?) opens a PR, one of the committers must notice > it and click Approve & Run so the contributor can find out if something broke > in our automated tests/precommit/linting. > > This seems like a waste, and a friction in the worst possible place for our > community: new contributor onboarding experience. > > I think we have it to prevent e.g. a crypto mining bot of a PR sneaking in > and taking tons of resources to mine dogecoin or so? > > But 1) that doesn't seem to be happening so far, 2) when I hit "Approve & > Run" I never look closely to see if there is in fact a hidden crypto miner in > there, and 3) can't we just put some reasonable timeout on the GitHub actions > to block such abuse? > > Is this some sort of requirement by GitHub, or did we choose to turn on this > silly step? > > Mike McCandless > > http://blog.mikemccandless.com - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
Re: Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
Hi, 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: https://cwiki.apache.org/confluence/display/INFRA/Git+-+.asf.yaml+features#Git.asf.yamlfeatures-GitHubsettings You can only add some users to a whitelist of "collaborators" through asf.yaml. Nevertheless, I see no problem for pressing the button. When I quickly review a PR, I generally press the button. For safety reasons this is required in most projects I was contributing, too (not only ASF). What's the problem in pressing the button? Of course you take responsibility when the crypto miner starts, but if there is a huge 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. -1 to ask INFRA to enable this. Uwe Am 16.10.2023 um 15:57 schrieb Michael McCandless: When a non-committer (I think?) opens a PR, one of the committers must notice it and click Approve & Run so the contributor can find out if something broke in our automated tests/precommit/linting. This seems like a waste, and a friction in the worst possible place for our community: new contributor onboarding experience. I think we have it to prevent e.g. a crypto mining bot of a PR sneaking in and taking tons of resources to mine dogecoin or so? But 1) that doesn't seem to be happening so far, 2) when I hit "Approve & Run" I never look closely to see if there is in fact a hidden crypto miner in there, and 3) can't we just put some reasonable timeout on the GitHub actions to block such abuse? Is this some sort of requirement by GitHub, or did we choose to turn on this silly step? Mike McCandless http://blog.mikemccandless.com -- Uwe Schindler Achterdiek 19, D-28357 Bremen https://www.thetaphi.de eMail:u...@thetaphi.de
Can we get rid of "Approve & Run" on GitHub PRs by new contributors (non-committers)?
When a non-committer (I think?) opens a PR, one of the committers must notice it and click Approve & Run so the contributor can find out if something broke in our automated tests/precommit/linting. This seems like a waste, and a friction in the worst possible place for our community: new contributor onboarding experience. I think we have it to prevent e.g. a crypto mining bot of a PR sneaking in and taking tons of resources to mine dogecoin or so? But 1) that doesn't seem to be happening so far, 2) when I hit "Approve & Run" I never look closely to see if there is in fact a hidden crypto miner in there, and 3) can't we just put some reasonable timeout on the GitHub actions to block such abuse? Is this some sort of requirement by GitHub, or did we choose to turn on this silly step? Mike McCandless http://blog.mikemccandless.com