Regression / Changed behavor on commit-added stream event in 3.7.0

170 views
Skip to first unread message

Anders Hanson

unread,
Nov 23, 2022, 4:36:08 PM11/23/22
to Repo and Gerrit Discussion
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]

The comment field in the comment-added stream-event is now "Patch Set 1:\n\n(1 comment)"

I'm not sure that this is intended or not, but it makes it impossible to trigger CI checks on comment-added, unless we have missed something. The code that was removed has a todo comment about that is should be removed when patch set level comments are exposed in comment added event.


/Anders Hanson

Edwin Kempin

unread,
Nov 24, 2022, 3:37:44 AM11/24/22
to Anders Hanson, Repo and Gerrit Discussion
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]
How did you find that this change broke this?

The handling of the publishPatchSetLevelComment option was not lost, but it was just moved into a different class (moved from PostReview into PostReviewOp).

I tried to reproduce this problem from a new test in [2], but it seems this is still working as expected?

 

The comment field in the comment-added stream-event is now "Patch Set 1:\n\n(1 comment)"

I'm not sure that this is intended or not, but it makes it impossible to trigger CI checks on comment-added, unless we have missed something. The code that was removed has a todo comment about that is should be removed when patch set level comments are exposed in comment added event.


/Anders Hanson

--
--
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.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/77291ce8-e710-4d71-93fb-a11a4476a119n%40googlegroups.com.

Anders Hanson

unread,
Nov 24, 2022, 8:34:11 AM11/24/22
to Repo and Gerrit Discussion
torsdag 24 november 2022 kl. 09:37:44 UTC+1 skrev eke...@google.com:
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]
How did you find that this change broke this?

Ouch, I was probably too tired yesterday and missed it.
However, the comment-added stream-event that is sent has the comment field set to comment": "Patch Set 1:\n\n(1 comment)"
Even when I add a message in the comment box when doing a reply.

I've looked bit more at the code and it looks like PostReviewOp. insertMessage(ChangeContext ctx) has the code that adds the exact
text that we get in the stream event. It does that in the if msg.isEmpty() block. I'm not that familiar with the Gerrit code base and might
look at the wrong place.

It looks like the problem is that the in.message is not populated from the draft comments when the UI is does  the post review rest call.
The content of that call looks like

{"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0,"Gating":0},"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"reviewers":[]}

A draft call was made right before with the following content
{"patch_set":1,"message":"recheck","unresolved":false,"path":"/PATCHSET_LEVEL","__unsaved":true}

Anders Hanson

unread,
Nov 24, 2022, 4:31:01 PM11/24/22
to Edwin Kempin, Repo and Gerrit Discussion


On 2022-11-24 14:46, Edwin Kempin wrote:


On Thu, Nov 24, 2022 at 2:34 PM Anders Hanson <anders....@gmail.com> wrote:


torsdag 24 november 2022 kl. 09:37:44 UTC+1 skrev eke...@google.com:
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]
How did you find that this change broke this?

Ouch, I was probably too tired yesterday and missed it.
However, the comment-added stream-event that is sent has the comment field set to comment": "Patch Set 1:\n\n(1 comment)"
Even when I add a message in the comment box when doing a reply.

I've looked bit more at the code and it looks like PostReviewOp. insertMessage(ChangeContext ctx) has the code that adds the exact
text that we get in the stream event. It does that in the if msg.isEmpty() block. I'm not that familiar with the Gerrit code base and might
look at the wrong place.

It looks like the problem is that the in.message is not populated from the draft comments when the UI is does  the post review rest call.
The content of that call looks like

{"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0,"Gating":0},"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"reviewers":[]}

A draft call was made right before with the following content
{"patch_set":1,"message":"recheck","unresolved":false,"path":"/PATCHSET_LEVEL","__unsaved":true}

I'm not much familiar with the publishPatchSetLevelComment option, but its name suggests that it only works for patch set level comments, aka change messages (so what's provided by the message field in CommentInput, which is not set in your example). I don't think it ever worked for inline comments, nor is it supposed to work for them (draft comments are always inline comments).
 
I've been able to reproduce it , using docker and starting both gerritcodereview/gerrit:3.6.3 and gerritcodereview/gerrit:3.7.0 with all defaults.
The only thing I did was to add my ssh key, browsed repositories, picked All-Users, picked "Edit Repo Config", added a space, saved & published the change.
I then started the review and after that I replied and added "recheck" in the edit box  and clicked SEND

