search

gocd-contrib / gocd-build-status-notifier

GoCD build status notifier plugin
Apache License 2.0
47 stars 40 forks source link

Gerrit notification generates too many comments #6

Open jozefs opened 9 years ago

jozefs commented 9 years ago

Thanks for adding Gerrit support. I've been testing it over the past couple days.

One of the issues I noticed is that it seems like the notification plugin is adding two comments to the Gerrit change per GoCD stage that completes successfully ("-Code Review" and "Code Review +1/-1"). This was unexpected. I expected there to be exactly one comment on the changeset from Go, one +1 or -1 depending on whether the pipeline passed or failed, not one per stage. This has several issues:

  1. Email spam. With a four stage pipeline, this generates eight emails to every person on the reviewer list. Gerrit is already pretty chatty over email, and this makes it worse without seemingly adding any value.
  2. UI clutter. Again, with a four stage pipeline, this generates eight comments per patch set. If the change goes through five patch sets (not uncommon), the build notifier will generate 40 comments, completely overwhelming any comments left by human reviewers.
  3. I don't think the plugin needs to do a "-Code Review". Gerrit should be smart enough to flip the value of an existing label when it changes. This is what my curl workaround script does.

Is this a bug or a deliberate design decision?

For reference, this is the relevant part of my workaround script:

curl -XPOST -H "Content-Type: application/json" --digest --user [username]:[password] [gerrit-url]/a/changes/[gerrit-change-number]/revisions/[gerrit-change-revision]/review -d "{'message': '[message with link to failing build on GO]', 'labels': { 'Verified': [+1 or -1] } }"
srinivasupadhya commented 9 years ago

Thank you for trying it out & feedback comments. I used Code Review instead of Verified since the verified wasn't seem to be working. May be, as you said it need to be enabled. I agree with the comments & email spam problems. It wasn't thought of. When i tried it out on Gerrit server the email wasn't enabled & so dint face this issue. I will see how i can make these changes.