Page MenuHomePhabricator

Allow hiding of non-discussion comments in Gerrit
Closed, ResolvedPublic

Description

In Gerrit patchsets, comments tend to fall into three categories: 1) the user uploading new patchsets, 2) jenkins verifying the patchset, 3) actual code review comments.

This leads to quite a mind-boggling interface. Example lengthy patch (gerrit 27022):

As a code reviewer, only the highlighted comments would be of any interest.

Recent versions of gerrit support hiding non-comments (1) and also allow hiding CI comments (2) as long as they are tagged (this requires setting a tag field when submitting the change; see docs). The Zuul code creating the review comments would have to be updated to support this.

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 1:18 AM
bzimport added a project: Gerrit.
bzimport set Reference to bz46148.
bzimport added a subscriber: Unknown Object (MLST).

Just for reference, the screenshot seems to stem from
https://gerrit.wikimedia.org/r/#/c/27022

as a code reviewer, only the highlighted comments would be of any
interest

Why would code reviewing want ignore jenkins output?
Why would code reviewing want ignore newer patch sets?

One solution maybe could be to [...] have the comments be separated
into sections based on patchset.

Comments specific to a patch set are typically made right in the code
(See for example patch set 7 of the referenced change).

To me, comments visible on the change screen are typically high level
comments that rather focus on the change itself than on a specific
detail that's only relevant to a single patch set. Even if such
comments were made when patch set 7 was current, the warnings to
check against ConfirmEdit are still relevant when reviewing patch set

  1. So when tying such comments to a patch set, they'd probably get

lost.

To me, it's a huge benefit to see all the high level comments in a
complete timeline.

While I do not have any concrete metrics for our gerrit instance,
it seems to me that changes with more than a dozen patch sets and
being actively worked on more than five months after opening the
change are rather the exception.

