Page MenuHomePhabricator

Jenkins comments no longer formatted on Gerrit 3
Open, MediumPublic

Description

Example from https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/608223/:

  • There is no longer tabbed alignment.
  • There is no longer colors red/green for SUCCESS/FAILURE.
  • There seems to be an enforced and really short line-length breaker.

All in all, this has imho made the Jenkins bot comment a bit chaotic and hard to navigate

Event Timeline

Krinkle created this task.Jun 28 2020, 6:08 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 28 2020, 6:08 PM
Krinkle renamed this task from Jenkins coments no longer formatted on Gerrit 3 to Jenkins comments no longer formatted on Gerrit 3.Jun 28 2020, 6:08 PM
Krinkle added a parent task: T254158: Gerrit 3.2 upgrade.
DannyS712 updated the task description. (Show Details)Jun 28 2020, 10:35 PM
QChris added a subscriber: QChris.Jun 29 2020, 6:13 AM

I had a tried a few things towards fixing that, but could not find a simple working solution.

First up: We can no longer match against HTML, but now need to match against the plain text in PolyGerrit. (See for example https://gerrit-review.googlesource.com/c/gerrit/+/139910/7/Documentation/config-gerrit.txt )

What seems straight forward comes with further complications, as I can match parts outside of an URL, but not URLs themselves or parts thereof. For example even trying to only replace console (nothing else) by foo in

- operations-puppet-tests-buster-docker https://integration.wikimedia.org/ci/job/operations-puppet-tests-buster-docker/4603/console : SUCCESS in 8s

by using

[commentlink "demo"]
    match = console
    html = foo

fails for me.

I was surprised that no one of us noticed this on gerrit-test :-D

Maybe this is too different, but is it possible to move the Jenkins output to a separate "checks" tab like upstream does?

QChris added a comment.EditedJun 29 2020, 7:14 AM

I dug a bit futher, and I think it's currently genuinely not possible to match across URLs in commentlinks in current Gerrit. Here are the relevant files:

The element for linked text is in gr-linked-text_html.js.

This element defines an output <span>, which is updated in chunks (this is the crucial part!) through the _handleParseResult method in gr-linked-text.js.

gr-linked-text.js delegates all the chunk-splitting and commentlink application to link-text-parser.js. That file does applies commentlinks (and does some url rewriting) on the chunks that it gets from ba-linkify.js.

So the raw text gets fed through to ba-linkify.js, which splits into text chunks and url chunks. link-text-parser.js applies commentlinks to each of the chunks individually. But what we'd need in our use-case is to match across (text-chunk, url-chunk, text-chunk). This is not possible in the current Gerrit commentlink architecture :-(

Maybe this is too different, but is it possible to move the Jenkins output to a separate "checks" tab like upstream does?

I think that's definitely the target for the long run.
But it only helps us with future CI results, not with all CI results that we already have in the comments :-/

I dug a bit futher, and I think it's currently genuinely not possible to match across URLs in commentlinks in current Gerrit.

I filed bug 13047 upstream, but I doubt they'll address it, as it would come with some major refactorings.

But it only helps us with future CI results, not with all CI results that we already have in the comments :-/

@Krinkle just told me in IRC that we keep older links only working for 7 days. That makes my point moot.
Ignore me. I'm stupid.

Change 608296 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/puppet@production] zuul: stop prefixing report with the job name

https://gerrit.wikimedia.org/r/c/operations/puppet/ /608296

hashar added a subscriber: hashar.Jun 29 2020, 11:52 AM

@QChris nice detailed report. So the issue is that it is now done in chunks when previously comment links were applied on the whole text or on a line by line basis, isn't it?

The links found by ba-linkify are indeed not processed at all. There is a shortcircuit to early return, and a hack to still process links in case is next to the protocols

