[RFC] Proposal to change Stream Event slightly.

[Khai Do]

Hello Gerriters,

We use Gerrit stream events extensively and we would like to see the label info (i.e. 'approvals' field) on every commentAdded event because we want our CI system to trigger off of that info[1].  Currently we cannot do that because the commentAdded event only provides the label info when there is a vote transition (or change):

 review comment with no vote change -> commentAddedEvent does not contains label info

 review comment with a vote change -> commentAddedEvent contains label info

  

My change proposal[2] was to simply add the label info to the event on every comment regardless of the vote.  However the problem with this change is that it breaks things that look for the existence or absence of the label info as an indication of a changed vote (i.e. Jenkins gerrit-trigger plugin).

I think the fact that the gerrit-trigger plugin needs to depend on an implicit indicator to determine vote transitions is not real nice and can be buggy.  My proposal to fix this is to add an explicit indicator to the commentAdded event for vote transitions so that things outside of gerrit can reliably determine that a vote has changed.  So what would that event look like?  Possible solutions:

A. Bjorn's proposal is to add a separate 'allLabels' field to the event.  The 'approvals' continue to only be in commmentAdded events that contained a vote transition.  The 'allLabels' field would appear in every commentAdded event showing the label and vote info.

 

  Stream-event:

    "approvals":[{"type":"Code-Review","description":"Code-Review","value":"-1"], "allLabels":[{"type":"Code-Review","value":"-1"}], "comment":"Patch Set 1: Code-Review+1\n\nbar"  

  Pros:

    Preserves backwards compatibility with the gerrit-trigger pliugin and possibily other third party things.

    

  Cons:

    Introduce a small amount of duplicate information (but normally the number of labels per vote is small)

    Vote transition indicator is still implicit.

    Probably not a big deal, but makes event less concise.

B.  Sven's proposal is to add an 'updated' field to the approvals list for every label. The 'approvals' list would be in every commentAdded event and include an 'updated' field to indicate whether a vote change occurred.

  Stream-event:

    "approvals":[{"type":"Code-Review","description":"Code-Review","value":"-1", "updated":"true"}],"comment":"Patch Set 1: Code-Review+1\n\nbar"  

  Pros:

    Vote transition info is explicit.  gerrit-trigger can just check this flag which may make it more reliable.

    'updated' field indicates the specified label that was updated.

    No duplicate info

    Keeps event concise

  

  Cons:

    Breaks backwards compatibility with the gerrit-trigger pliugin and possibly other third party things.

I prefer option A. but wanted to get feedback just to see if there were strong opinions either way.  I'm also open to other suggestions as well.

[1] https://code.google.com/p/gerrit/issues/detail?id=3220

[2] https://gerrit-review.googlesource.com/#/c/68242

[Zaro]

Correction, I meant that I prefer option B.

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Sven Selberg]

With suggestion B In case of Gerrit trigger, I think backwards compatibility is not a big issue.

From what I can see, if nothing is done in gerrit-events the 'updated' flag wont change a thing, it will simply not get parsed and gerrit-trigger would be none the wiser [1] so nothing "should" blow up with a  Gerrit version with an 'updated' flag in its CommentAddedEventand older gerrit-trigger plugin.

And IIRC gerrit-trigger keeps track of the version of the Gerrit server and should be able to handle the extra 'updated' field elegantly enough even with an updated plugin connected to a Gerrit version without an 'updated' flag in its CommentAddedEvent.

https://github.com/sonyxperiadev/gerrit-events/blob/master/src/main/java/com/sonymobile/tools/gerrit/gerritevents/dto/attr/Approval.java

/Sven

[Björn Pedersen]

Well, 

gerrit-trigger allows to trigger once a specific label gets set. This is done by looking at the pass 'Approvals', so if the meaning of  this field changes, it will only be possible to use a version that supports the new 'updated' flag.
(see https://github.com/jenkinsci/gerrit-trigger-plugin/blob/master/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java#L875 ).
I do not see problems for old gerrit versions with new plugin, but for new gerrit versions with old plugin.

Björn

[Sven Selberg]

You are absolutely right. I did not consider that older plugin versions have no way of knowing when the meaning will change in later Gerrit versions.

A change would require an upgrade of the gerrit-trigger plugin for those who wish to connect to a Gerrit instance with the new stream event.

IMHO it would suffice to address this in the release-notes.

/Sven

[David Pursehouse]

On Monday, 7 September 2015 18:28:06 UTC+9, Sven Selberg wrote:

You are absolutely right. I did not consider that older plugin versions have no way of knowing when the meaning will change in later Gerrit versions.

A change would require an upgrade of the gerrit-trigger plugin for those who wish to connect to a Gerrit instance with the new stream event.

IMHO it would suffice to address this in the release-notes.

Agreed.  As long as the plugin gets the necessary update and a new version is released in time to be deployed at the same time as the new Gerrit.

[Khai Do]

I believe the proposed changes for this are ready on the Gerrit side: 

  https://gerrit-review.googlesource.com/#/c/68242

  https://gerrit-review.googlesource.com/#/c/71051

[Khai Do]

This is now ready on the Jenkins gerrit-trigger-plugin side as well:

 https://github.com/sonyxperiadev/gerrit-events/pull/48

 https://github.com/jenkinsci/gerrit-trigger-plugin/pull/253