(In reply to comment #1)

Just for reference, the screenshot seems to stem from
https://gerrit.wikimedia.org/r/#/c/27022

as a code reviewer, only the highlighted comments would be of any
interest

Why would code reviewing want ignore jenkins output?
Why would code reviewing want ignore newer patch sets?

It's not that you want to ignore them, it's that they're not comments and yet still mix in as comments. When I want to jump into a patchset and see what discussion is going on, I don't care if Jenkins couldn't merge patchset 6.

demon removed a subscriber: demon.Dec 8 2014, 6:25 PM
Aklapper triaged this task as Lowest priority.Mar 23 2015, 6:59 PM
saper removed a subscriber: saper.Mar 23 2015, 7:57 PM

Gerrit 2.14 seems to include a way for you to hide tagged comments. For example editing a commit msg, you can click the hide tagged comments button and it will hide the comments mentioning you edited the commit msg.

Paladox moved this task from Bugs & stuff to Maybe fixed? on the Gerrit board.Aug 8 2017, 1:19 PM

Gerrit 2.14 seems to include a way for you to hide tagged comments. For example editing a commit msg, you can click the hide tagged comments button and it will hide the comments mentioning you edited the commit msg.

Should this task be closed then? (since it seems it's already possible)

Dalba added a subscriber: Dalba.Mar 14 2018, 6:56 AM
Tgr added a subscriber: Tgr.EditedApr 1 2018, 9:11 PM

There is a "Hide tagged comments" (old UI) / "show comments only" (new UI) option that is supposed to hide the clutter. On gerrit-review.googlesource.com that hides CI comments too, but on gerrit.wikimedia.org it does not. From the code it seems the option will hide any comment that's tagged, so we should tag Jenkins comments.

(Note: comment tags are different from git tags; from T37534: Free-form tagging in gerrit aka hashtags; and from robot comments.)

Paladox added a comment.EditedApr 1 2018, 9:13 PM

This will require a change to zuul. To get it to tag comments.

Tgr updated the task description. (Show Details)Apr 1 2018, 9:21 PM
Paladox removed a subscriber: wikibugs-l-list.
Paladox added a comment.EditedJun 9 2018, 1:16 PM

We have upgraded to 2.15 now and using polygerrit ui, and clicking on the "Show comments only" button, it hides the Uploaded patchset <number>.

Tgr added a comment.Jun 9 2018, 6:09 PM

2.15 looks awesome! Now we only need to tag jenkins-bot, BarryTheBrowserTestBot and Cindy-the-browser-test-bot as bots.
(There are some other bot gerrit accounts: Jenkins-mwext-sync, L10n-bot, Libraryupgrader, Maintenance-bot, Pywikipedia Conversion Bot, Release notes rebase bot, Reviewer-bot. I don't think any of those create comments though.)

Tgr awarded a token.Jun 9 2018, 6:10 PM
Tgr added a comment.Jun 9 2018, 6:14 PM

gerrit-review.googlesource.com has a "Comment threads" mode, which adds another level of awesome. Is that a version difference? They are also on 2.15.2.

Paladox added a comment.EditedJun 9 2018, 6:31 PM

@Tgr upstream use the master branch, it has alot of new features to polygerrit. this will be in 2.16 / 3.0.

Upstream may be planning on branching a new release in the next couple of weeks (not sure). Upstream are working towards removing gwtui now so the new release will be the last to support gwtui.

Upstream show 2.15.2 due to the way the version system works. but basically they are running 2.16 / 3.0.

You can test this at https://gerrit-new.wmflabs.org/r/

hashar added a subscriber: hashar.

Gave it a try on https://gerrit.wikimedia.org/r/#/c/test/gerrit-ping/+/226272/ By using:

gerrit review --tag CI --message 'Testing addition of a CI tag' 226272,3

So I guess in the Zuul configuration it is all about passing tag: autogenerated:ci in the various gerrit reports:

 pipelines:
   - name: test
      success-message:  'Main test build succeeded.'
      success:
        gerrit:
          verified: 2
+         tag: autogenerated:ci

The autogenerated prefix should cause the comments to automatically be hidden in the new UI (polygerrit).

Tgr added a comment.EditedFeb 22 2020, 6:40 AM

Doesn't seem to work for the linked patch. OTOH per T217008#5908642 a plain tag: autogenerate works and is already being used for SonarQube (and is super nice) - would be great to have that for the rest of the bots.

The example I gave with --tag CI was wrong. But if I prefix it with autogenerated:: --tag=autogenerated:ci the message can then be filtered out.

Aklapper added a subscriber: Aklapper.

@hashar: A good first task is a self-contained, non-controversial task with a clear approach and links to documentation and the codebase (see the project description). Given the current task description I'm removing the good first task tag.

Change 610361 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] zuul: let Gerrit filterout CI reviews

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

I dropped this task from our onboarding milestone cause it turns out to require knowledge about a lot of areas so that is not very suitable :] I think the above patch might work, at least the generated gerrit review commands sound legit.

hashar claimed this task.Jul 8 2020, 7:58 PM

Mentioned in SAL (#wikimedia-releng) [2020-07-09T19:15:49Z] <hashar> Reloading Zuul to have messages to gerrit tagged with 'autogenerated:ci' | https://gerrit.wikimedia.org/r/c/integration/config/+/610361 | T48148

Change 610361 merged by jenkins-bot:
[integration/config@master] zuul: let Gerrit filterout CI reviews

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

Change 610907 had a related patch set uploaded (by Hashar; owner: Hashar):
[test/gerrit-ping@master] Exerice CI and check autogenerated:ci comments

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

Change 610907 abandoned by Hashar:
[test/gerrit-ping@master] Exerice CI and check autogenerated:ci comments

Reason:
Seems like that is working as intended ;]

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

hashar added a comment.Jul 9 2020, 7:27 PM

And 7 years later that is now possible. I have send a dummy change for demo purposes https://gerrit.wikimedia.org/r/c/test/gerrit-ping/+/610907 and I made a dummy comment (and CI did as well):

After clicking Only comments:

@Tgr does that look any correct?

dancy added a subscriber: dancy.Jul 9 2020, 7:34 PM

Very nice!

Tgr added a comment.Jul 9 2020, 7:34 PM

Awesome, thank you for fixing that!

hashar added a comment.EditedJul 9 2020, 7:37 PM

So that is essentially done for Zuul / jenkins-bot. We could use the same system for the other various bots mentioned at T48148#4269388 :)

Thank you @Tgr for spotting the issue in upstream Gerrit and @Paladox for the guidance. It is rolled for Zuul / jenkins-bot which is arguably the largest spammer. SonarQubeBot already uses it as far as I understand.

Ammarpad added a subscriber: Ammarpad.EditedJul 10 2020, 10:52 AM

I am not sure, but me I am still seeing Jenkins-bot comments even after toggling 'Only comments' (r604184)

@Ammarpad unfortunately it won't work retroactively, as Antoine noted in the email to wikitech-l

Do note that old CI messages written for the last 9 years or so are not tagged and can not be filtered out.

@Ammarpad indeed those recent messages from jenkins-bot on patchset 10 are not properly tagged for some reason. They can be inspected with:

cd mediawiki/extensions/UniversalLanguageSelector
git fetch origin refs/changes/84/604184/meta
git log FETCH_HEAD

One of those comment looks like:

commit fc513a695df74a865726a449953bf7c6713fcfe2
Author: Gerrit User 75 <75@e9e9afe9-4712-486d-8885-f54b72dd1951>
Date:   Mon Jul 6 23:21:40 2020 +0000

    Update patch set 10
    
    Patch Set 10:
    
    PHP test coverage increased (or stayed the same) :-)
    
    - mwext-phpunit-coverage-patch-docker https://integration.wikimedia.org/ci/job/mwext-phpunit-coverage-patch-docker/29582/console : SUCCESS in 2m 05s
    
    Patch-set: 10
    Reviewer: Gerrit User 75 <75@e9e9afe9-4712-486d-8885-f54b72dd1951>