The difference is that 3.6.3 is never using the draft endpoint.

The json for draft and review is as below for 3.7.0

draft: {"patch_set":1,"message":"recheck","unresolved":false,"path":"/PATCHSET_LEVEL","__unsaved":true}
review: {"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0},"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"reviewers":[]}

While it looks like below for 3.6.3

review: {"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0},"ready":true,"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"comments":{"/PATCHSET_LEVEL":[{"message":"recheck","unresolved":false}]},"reviewers":[]}

The stream-events for 3.6.3 the comment is "Patch Set 1:\n\nrecheck" while 3.7.0 has comment  "Patch Set 1:\n\n(1 comment)"



 
The handling of the publishPatchSetLevelComment option was not lost, but it was just moved into a different class (moved from PostReview into PostReviewOp).

I tried to reproduce this problem from a new test in [2], but it seems this is still working as expected?

 

The comment field in the comment-added stream-event is now "Patch Set 1:\n\n(1 comment)"

I'm not sure that this is intended or not, but it makes it impossible to trigger CI checks on comment-added, unless we have missed something. The code that was removed has a todo comment about that is should be removed when patch set level comments are exposed in comment added event.


/Anders Hanson
--
--
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.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/77291ce8-e710-4d71-93fb-a11a4476a119n%40googlegroups.com.
--
--
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.

Luca Milanesio

unread,
Nov 24, 2022, 4:36:39 PM11/24/22
to Repo and Gerrit Discussion, Luca Milanesio, Edwin Kempin, Anders Hanson
Thanks @Anders for reproducing the issue: I am glad that the Docker containers helped creating a test environment for it :-)

Can you create an issue on:

Luca.

Anders Hanson

unread,
Nov 24, 2022, 4:56:21 PM11/24/22
to Luca Milanesio, Repo and Gerrit Discussion, Edwin Kempin

Luca Milanesio

unread,
Nov 24, 2022, 6:59:16 PM11/24/22
to Repo and Gerrit Discussion, Luca Milanesio, Edwin Kempin, Anders Hanson
Thanks Anders,
I was able to follow your instructions and verify the bug locally \o/

As I mentioned in the ticket, I believe I SAW IT BEFORE on another release … and I’ve fixed it !!
Interestingly, the fix got lost somewhere …

Luca.

Luca Milanesio

unread,
Nov 24, 2022, 8:10:47 PM11/24/22
to Repo and Gerrit Discussion, Luca Milanesio, Edwin Kempin, Anders Hanson
On 24 Nov 2022, at 23:58, Luca Milanesio <luca.mi...@gmail.com> wrote:

On 24 Nov 2022, at 21:56, Anders Hanson <anders....@gmail.com> wrote:



On 2022-11-24 22:36, Luca Milanesio wrote:
On 24 Nov 2022, at 21:30, Anders Hanson <anders....@gmail.com> wrote:



On 2022-11-24 14:46, Edwin Kempin wrote:
On Thu, Nov 24, 2022 at 2:34 PM Anders Hanson <anders....@gmail.com> wrote:
torsdag 24 november 2022 kl. 09:37:44 UTC+1 skrev eke...@google.com:
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOpwas created

No, it is an existing bug and it should be fixed with:

The GUI didn’t use drafts before for the “Reply” button, but it does in v3.7 and hence the existing bug came out :-)

Luca.

Edwin Kempin

unread,
Nov 28, 2022, 2:20:43 AM11/28/22
to Anders Hanson, Repo and Gerrit Discussion
On Thu, Nov 24, 2022 at 2:34 PM Anders Hanson <anders....@gmail.com> wrote:


torsdag 24 november 2022 kl. 09:37:44 UTC+1 skrev eke...@google.com:
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]
How did you find that this change broke this?

Ouch, I was probably too tired yesterday and missed it.
However, the comment-added stream-event that is sent has the comment field set to comment": "Patch Set 1:\n\n(1 comment)"
Even when I add a message in the comment box when doing a reply.

I've looked bit more at the code and it looks like PostReviewOp. insertMessage(ChangeContext ctx) has the code that adds the exact
text that we get in the stream event. It does that in the if msg.isEmpty() block. I'm not that familiar with the Gerrit code base and might
look at the wrong place.

It looks like the problem is that the in.message is not populated from the draft comments when the UI is does  the post review rest call.
The content of that call looks like

{"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0,"Gating":0},"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"reviewers":[]}

