Page MenuHomePhabricator

make gerritbot remove the "patch-for-review" tag once a patch is merged (or abandoned)
Open, LowPublic

Tokens
"Like" token, awarded by Pppery."Like" token, awarded by Liuxinyu970226."Like" token, awarded by Zoranzoki21."Like" token, awarded by Dzahn."Like" token, awarded by MarcoAurelio."Love" token, awarded by TerraCodes."Like" token, awarded by Dalba.
Assigned To
None
Authored By
mmodell, Apr 7 2015

Description

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@MZMcBride: There is still desire to transition to differential but it seems like there is a lot of FUD as well. Personally I've just been assigned with higher priority things to work on so I haven't had much time to think about it.

Yeah. I'm not exactly opposed to improving gerritbot (Gerrit is still in production and it manages the repos of record, after all), I'm just wary of investing more time and energy into Gerrit<-->Phabricator Maniphest integration given that I'm not sure Gerrit will survive the year. Plus I imagine Differential<-->Maniphest integration is easier and better long-term.

I'll leave a note on T95307: Create a phabricator api method to remove 'patch-for-review' tag from a task now... this task definitely seems like it could be tagged with the good first bug project at least once T95307 is resolved.

Tgr added a subscriber: Tgr.Jun 18 2015, 5:07 PM

this task definitely seems like it could be tagged with the good first bug project at least once T95307 is resolved.

It's easy to remove the tag when a linked patch gets merged or abandoned, but how would you tell whether all of them have been? Doesn't seem straightforward to me.

Restricted Application added a subscriber: scfc. · View Herald TranscriptJun 18 2015, 5:07 PM
scfc added a comment.Jun 23 2015, 9:29 PM

I noticed that some users remove the Patch-For-Review tag manually after a task is closed (resolved/declined/declared invalid). To reduce these blips I thought about creating a Herald rule, but personal rule triggers do not allow something like "Remove project".

If global Herald rules allow to remove projects, I'd propose to set up one that, if task status is any of resolved, declined, invalid, removes the project Patch-For-Review.

When a task is closed a task is closed anyway. I don't really understand why people care about that case. Do you search for patch-for-review tasks and don't exclude closed tasks? Why? And even if a task is closed it might still have a non-merged patch attached - a Patch-For-Review.

I do understand why people care about open tasks that have no patches left for review, though.

scfc added a comment.Jun 24 2015, 2:41 PM

I don't use the Patch-For-Review tag in any way so people or bots adding or removing it without doing something else is a (minor) nuisance to me.

We will soon move to differential and away from gerrit. When that happens this becomes a non-issue.

Propose closing this as declined.

I just linked up some revisions to demonstrate how that works. It shows which revisions are still open, and which ones are closed, so you get a much better status indicator than simply "patch for review" - it actually links to each revision in the task header.

Propose closing this as declined.

Wouldn't this task be a task, which is blocked by the gerrit -> differential migration? Declined doesn't really match, because the move to Differential is a solution for this problem, so it can't be declined (I understand declined as the old bugzilla "WONTFIX") :)

Btw.: I don't know any roadmap for the move to differential, so this might be good to fix anyway (depends on how useful it is, how much work it needs and how much work people who are familar with the components can invest :P).

@Florian: The move to differential is high priority for Release-Engineering-Team-TODO (201907), expect to see a roadmap soon ;)

scfc added a comment.Oct 14 2015, 4:09 PM

So how about we wait to see Differential in action, as a functioning solution for this task, and we decline it then? (Or mark as it invalid, as Gerritbot will likely be decommissioned.)

Aklapper lowered the priority of this task from Low to Lowest.Feb 9 2016, 10:01 PM
Dalba awarded a token.Feb 10 2016, 2:13 AM
Dalba added a subscriber: Dalba.
Ricordisamoa added a subscriber: Ricordisamoa.EditedMar 4 2016, 11:08 AM

How about a new text field for tasks? Gerritbot could update it whenever a relevant Gerrit change is uploaded, humans would update it for other code review systems, then Gerritbot would autoremove the Patch-For-Review tag only if all relevant patches are on Gerrit and they have been merged/abandoned.

I don't see suddenly a strong case for adding more UI noise (new text field) to work around a situation that will be gone in a few months, plus active maintenance of our the Gerrit-to-Phab bot implementation code.

will be gone in a few months

Where did I hear it already? :-þ

Which (sub)problem related to this task gets solved by introducing some dedicated text field?

Which (sub)problem related to this task gets solved by introducing some dedicated text field?

Making easy to see at a glance which closed tasks still have associated patches open?

What would that "text field" contain exactly?

What would that "text field" contain exactly?

A list of URIs of relevant patches that address the bug, one per line.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 1 2016, 12:18 AM

What would that "text field" contain exactly?

A list of URIs of relevant patches that address the bug, one per line.

