Last modified: 2013-08-02 13:19:35 UTC
gerrit-wm should always relay human approval/submit/+2/"C: 2", even when there's "(no comment)" (removed by bug 35427 I believe). On the contrary, people don't seem interested in mere +1/-1 (and -2?), per bug 40020 comment 1. I don't know if those are or should be relayed in some other feed (of the kind #mediawiki-codereview used to be and #mediawiki-feed is not yet). It would be a separate request.
(In reply to comment #0) > gerrit-wm should always relay human approval/submit/+2/"C: 2", even when > there's "(no comment)" (removed by bug 35427 I believe). Bug 37047, actually. I agree. Suppressing notifications for reviews that didn't include a comment is generally nice to reduce spam. But the CR+2 events should be reported. When we originally disabled reporting of reviews without comment this wasn't a problem because CR+2 would be CR+2 & submit. Nowadays the submissions are done by jenkins-bot and which means the average CR+2 event no longer includes submission, which means it gets caught in the filter from bug 37047.
Changing subject to include bug 46450, as they are closely related and it doesn't make sense to address them separately.
*** Bug 46450 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > Changing subject to include bug 46450, as they are closely related and it > doesn't make sense to address them separately. Ok; I filed separately just because one side decreases messages (as asked on bug 35427, bug 35538, bug 37047, bug 46282) and the other increases it. :) This bug is hopefully uncontroversial, though.
I'm wondering if we could maybe have jenkins/zuul use suexec[0] for the submit. This would solve both this problem as well as the problem of merges being attributed to jenkins instead of the person who +2'd. [0] https://gerrit.wikimedia.org/r/Documentation/cmd-suexec.html
(In reply to comment #5) > I'm wondering if we could maybe have jenkins/zuul use suexec[0] for the > submit. > This would solve both this problem as well as the problem of merges being > attributed to jenkins instead of the person who +2'd. > > [0] https://gerrit.wikimedia.org/r/Documentation/cmd-suexec.html And in turn that would avoid us useless bot comments like 23942 comment 14 ("Change 65453 merged by jenkins-bot") that we're getting since bug 47622 was fixed, I suppose. Is there any easier solution? If yes it would be nice to fix this specific bug for IRC and file those byproducts separately.
The part about adding +2 CR is done now with grrrit-wm: 18.42 < grrrit-wm> (CR) Nikerabbit: [ C: 2] Remove call to wfArrayMerge [extensions/Translate] - https://gerrit.wikimedia.org/r/72237 (owner: PleaseStand) Still have to remove the jenkins-bot message: 18.43 < grrrit-wm> (Merged) jenkins-bot: Remove call to wfArrayMerge [extensions/Translate] - https://gerrit.wikimedia.org/r/72237 (owner: PleaseStand)
So I currently filter out all 'MERGED' messages that are *not* from Jenkins-bot, since they are going to be preceded immediately by a C+2. I can instead not show any MERGED messages at all, and only report any merge failures. Is that good?
Since the merge is asynchronous and potentially delayed, I personally find more value in knowing when it is merged than knowing when the CR+2 was given. Also, some repositories don't have automated merge yet. So hiding the MERGED for non-jenkins is bad I think. If anything, I especially want to know when someone manually merged something by passing jenkins. Or, if it is a repo without an automated gate-and-submit, those will be the only MERGED messages there are (e.g. various ops repos and non-wmf-deployed mw extension repos). For consistency I'd show both jenkins and non-jenkins merges. For the long term we should see if we can make Zuul use suexec and/or faux the log message in gerrit-wm to show the username of the person who last CR+2'ed (defaulting to the real merger, jenkins-bot, if there was none of whatever reason).
(In reply to comment #8) > So I currently filter out all 'MERGED' messages that are *not* from > Jenkins-bot, since they are going to be preceded immediately by a C+2. I can > instead not show any MERGED messages at all, and only report any merge > failures. Is that good? Yes, IMHO: the actual merge is notified to the reviewers by email, that's more than enough as a check. However, what time does this "currently" refer to? This was 20 min after it: 21.27 < grrrit-wm> (Merged) jenkins-bot: ContentHandler: Fix a typo [core] - https://gerrit.wikimedia.org/r/76305 (owner: MarkAHershberger)
By 'filter out' I mean that they are not shown. Only jenkins-bot merges are shown, since as Krinkle mentioned they're asynchronous.
(In reply to comment #9) > For the long term we should see if we can make Zuul use suexec and/or faux > the > log message in gerrit-wm to show the username of the person who last CR+2'ed > (defaulting to the real merger, jenkins-bot, if there was none of whatever > reason). Luckily I don't think we'll need suexec anymore. There's already be On-Behalf-Of permissions added for Verified/CR, I don't see any reason we couldn't extend that for Submit. I'd sleep much better at night knowing Jenkins didn't have full suexec :)
(In reply to comment #4) > (In reply to comment #2) > > Changing subject to include bug 46450, as they are closely related and it > > doesn't make sense to address them separately. > > Ok; I filed separately just because one side decreases messages (as asked on > bug 35427, bug 35538, bug 37047, bug 46282) and the other increases it. :) > This bug is hopefully uncontroversial, though. Now it turns out it isn't, so I'm restoring its original scope (which is now fixed) and reopening bug 46450.
