Page MenuHomePhabricator

[upstream] Gerrit patchset-created event has been changed
Closed, ResolvedPublic

Description

When a patch is rebased via the Gerrit graphical interface, it sends an event of type patchset-created to stream-events listener. The event has an uploader field which used to be the person doing the action, since Gerrit 2.8.1 the uploader field points to the patch author.

An example is change https://gerrit.wikimedia.org/r/#/c/109891/ . The change has been authored and is owned by untrusted user Tinaj1234, hence tests are not run. I have rebased patchset 3 to trigger the tests did not get triggered.

Looking at Zuul debug log for Gerrit events, the change structure is roughly:

change:

owner:
  name: Tinaj1234
  project: mediawiki/core
patchset:
  author:
    name: Tinaj1234
  uploader:
    name: Tinaj1234
type: patchset-created
uploader:
  name: Tinaj1234

The uploader field should have pointed to myself, aka hashar. That was the behavior before our switch to Gerrit 2.8.1.

For each type of event we had a field representing the authenticated person doing the action. The corresponding map is:

'patchset-created': 'uploader',
'draft-published': 'uploader',  # Gerrit 2.5/2.6
'change-abandoned': 'abandoner',
'change-restored': 'restorer',
'change-merged': 'submitter',
'merge-failed': 'submitter',  # Gerrit 2.5/2.6
'comment-added': 'author',
'ref-updated': 'submitter',
'reviewer-added': 'reviewer',  # Gerrit 2.5/2.6

Aka for the event type 'patchset-created', the account that have done the action is mentioned in the 'uploader' field.


Version: wmf-deployment
Severity: normal
See Also:
http://code.google.com/p/gerrit/issues/detail?id=2493

Details

Reference
bz60781

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:07 AM
bzimport added projects: Gerrit, Upstream.
bzimport set Reference to bz60781.

I have not verified myself, but does this actually break things for
Jenkins/Zuul, or is it just a “nuissance”?

Since I'd forget otherwise:

fd23ab518fbd5a3a8c02badd8583ade36cb30edb
Refactor Rebase to use PatchSetInserter

is the first affected commit.

Change 115316 had a related patch set uploaded by QChris:
Set uploader to current user in "patchset-created" event upon rebasing

https://gerrit.wikimedia.org/r/115316

Upstream bug http://code.google.com/p/gerrit/issues/detail?id=2493 has been fixed by Christian (thus assigning bug to him since he did all the job).

Upstream commit is https://gerrit-review.googlesource.com/#/c/54850/

Should be included in Gerrit 2.8.2.

Backport of change in our Gerrit copy is proposed at https://gerrit.wikimedia.org/r/115316 I guess whenever it get deployed the issue will be solved :-]

Change 115316 merged by QChris:
Set uploader to current user in "patchset-created" event upon rebasing

https://gerrit.wikimedia.org/r/115316

It seems new gerrit has been deployed on 2014-03-10 23:30.
Does Zuul get the correct JSON objects now?

Yes. (Or at least grrrit-wm does.) Thank you!

This is still a problem when one uses the "Cherry Pick" button to cherry-pick a commit to the same branch (to remove dependencies):

[21:14] <grrrit-wm> (PS5) Prtksxna: Use tooltip role for Popups [extensions/Popups] - https://gerrit.wikimedia.org/r/120344

(I submitted that patchset using the method above, not Prtksxna.)

Change 125181 had a related patch set uploaded by QChris:
Set uploader to current user in "patchset-created" event upon cherry-picking

https://gerrit.wikimedia.org/r/125181

Thanks Chris that was quick. I am wondering whether the other events are suffering the same issue (i.e. abandoned patchset).

(In reply to Antoine "hashar" Musso from comment #12)

I am wondering whether the other events are
suffering the same issue (i.e. abandoned patchset).

I think we should be fine.

For the patchset-created event, I know three ways to trigger it
through the UI:

  • Rebasing a change (fixed)
  • Cherry-picking a change (fixed)
  • Editing commit message (worked already)

(If you know further ways, please let me know)

The draft-published event also has an uploader field. And it is not
set to the current user when publishing a draft. But that is arguably
ok, as the one who publishes a patch set is not necessarily the
uploader of the patch set. Documentation is a bit scarce on that end,
so it's hard to read off intentions from the gerrit devs. But as it's
ok for draft-published, I did not change it there.

All other events come without an uploader field, if grep did not fool
me.

I've been told that we probably will not deploy this fix right away, but
wait for gerrit 2.8.4 (which is just around the corner, and will include our
fix).

Change 125181 merged by Chad:
Set uploader to current user in "patchset-created" event upon cherry-picking

https://gerrit.wikimedia.org/r/125181

Fix has been deployed on 2014-07-08.