New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 636454 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
User never visited
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make all code-review labels sticky for ANGLE

Project Member Reported by jmad...@chromium.org, Aug 10 2016

Issue description

Currently ANGLE's gerrit preserves code-review -2 and code-review +2, but ideally we could preserve all code-review labels given by valid reviewers.

vadimsh can you help with this? You helped us in a prior request.
 
Cc: dsansome@chromium.org vadimsh@chromium.org
Components: -Infra>Git Infra>Git>Admin
Owner: ----
Putting it in general git admin triage queue and CCing this week's git admin.

Also, I don't think it is possible to always copy all scores.

For the reference, here's current config (https://chromium.googlesource.com/angle/+/refs/meta/config/project.config):

[label "Code-Review"]
	copyMinScore = true
	copyMaxScore = true
	copyAllScoresOnTrivialRebase = true
	copyAllScoresIfNoCodeChange = true

And here's possible set of knobs there (https://gerrit-review.googlesource.com/Documentation/config-labels.html):

label.Label-Name.copyMinScore
label.Label-Name.copyMaxScore
label.Label-Name.copyAllScoresOnMergeCommitFirstParentUpdate
label.Label-Name.copyAllScoresOnTrivialRebase
label.Label-Name.copyAllScoresIfNoCodeChange
label.Label-Name.copyAllScoresIfNoChange

There's no option to "copyAllScoresAlways".
Any at Google who maintains Gerrit? I'm wary of leaving this bug without an owner, I have a feeling it'll sit in vacant space until the end of time.
Infra>Git>Admin is a bug queue that is actively triaged (we have a weekly bug triage rotations and stuff). I have faith in the system :)

Comment 4 by dsansome@google.com, Aug 11 2016

Hmm well *I* have no idea.  If this isn't supported at the moment it sounds like it should be a feature request in https://bugs.chromium.org/p/gerrit/issues/list?
This is indeed possible. An example configuration:
[label "Code-Review"]
        abbreviation = R
        value = -1 Don't submit as-is
        value =  0 No score
        value = +1 Looks good to me
        defaultValue = 0
        function = MaxWithBlock
        copyMinScore = true
        copyMaxScore = true
        copyAllScoresOnTrivialRebase = true
        copyAllScoresIfNoCodeChange = true

Barring the min/max labels, this should let LGTM be “sticky”.
This would mean there is no Code-Review +2 though, I think. We would like to keep +1 and +2, but make the +1s sticky.
Components: -Infra>Codereview>Gerrit
You don’t have to change the range of review values. It will still remain sticky.

Keep in mind there is a bug with sticky LGTMs that is being fixed: https://bugs.chromium.org/p/gerrit/issues/detail?id=4373
Oh, cool. How can we get the config then that you listed above? Then after the gerrit bug is fixed we'll be good to go?
Hm... It looks like the configuration is already set up that way (from https://chromium.googlesource.com/angle/+/refs/meta/config/project.config):

[label "Code-Review"]
	function = MaxWithBlock
	abbreviation = R
	copyMinScore = true
	copyMaxScore = true
	copyAllScoresOnTrivialRebase = true
	copyAllScoresIfNoCodeChange = true
	value = -2 Do not submit
	value = -1 I would prefer that you didn't submit this
	value =  0 No score
	value = +1 Looks good to me, but someone else must approve
	value = +2 Looks good to me, approved
	defaultValue = 0

Is this still an issue for folks, or did the bug mentioned in #8 actually fix this?
Second ping :). If there's no response for another day I'll close.
Did the Gerrit change land? I'd like to test it, but unsure if it's on the deployed Gerrit site for ANGLE.
Ah... I saw that it was committed. I'll check to make sure that it's deployed
Yes, this is deployed for angle: https://chromium.googlesource.com/angle/angle

Comment 15 by smut@chromium.org, Aug 22 2016

Cc: -jmad...@chromium.org
Labels: Needs-Feedback
Owner: jmad...@chromium.org
Status: Assigned (was: Untriaged)
Can angle confirm whether their code-review labels are sticky now or not?
Cc: jmad...@chromium.org
Owner: smut@chromium.org
I can confirm this does not work as intended currently, Code-Review +1 labels are dropped. Smut, can you close this as WontFix if there is no possible resolution?

See: https://chromium-review.googlesource.com/#/c/374338/

Comment 17 by s...@google.com, Aug 24 2016

Labels: -Needs-Feedback
Status: ExternalDependency (was: Assigned)
I posted on https://bugs.chromium.org/p/gerrit/issues/detail?id=4373 that their fix doesn't seem to have worked.

Comment 18 by smut@chromium.org, Aug 26 2016

Status: Fixed (was: ExternalDependency)
They tell me it's working as intended not to copy Code-Review+1. They only copy +2 because +1 means someone else must approve anyways. +1 only sticks across rebases and commit message changes.
Thanks. If I wanted to file a feature request for our use case, do you know which issue tracker or labels to use?

Sign in to add a comment