New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment
link

Issue 891090: Add a CL description flag to disable tricium

Reported by qyears...@chromium.org, Oct 1 Project Member

Issue description

This would be something like NOTRY, NOPRESUBMIT, etc.

Currently Tricium doesn't get commit descriptions since that's not included in the data returned by the /changes REST API query (https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes), so it's possible that implementing this would require making more requests to Gerrit.

This feature would be helpful in preventing noise in the cases where occasional CLs have lots of false positives and we don't want Tricium to automatically run.
 

Comment 1 by parmsing...@gmail.com, Dec 1

I would like to work on this one. I'll get to understand how Tricium fetches data. :)

Comment 2 by qyears...@chromium.org, Dec 3

Cc: juliehockett@google.com parmsing...@gmail.com mar...@chromium.org
Alright!

Some background that might be helpful: When a CL is analyzed by Tricium, the following things happen in this order:

 1. A poll request is triggered by cron (once per minute)
    to get a list of changes. This is how we get initial info
    about the changes, including list of files etc.
    https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/poll.go
    This creates a tricium.AnalyzeRequest proto used as the payload
    for the Analyze request.

 2. Then there is the Analyze request, which puts a track.AnalyzeRequest
    entity in datastore and triggers the launch if everything is OK.
    https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/frontend/rpc_analyze.go

 3. Then the launch request, which decides which analyzers are
    actually going to be run, and adds trigger tasks to task queue.
    https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/launcher/rpc_launch.go
    
Note, code related to making requests Gerrit is in:
https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/gerrit.go

So, it appears that there's actually an option we can add to the "changes" request (https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/gerrit.go?g=0&l=265) called COMMIT_FOOTERS (according to https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#commit-footers).

This means that we may not need to make an extra request; we can just fetch the commit description for every CL while we're polling. Inspection of commit footers could potentially be done in the poll request to decide whether to analyze the commit
or not (i.e. just to add a NOTRICIUM or NOANALYZE option).

In order to use the commit description in other places, it would have to be persisted somehow (e.g. added to track.AnalyzeRequest, but if it's only used in poll.go, then we don't need to keep it.

Comment 3 by parmsing...@gmail.com, Dec 15

Sorry for the late response, I had my final university exams. :)

In file gerrit.go (https://cs.chromium.org/chromium/infra/go/src/infra/tricium/appengine/gerrit/gerrit.go?l=24) the imported library doesn't have the property MessageWithFooter in the RevisionInfo struct.
Docs: https://godoc.org/golang.org/x/build/gerrit#RevisionInfo

I am continuing by just editing the source locally for now. We might need a merged PR on golang's build repo before merging the changes.

I had some questions,
I need to extract the footer or I can just search for NOTRICIUM in the messageWithFooter string?
Commit footers mean the data that is placed after the commit description?
e.g.
Commit title
This change makes some new changes

Footer1: somevalue

Comment 4 by qyears...@chromium.org, Dec 17

Good point that the gerrit library needs to be updated. I think they just completed the parts that they needed but didn't include all fields in the API, so adding more fields when we need them is correct.
 
Personally, for correctness, I think we should extract the footer and skip Tricium if the value for the footer is some "truthy" value, but not skip if just the word NOTRICIUM is present anywhere in the description.

It's correct that the footers in Gerrit are the last paragraph ofd the description where each line is of the form "Key: Value".

Although, for historical reasons we use description "flags" that are in all caps with an equals sign, like NOTRY=yes or TBR=username -- these are different because we used to use Rietveld rather than Gerrit which didn't have the same footer concept.

Hope your exams went well :-)

Comment 5 by parmsing...@gmail.com, Jan 5

Started code-review on the change in the Gerrit library.
Can be followed on https://go-review.googlesource.com/c/build/+/156438

I had to use my other email though. Gerrit was rejecting my parmsingh129 id even though it is registered on go-review. Do you know any fix on this?

remote: ERROR: commit 9b438d4: email address parmsingh129@gmail.com is not registered in your account, and you lack 'forge author' permission.
remote: The following addresses are currently registered:
remote:    parmsingh101@gmail.com

Comment 6 by qyears...@chromium.org, Jan 8

Alright, thanks for making the Go library side change too :-)

My interpretation of that error message would be: On go-review you have an
account with the email "parmsingh101@gmail.com", and in your local git
config you have user.email set as "parmsingh129@gmail.com"; I think the
error is saying that "parmsingh129@gmail.com" is not "registered"... I'm
not entirely sure what Gerrit needs you to do to use different email
addresses; in Gerrit settings I see both "email addresses" and "identities"
which might be relevant.

Anyway in this case depending on what you want, maybe you could also use
git config to set the local user.email to the parmsingh129 account.

Comment 7 by rmis...@google.com, Jan 8

Cc: tandrii@chromium.org
My 2cents: We should be moving away from CL description flags. They were added as a hacky way to get around some CQ checks in Rietveld and now have carried over to Gerrit as footers. They are easy to have typos in and the secret keywords are completely non-obvious.