It should have a Tag: autogenerated:ci :-\

Ammarpad added a comment.EditedJul 10 2020, 11:08 AM

Thank you all. On newer patches I am not seeing the issue, so it's OK

Oh no sorry I got confused because the comment are made on Patchset 10 and I thought it was July 10th.

The reason for those commits to not be tagged is that they have been before has been deployed ( https://gerrit.wikimedia.org/r/610361 ). So essentially any comments made before that are not tagged and thus can not be filtered out.

I do not think we have a way to easily tag the old messages without potentially breaking the git repository history :-\

It looks like the new tagged bot comments no longer have an option to reply to them (example comments from this change – left comment is untagged, right comment is tagged):

Is it possible to restore the “Reply” button? I find it useful sometimes, to comment on the individual FAILURE lines (example comment).

Tgr added a comment.Jul 17 2020, 9:48 PM

On Gerrit master CI checks are rendered differently, so you can't quote them anyway. Not sure if that's another Gerrit upgrade away, or just a matter of configuration. See the Checks tab here. (That commit itself is somewhat relevant to this task BTW, although at the time of writing it has not been merged yet.)

In T48148#6316142, @Tgr wrote:

On Gerrit master CI checks are rendered differently, so you can't quote them anyway.

And while it looks pretty nice, at least the integration on gerrit-review is not especially robust. I've had many changes where one received overall +1 votes although some checks failed.

Not sure if that's another Gerrit upgrade away, or just a matter of configuration.

We'd need to install the checks plugin for that tab. So it's more than a config change. But note that our Zuul does not speak the required protocol. So the plugin alone would not do. We'd need to build a bridge between our old Zuul and the plugin (or upgrade Zuul, because IIRC latest Zuul learnt to speak that API).

Regardless of the above, upstream gerrit recently announced that they'll stop working on the checks plugin and will redo the CI integration part. So it's not clear if the checks plugin is the future or even has a future.

Putting those things together makes me think that what we have is probably a good and stable choice for now.

Note the Gerrit checks plugin is being abandoned. They are rethinking it entirely, from https://groups.google.com/g/repo-discuss/c/QYZFCvh_x98/m/z2uyvVrLAAAJ Gerrit CI experience reboot:

Dear Gerrit users, admins and developers,

We have been working on a new strategy for surfacing CI results in Gerrit. This means that we are stopping work on the checks plugin. The new strategy is documented in “Gerrit CI Experience Reboot (public)”. I hope you like it.

We are currently working on gathering detailed requirements from our internal partners. If you are interested in helping us gather requirements, please send me or bro...@google.com an email, so we can set up a channel to collect your input.

It looks like the new tagged bot comments no longer have an option to reply to them (example comments from this change – left comment is untagged, right comment is tagged):

Is it possible to restore the “Reply” button? I find it useful sometimes, to comment on the individual FAILURE lines (example comment).

Sounds it might be a valid enhancement request, even though I am not sure how often people would want to reply to a robot comment, but surely that might happen. I would guess there is some tweak to be made in the Gerrit javascript somewhere. I haven't looked at Gerrit code though.

Should be hold this task on it or do we mark it resolved and file another one?

hashar added a comment.Sep 2 2020, 4:54 PM

It looks like the new tagged bot comments no longer have an option to reply to them (example comments from this change – left comment is untagged, right comment is tagged):

Is it possible to restore the “Reply” button? I find it useful sometimes, to comment on the individual FAILURE lines (example comment).

Seems tha is done on purpose, the messages whose tag starts with autogenerated are considered automated ones and that gets the reply button hidden:

polygerrit-ui/app/elements/change/gr-message/gr-message.js
  _computeShowReplyButton(message, loggedIn) {
    return message && !!message.message && loggedIn &&
        !this._computeIsAutomated(message);
  }

...

  _computeIsAutomated(message) {
    return !!(message.reviewer ||
        this._computeIsReviewerUpdate(message) ||
        (message.tag && message.tag.startsWith('autogenerated')));
  }

I don't think we should bother much, that seems to be a very narrow corner case

hashar closed this task as Resolved.Sep 6 2020, 9:44 PM

Forgot to mark this one resolved. That has been done by upgrading to Gerrit 3.2 and having bots to tag their comments with a tag starting with autogenerated.