On 1/10/19 12:17 AM, Ghanshyam Mann wrote:
---- On Thu, 03 Jan 2019 22:51:55 +0900 Sean McGinnis <sean.mcginnis@gmx.com> wrote ----
On Fri, Dec 28, 2018 at 11:04:41AM +0530, Surya Singh wrote:
Dear All,
There are many occasion when we want to priorities some of the patches whether it is related to unblock the gates or blocking the non freeze patches during RC.
So adding the Review-Priority will allow more precise dashboard. As Designate and Cinder projects already experiencing this[1][2] and after discussion with Jeremy brought this to ML to interact with these team before landing [3], as there is possibility that reapply the priority vote following any substantive updates to change could make it more cumbersome than it is worth.
With Cinder this is fairly new, but I think it is working well so far. The oddity we've run into, that I think you're referring to here, is how those votes carry forward with updates.
I set up Cinder with -1, +1, and +2 as possible priority votes. It appears when
This idea looks great and helpful especially for blockers and cycle priority patches to get regular review bandwidth from Core or Active members of that project.
IMO only +ve votes are more appropriate for this label. -1 is little confusing for many reasons like what is the difference between Review-Priority -1 and Code-Review -2 ? Review-Priority -1 means, it is less priority than 0/not labelled (explicitly setting any patch very less priority).
After seeing Cinder dashboard, I got to know that -1 is used to block the changes due to procedural or technical reason. But that can be done by -2 on Code-Review label. Keeping Review-Priority label only for priority set makes it more clear which is nothing but allowing only +ve votes for this label. Personally, I prefer only a single vote set which can be +1 to convey that these are the set of changes priority for review but having multiple +ve vote set as per project need/interest is all fine.
I don't know if this was the reasoning behind Cinder's system, but I know some people object to procedural -2 because it's a big hammer to essentially say "not right now". It overloads the meaning of the vote in a potentially confusing way that requires explanation every time it's used. At least I hope procedural -2's always include a comment. Whether adding a whole new vote type is a meaningful improvement is another question, but if we're adding the type anyway for prioritization it might make sense to use it to replace procedural -2. Especially if we could make it so any core can change it (apparently not right now), whereas -2 requires the original core to come back and remove it.