AFAIK, the CQ keywords (No-Try/No-Tree-Checks etc) are eventually going to be moved away from the description and into a CQ server. (tandrii@ can confirm).

Ideally, tricium should have a checkbox in it's gerrit plugin that says "Run tricium". It should be checked by default and if turned off then tricium should not run.

Comment 8 by tandrii@chromium.org, Jan 8

totally agree with rmistry@, and I think agable@ would concur.

Comment 9 by qyears...@chromium.org, Jan 9

Good points -- I guess even if we do add a CL description flag for now, it's not ideal, partly because it's not very discoverable.

Note, I think a checkbox in the plugin wouldn't help in the case where you don't want Tricium to run even once, upon first upload, whereas a CL description flag could be used for that purpose.

Also note, parmsingh129 has got a CL up for this currently: https://chromium-review.googlesource.com/c/infra/infra/+/1379425

tandrii@, rmistry@, I wonder do you have any thoughts about committing this given that it's already implemented? Also, if we did go ahead with adding this, would it be preferable to only add "No-Tricium" (Gerrit style) or NOTRICIUM (older style)?

Comment 10 by tandrii@chromium.org, Jan 9

I've replied on the CL, but basically I'm recommending against negation in key name, it's confusing, even though familiar for those used to NOTRY/NOPRESUBMIT. Also, no need to support legacy style TAGS, ie just Gerrit footer Tricium: disable|no|none|skip

That said, it's true that it's hard to disable tricium run at the time of upload, but if it is so important that developer will go and find specific gerrit footer value, can't this be done with the help of WIP mode?
  (1) dev uploads in WIP, tricium does nothing, because WIP
  (2) dev unchecks tricium plugin checkbox
  (3) dev changes from WIP to ready for review.

In fact, good Gerrit integration with Tricium plugin in (3) would result in smth like "do you want to run tricium before asking for review?" and then Tricium would publish change instead of a dev.

Comment 11 by rmis...@google.com, Jan 9

I like Andrii's WIP suggestion. The tricium plugin checkbox would then probably be labelled something like 'Run Tricium after publish' (or something else descriptive and better). Could even remove the checkbox after it is out of WIP or rename the label.

The only problem for me in this approach, is that in WIP mode, I will not have a way to force a tricium run if I want to make sure I address all issues before sending it out for review.
But maybe that is ok, after I send it out for review, tricium will just be one of the reviewers whose comments I have to address.


Another approach- I personally think it is OK for tricium to run by default even in WIP mode. Devs will get used to it. I think the benefit of that could out weight the annoyance from noise. Devs who are really bothered by it will learn to use the keyword or uncheck the checkbox immediately or however we decide to disable tricium runs.
But even in this approach I think having a tricium checkbox would help. It gives flexibility to issue owners to turn off tricium at any time for whatever reason.

Comment 12 by parmsing...@gmail.com, Jan 9

Hi everyone, glad to meet more members :)
I agree that CL description flags are quite hard to find.
I like the idea of making a comment with a checkbox (to enable or disable) in WIP or starting to analyze in WIP mode, with an option to disable. Gives a better UI to devs.

About the CL I made, if the changes would be removed anyway in near future, I have no problem with it being abandoned. I'll try implementing the checkbox functionality instead. Seems fun to work on. :p

I wish we could keep both as I liked that I can disable Tricium by just typing footer rule in the text editor without having to open or click on anything.
The problem I see with keeping both is that, it would be very hard to keep them in sync.

So, I am also inclining towards having a checkbox based functionality.

Comment 13 by tandrii@chromium.org, Jan 9

I've thought a bit more about this. Until Gerrit supports custom key-value metadata alongside CL, we'd need to use CL description, whether we like or not, if not for Tricium, than at least for CQ. Thus, I think you should still support disabling tricium based on special key: value in description footers.

Comment 14 by rmis...@google.com, Jan 9

> Until Gerrit supports custom key-value metadata alongside CL
Do you know if there is a bug open for this? This would be generally useful for lots of things.

Comment 15 by bugdroid1@chromium.org, Jan 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/210ee1b8086034dcb4df7d93fd8eaabca108a001

commit 210ee1b8086034dcb4df7d93fd8eaabca108a001
Author: Parminder Singh <parmsingh129@gmail.com>
Date: Fri Jan 11 19:12:24 2019

[tricium] disable tricium on CLs with "Tricium: no" in description

The Tricium flag marks skipping of the changes.
A poll step filters out changes with the flag set to disable|no|none|skip|false value.

Bug: 891090
Change-Id: Id0d34baa38354a3a8af4d8a35f1c8955861c1a49
Reviewed-on: https://chromium-review.googlesource.com/c/1379425
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#19936}
[modify] https://crrev.com/210ee1b8086034dcb4df7d93fd8eaabca108a001/go/src/infra/tricium/appengine/gerrit/poll.go
[modify] https://crrev.com/210ee1b8086034dcb4df7d93fd8eaabca108a001/go/src/infra/tricium/appengine/gerrit/poll_test.go

Sign in to add a comment