Page MenuHomePhabricator

Enable and document "WIP" workflow status in Gerrit
Closed, DeclinedPublic

Description

Currently there is no standard, easily parsable-by-machine way to mark a Gerrit change as work in progress (WIP). This means that either dashboards and queries need to be filtered by a human brain, or each developer has to amend their search strings by excluding "DO NOT MERGE", "WIP", -1 votes by the changeset owner, etc. and hope that they don't miss false negatives.

Add a new label "WIP", inspired by OpenStack's "Workflow" label. Its "neutral" value is 0 ("Ready for reviews"). If it is "voted" to -1 ("Work in progress"), the change cannot be submitted. This vote will stick with new uploads until it is changed back to 0.

For searches, this will allow Gerrit users to restrict search results by adding "label:WIP+0" to their filters.

Mailing list (wikitech-l) discussion summary from @greg: https://lists.wikimedia.org/pipermail/wikitech-l/2016-May/085611.html
How to do it from Tim L (from Sept 2015) https://lists.wikimedia.org/pipermail/wikitech-l/2015-September/083172.html :

Untested, the change would be something like:

diff --git a/project.config b/project.config
index 151eebd..93291e1 100644
--- a/project.config
+++ b/project.config
@@ -12,6 +12,7 @@
        owner = group ldap/ops
        label-Code-Review = -2..+2 group Project Owners
        label-Code-Review = -1..+1 group Registered Users
+       label-WIP = -1..+0 group Registered Users
        create = group Project Owners
        editTopicName = group Registered Users
        viewDrafts = group JenkinsBot
@@ -78,6 +79,11 @@
        value = +2 Looks good to me, approved
        copyAllScoresOnTrivialRebase = true
        copyAllScoresIfNoCodeChange = true
+[label "WIP"]
+       function = AnyWithBlock
+       value = -1 Work in progress
+       value =  0 Ready for reviews
+       copyMinScore = true
 [access "refs/meta/dashboards/*"]
        create = group Project Owners
        create = group platform-engineering

Related: T52842

Event Timeline

Thanks to T68283 korma.wmflabs.org currently parses "WIP" in the commit message and excludes such changesets e.g. from http://korma.wmflabs.org/browser/gerrit_review_queue.html . If we set up an explicit "Label", we need to check whether korma also needs some changes (separate task; I have not checked).

Why? Currently searching with -message:WIP to exclude them works.

Downside of the label is that if somebody else than the patch owner sets it the owner can not unset it.

Downside of the label is that if somebody else than the patch owner sets it the owner can not unset it.

That is an unfortunate downside. Can this be relaxed?

Why? Currently searching with -message:WIP to exclude them works.

The problem is some people used POC (proof of concept), DONOTMERGE and DONOTSUBMIT. We need something that is more obvious that can be set by any reviewer via the UI. Right now this makes it harder to find open patchsets that need a reviewer :-/

Is there even a UI in gerrit to remove or change someone else their vote? Currently the only way I know of is to remove the reviewer entirely from the patchset which removes their current votes including a CR-2. The right to do that can technically be given to everyone.

Anyway, no it doesn't make it harder. What we currently have allows you to filter and if you find one where a different wording was used, replace or add WIP (as you would need to with the label if it was only mentioned in the commit message).

Why not standardize it though? That's basically the request here.

And, the various free text options are not easily discoverable by a new user, whereas a button/label is (is my working assumption/theory).

I won't die for this :) (especially when differential has this built in).

Why not standardize it though? That's basically the request here.

And, the various free text options are not easily discoverable by a new user, whereas a button/label is (is my working assumption/theory).

I won't die for this :) (especially when differential has this built in).

I meant to mention @Jdlrobson, not remove him, with this comment :) (was replying from my phone)

The potentially upcoming gerrit.w.o update will add a tags feature, which might fulfill all requirements in T135245#2293843.

If we add a Gerrit label Workflow, we could configure Zuul/CI to prevent attempting to merge them on CR+2 (Gerrit would reject it anyway) That is T48860

This proposal is selected for the developer wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) summarizing the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

Upstream have started implementing hip status in gerrit now, no more proposal but actually implementing it :)

There is a proposal at https://gerrit-review.googlesource.com/#/c/97245/ by David Ostrovsky. And he proposes patches to drop the drafts entirely.

Ref:
[OpenStack-Infra] Gerrit: Proposals for private changes and wip workflows
http://lists.openstack.org/pipermail/openstack-infra/2017-February/005193.html

There is a proposal at https://gerrit-review.googlesource.com/#/c/97245/ by David Ostrovsky. And he proposes patches to drop the drafts entirely.

Ref:
[OpenStack-Infra] Gerrit: Proposals for private changes and wip workflows
http://lists.openstack.org/pipermail/openstack-infra/2017-February/005193.html

Yep they actually have patches that are doing this. But it is being replaced with private edits which they are also implementing.

Proposing to decline this task as upstream support wip now which can be tested at http://gerrit-new.wmflabs.org/ will be in gerrit 2.15 which is a while away from being released.

demon added a subscriber: demon.

Declining because upstream is already working on this.