This is supported now that we are importing un-merged patches into diffusion. For example see the details box under the description on T125678
links to unmerged patches. This would work more reliably if gerritbot explicitly added the link but phabricator figures it out from the commit metadata eventually.

Paladox edited projects, added Gerrit; removed GerritBot.Aug 7 2016, 1:09 PM
Paladox added a subscriber: Paladox.

Actually this requires a change in the its-phabricator plugin.

Paladox added a subscriber: demon.Aug 7 2016, 1:12 PM
Aklapper lowered the priority of this task from Normal to Lowest.Aug 7 2016, 7:36 PM
Aklapper removed a subscriber: demon.

@Paladox: Decreasing the priority of this task back to its previous value. As priority reflects reality and does not cause it, feel free to raise priority if you plan to actively work on this task, or if you can provide reasons why this task has suddenly become more urgent.

Hi sorry I raised it. I raised it since one of the tasks I merged had it at normal.

Dzahn added a comment.Aug 9 2016, 3:22 PM

He merged 2 tasks and made sure that the resulting task had the same priority as the original one. No more and no less.

Paladox edited projects, added GerritBot; removed Gerrit.Sep 1 2016, 2:05 PM
Paladox added a comment.EditedFeb 25 2017, 6:28 PM

This requires T159041 first be fixed. As the maniphest.update conduit api dosen'
t support this but maniphest.edit conduit does.

Paladox raised the priority of this task from Lowest to Normal.Feb 26 2017, 10:20 PM

I have this working on https://gerrit-new.wmflabs.org/ except from i am not doing abandoned on there yet. I have only tried merging :)

Dzahn awarded a token.Feb 28 2017, 4:24 PM
Tgr added a comment.Feb 28 2017, 6:16 PM

I don't think this is doable right now. We want to remove the tag when all pending patches are merged, but not one a patch is merged but there still are other unmerged tasks associated with the task. gerrit cannot tell whether that is the case; Differential probably could but it's too unreliable (some repos aren't even mirrored to it).

Dzahn added a comment.Feb 28 2017, 6:35 PM

In that case i would tend to change the proposal to "stop adding patch-for-review tags" since it seems they are not used much in the first place. People who use them please speak up.

Tgr added a comment.Feb 28 2017, 8:37 PM

In that case i would tend to change the proposal to "stop adding patch-for-review tags" since it seems they are not used much in the first place. People who use them please speak up.

It's occasionally mildly useful when looking at lists of potential outreach tasks to see which ones are already taken (in a sense).
I think it would be also useful to improve code review times, if we took that more seriously.

I am not sure if there is an actual problem here TBH. The current setup works well enough. Removing the tag when all patches are merged or abandoned would be a nice extra, if it was easy to do, which it is not. (Even if the gerrit->Phabricator replication was reliable, I don't think there is a way to query Phabricator to see whether the linked patches are abandoned.) I'm not sure anyone had a problem with the non-removal of tags beyond that.

Dzahn added a comment.Feb 28 2017, 8:40 PM
In T95309#3062430, @Tgr wrote:

I am not sure if there is an actual problem here TBH. The current setup works well enough.

Ok, convinced. Fine with me either way.

In T95309#3062430, @Tgr wrote:

In that case i would tend to change the proposal to "stop adding patch-for-review tags" since it seems they are not used much in the first place. People who use them please speak up.

It's occasionally mildly useful when looking at lists of potential outreach tasks to see which ones are already taken (in a sense).
I think it would be also useful to improve code review times, if we took that more seriously.
I am not sure if there is an actual problem here TBH. The current setup works well enough. Removing the tag when all patches are merged or abandoned would be a nice extra, if it was easy to do, which it is not. (Even if the gerrit->Phabricator replication was reliable, I don't think there is a way to query Phabricator to see whether the linked patches are abandoned.) I'm not sure anyone had a problem with the non-removal of tags beyond that.

What we could do is do something in phabricator core to make any comments created through conduit to add maybe a little identifier. And when merges take place it will remove one identifier one by one until all the comments that were created and is merged now. Then when we merge the last patch the bot will know to remove the project tag. Note that we should also add a field so that only if we want this identifier to be added to he comments it creates.

Maybe @mmodell knows how to do that :)

Aklapper lowered the priority of this task from Normal to Low.Mar 22 2017, 5:25 PM
Urbanecm added a subscriber: Urbanecm.

Sorry, I should be looking for tickets before I create new one, not after :).

Urbanecm moved this task from Backlog to Watching on the User-Urbanecm board.

This can be done now as we upgraded to stable-2.14 which includes my change i did to support this.

Though i wonder how easy is it for the bot to detect open changes related to this task @mmodell?

Urbanecm moved this task from Watching to Created on the User-Urbanecm board.Apr 23 2018, 11:26 AM

bump any progress on this api @mmodell ?

gerritBot can now remove tags but if there's more then one patch for that bug it will still remove it once one of the changes is merged.