A draft call was made right before with the following content
{"patch_set":1,"message":"recheck","unresolved":false,"path":"/PATCHSET_LEVEL","__unsaved":true}

I'm not much familiar with the publishPatchSetLevelComment option, but its name suggests that it only works for patch set level comments, aka change messages (so what's provided by the message field in CommentInput, which is not set in your example). I don't think it ever worked for inline comments, nor is it supposed to work for them (draft comments are always inline comments).
 

 
The handling of the publishPatchSetLevelComment option was not lost, but it was just moved into a different class (moved from PostReview into PostReviewOp).

I tried to reproduce this problem from a new test in [2], but it seems this is still working as expected?

 

The comment field in the comment-added stream-event is now "Patch Set 1:\n\n(1 comment)"

I'm not sure that this is intended or not, but it makes it impossible to trigger CI checks on comment-added, unless we have missed something. The code that was removed has a todo comment about that is should be removed when patch set level comments are exposed in comment added event.


/Anders Hanson

--
--
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.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/77291ce8-e710-4d71-93fb-a11a4476a119n%40googlegroups.com.

--
--
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.

Luca Milanesio

unread,
Nov 28, 2022, 4:51:58 AM11/28/22
to Repo and Gerrit Discussion, Luca Milanesio, Anders Hanson, Edwin Kempin

On 24 Nov 2022, at 13:46, 'Edwin Kempin' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:



On Thu, Nov 24, 2022 at 2:34 PM Anders Hanson <anders....@gmail.com> wrote:


torsdag 24 november 2022 kl. 09:37:44 UTC+1 skrev eke...@google.com:
On Wed, Nov 23, 2022 at 10:36 PM Anders Hanson <anders....@gmail.com> wrote:
We upgraded to 3.7.0 last weekend and are now receiving reports that our CI (Zuul-CI)
is not triggering builds when people add a comment with the text "recheck".
This worked before the upgrade when we were on 3.6.2.

I tried to track down the cause of this and think that I've found the problem.
It looks like the code that were in PostReview that handled the publishPatchSetLevelComment option was lost when  PostReviewOp was created
and code was moved out from PostReview. The change that did this is [1]
How did you find that this change broke this?

Ouch, I was probably too tired yesterday and missed it.
However, the comment-added stream-event that is sent has the comment field set to comment": "Patch Set 1:\n\n(1 comment)"
Even when I add a message in the comment box when doing a reply.

I've looked bit more at the code and it looks like PostReviewOp. insertMessage(ChangeContext ctx) has the code that adds the exact
text that we get in the stream event. It does that in the if msg.isEmpty() block. I'm not that familiar with the Gerrit code base and might
look at the wrong place.

It looks like the problem is that the in.message is not populated from the draft comments when the UI is does  the post review rest call.
The content of that call looks like

{"drafts":"PUBLISH_ALL_REVISIONS","labels":{"Code-Review":0,"Gating":0},"ignore_automatic_attention_set_rules":true,"add_to_attention_set":[],"remove_from_attention_set":[],"reviewers":[]}

A draft call was made right before with the following content
{"patch_set":1,"message":"recheck","unresolved":false,"path":"/PATCHSET_LEVEL","__unsaved":true}

I'm not much familiar with the publishPatchSetLevelComment option, but its name suggests that it only works for patch set level comments, aka change messages (so what's provided by the message field in CommentInput, which is not set in your example). I don't think it ever worked for inline comments, nor is it supposed to work for them (draft comments are always inline comments).

It looks like in Gerrit v3.7 (and master branch) the patch-set level comments issued by the UI are first saved as drafts and then published.
That is done transparently from the UI, without intention from the user to do so.

I’ve created a change on All-Projects and just added a patch-set level comment, see below the httpd_log entries:

172.17.0.1 [HTTP-123] - admin [2022-11-28T09:48:04.276Z] "PUT /changes/All-Projects~1/revisions/2/drafts HTTP/1.1" 201 192 23 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36" 20 20 1039432 -
172.17.0.1 [HTTP-120] - admin [2022-11-28T09:48:04.464Z] "POST /changes/All-Projects~1/revisions/2/review HTTP/1.1" 200 49 96 - "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36" 63 60 3837584 -

I’ve also posted a tentative fix at:

I need to add more tests though before it is ready for review.
Also, I’d like to dig into the change that triggered this behavioural change, because I’ve missed it in the release notes :-(

Luca.

Reply all
Reply to author
Forward
0 new messages