if (href) {  // ba-linkify found a link
  const result = URL_PROTOCOL_PATTERN.exec(href);
  if (result) {
    const prefixText = result[1];
    if (prefixText.length > 0) {
      // Fix for simple cases from
      // https://bugs.chromium.org/p/gerrit/issues/detail?id=11697
      // When leading whitespace is missed before link,
      // linkify add this text before link as a schema name to href.
      // We suppose, that prefixText just a single word
      // before link and add this word as is, without processing
      // any patterns in it.
      this.parseLinks(prefixText, []);
      text = text.substring(prefixText.length);
      href = href.substring(prefixText.length);
    } 
    this.addText(text, href);
    return;
  } 
}

Given "job-namehttps://www.example.org/job-name", prefixText would contain "job-name" and would have links processed. But that does not really prove to be any helpful in our case.

@Paladox found out a small Gerrit plugin that addresses our use case: https://github.com/dburm/pg-test-result-plugin :]

I hacked a bit on the above pg-test-result-plugin and got something like

working. Judging by feedback from Reedy, paladox and Majavah in IRC it's worth having that. So I'll work towards getting it onto our gerrit.

Change 609519 had a related patch set uploaded (by QChris; owner: Christian Aistleitner):
[operations/puppet@production] gerrit: Add table for Zuul CI results underneath the commit message

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

Change 609520 had a related patch set uploaded (by QChris; owner: Christian Aistleitner):
[operations/puppet@production] gerrit: Add SonarQube results to CI table

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

QChris added a comment.Jul 4 2020, 2:22 PM

I've put this change live on gerrit-test.wikimedia.org to allow reviewing:

  • change 579818 is a commit with only a few Zuul results (only successes)
  • change 602422 is a commit with lots of Zuul and SonarQube results (both failures and successes)

Change 609742 had a related patch set uploaded (by QChris; owner: Christian Aistleitner):
[operations/puppet@production] gerrit: Drop bot name, check date, and check duration from CI table

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

Change 609743 had a related patch set uploaded (by QChris; owner: Christian Aistleitner):
[operations/puppet@production] gerrit: For Zuul, bring only 'Main test build' jobs to CI table

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

Change 609744 had a related patch set uploaded (by QChris; owner: Christian Aistleitner):
[operations/puppet@production] gerrit: For SonarQube, bring only overall result to CI table as 'Sonar Cloud'

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

Change 609519 merged by Dzahn:
[operations/puppet@production] gerrit: Add table for Zuul CI results underneath the commit message

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

Change 609520 merged by Dzahn:
[operations/puppet@production] gerrit: Add SonarQube results to CI table

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

Change 609742 merged by Dzahn:
[operations/puppet@production] gerrit: Drop bot name, check date, and check duration from CI table

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

Change 609743 merged by Dzahn:
[operations/puppet@production] gerrit: For Zuul, bring only 'Main test build' jobs to CI table

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

Change 609744 merged by Dzahn:
[operations/puppet@production] gerrit: For SonarQube, bring only overall result to CI table as 'Sonar Cloud'

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

QChris closed this task as Resolved.Tue, Jul 7, 6:49 AM
QChris claimed this task.

After some hacking on the plugin mentioned by hashar above, we got it to work for us and we now have a dedicated CI table (much in the spirit of OpenStack's HideCI).

It's not perfect, and people will come around to add/remove things there. But it's better than what we had on Gerrit 2.15.

This is absolutely fantastic and so much better than what we had previously \o/

Because it shows up by default, I prefer it to the separate checks pane I had mentioned earlier.

hashar reopened this task as Open.Fri, Jul 10, 9:09 AM

I definitely love the addition of the table of results under the commit message. We still have to remove the insertion of the job name in front of each messages which was the original intent of this task hence reopening.

I made a lengthy comment on the Zuul config change required for that at https://gerrit.wikimedia.org/r/c/operations/puppet/+/608296 , it seems that the Javascript used to generate the table does not rely on the job name being prefixed anymore \o/

hashar triaged this task as Medium priority.Mon, Jul 27, 1:30 PM
Krinkle removed a subscriber: Krinkle.Mon, Jul 27, 5:55 PM