489: [ ] DON'T SHIP IT button
- Confirmed
- Review Board
| bela.******@gmai***** (Google Code) (Is this you? Claim this profile.) | |
| 963, 3219 |
I'm using ReviewBoard at VMware. Today I saw a review where someone else had x'd [x] Ship it! -- but I saw a serious flaw that made me want to actively retract the other person's approval. I want a [ ] DON'T SHIP IT! button. Something that I can use to mark an already approved review as really requiring further fixing -- or at least discussion. >Bela<
-
- Type-Defect + Type-Enhancement
Maybe "Sink-it" should be used? That way, we can report that a review was done and the quality isn't good enough to be shipped. It seems that Review Board assumes that all code presented for review is shippable. Without a "SINK IT" report, it's not possible to restrain a release. As I see it, this should probably be an urgent enhancement.
What we've been doing at VMware is to just write in the review that the change isn't good enough and either shouldn't go in or needs to be rewritten. Ship It is a boolean. If checked, it means the code's ready. If not checked, the code isn't ready to go in. I'm still not convinced that we need a tri-state, but I'll think about it some more.
Bugzilla has a multi-state value for "flags" - the value can be unset, ?, + or -. Bugzilla uses ? to indicate request you a grant, + means the flag was granted, and - means it was denied. So, review? requests a review grant, review+ means review granted (implying the code passed review), and review- means review denied (implying the code did not pass review). Being able to specifically say - review denied or "SINK IT" speaks far more clearly to review requestors than simply not checking the "Ship It" checkbox. I think it should become either a radio or a drop-down selection that defaults to "Select One" or "Not Fully Reviewed."
I think it would be enough to undo the "ship it" action.
Re Comment 5: It wouldn't here. It's helpful to know when something specifically needs work, that a review was done but the code did not pass muster, helping others to see that further review is not warranted until the "SINK IT" is cleared, or at least be sure to read the "SINK IT" first, then determine if the remarks are valid and proceed accordingly. I do agree, however, that revoking a SHIP IT or SINK IT should be possible in either case, but that privilege should be limited so that say the reviewer and a limited set of individuals can override a ship/sink it decision. Kevin
Re Comment 6: I failed to mention, when a replacement attachment is published, I feel that the configuration should support clearing any/all SHIP/SINK IT flags, but that's probably a different RFE.
I do want this sort of thing in for 1.0. There's become enough demand for it. How we do it is somewhat tricky due to the database schema, though, so we'll need to figure things out first.
-
+ Milestone-Release1.0
Part of the problem here is that "ship-it" is a flag on a particular comment, and is not tied to a particular user. So there's no concept of "retracting a ship-it", since the same user can post multiple ship-its (from multiple comments), and there's no way to indicate that a new comment is a retraction of a ship-it from an old comment. What should probably be done is to break off the ship-it flag from comments, and make it a separate column of "user approvals" (or disapprovals, as the case may be), where a user can set an approval, retract it later, or even set a disapproval to counteract some other user's approval.
-
+ Confirmed
As much as I want this ASAP, we need to push out a 1.0 release and this is going to require some additional work and discussion. It's going to be a top priority for 1.5. I'm trying to tie it into some policy stuff I'm toying around with, where companies can sort of customize what they want a bit.
-
- Priority-Medium - Milestone-Release1.0 + Priority-High + Milestone-Release1.5 + Component-Reviews
(Late to the party..) Would it make sense to have a ReviewFlag concept (similar to CommentFlag from django.contrib.comments), where a user could flag another reviewers "SHIP IT" as "NOT YET"/"DISAGREE"/etc. And this can be tied together with reviewboard.reviews.models.Comment to provide a rationale on the action taken?
I'm happy to see the comment blocking issue 963 being merged here. IMO, an ideal solution lets the reviewer block at the diff/comment level, and his/her overall "ship-it/sink-it" flag can't go to "ship-it" if they have unsatisfied "sink- it" blocks. The overall "ship-it" status for the review (across all reviewers) should be dependent on some rule, such as: "75% of participating reviewers designate "ship-it" and NO unresolved "sink-it" blocks".
-
- Milestone-Release1.5 + Milestone-Release1.1
A lot of discussion and design needs to take place regarding this and the proposals for comment types (which likely will not be happening as part of the core Review Board product, but that's another discussion). I don't feel comfortable making this part of the 1.1 plans. Moving back to 1.5.
-
- Milestone-Release1.1 + Milestone-Release1.5
I don't like the sink it idea. If the code has not passed review then the reviewer should never select Ship It. And if the code is just too horrible then it should be marked so in the comments and never shipped. To my a review request that has not been selected as Ship It is the same as being selected as Sink It. It makes no sense to add this feature. My 2 cents.
Comment #17 misses my point. Perhaps in Peter's project it is normal for a change to be strictly reviewed by a single reviewer. In my project, a review is broadcast to one or more groups as well as a few focal reviewer individuals. A single change is likely to get reviewed by several people -- whose opinions may differ. Of course if reviewer A thinks the code is not worthy, he should not checkmark "Ship it". My request is for when reviewer A said it was good, then reviewer B came along and found problems. I want B to be able to negate A's "ship it" in a manner which is visible to review- board itself -- not only to a person who is reading every word of every comment. It my environment it is also quite normal for people to post reviews that read like "I've reviewed your use of the XYZ API, which seems good. However since I don't know anything about the ABC API which you are also using, you need to get this reviewed by someone from that team." Such a review is very unlikely to be tagged "Ship it" since the reviewer cannot speak to the entire change. The box is left unchecked indicating not that the review fails, but that it is incomplete. On the other hand, a partial review could very easily result in a "Don't ship it": "your use of XYZ API is wrong, you need to call free_blarg() on each item you originally allocated with create_blarg(); you're leaking them like crazy".
If I wanted to be able to give a detailed state along with a comment, those states
would be something like:
( ) I reject this entire change, it's a bad idea in the first place
[no, I don't think we should add a battleship game to the kernel!]
( ) I reject this entire change, good idea but wrong design, try again
[yes this driver needs some manageability; no, don't create a new /proc
hierarchy]
( ) I reject this change due to specific design or code problems (see comment)
[you missed several calls to free_blarg()]
( ) I approve of part of this change but am not qualified/permitted/don't have
time to approve the whole thing
[I approve this for the XYZ team, wait for review from ABC team]
( ) I approve the whole change
[Ship it!]
( ) Just a random comment with no semantic meaning to reviewboard
[I'm glad you're fixing this! Can't comment on the tech, not familiar with
that module]
I agree with #19. Having a Ship it or Sink it will turn a code review into a vote. If more reviewers choose Ship it does it win? I foresee weird issues with having a black and white selection. What if one reviewer Ships and one Sinks? Then what? #19's comment makes much more sense to me. Anyway I think we all agree that the Ship It mechanism could some changes. :)
-
+ Milestone-Release1.6
Basis of problem is in differences between OpenSource and commerical reviewing model. RB support OpenSource review model from nature. When I need update some informations in bug track I made this like this: * When RB is submitted I update issue - review ok * When RB comment is published I check Ship It and: * When checked I don't do anything * When unchecked I update issue - review rejected Of course Ship It is stricly OpenSource function which doesn't have sens in commercial model but I think my idea is fine. But this should be noticed under webhooks implementation to pass enough informations to webhook in this situations.
I do not understand very well your point. How is reviewing different between a "commercial model" versus an "open source model"?
This is complex issue. Reasons of this difference come from variety of reviews occur in Open Source. In open source review system people send various requests. One will be submit, other will be discard. There is also something like "community" which can rate (ship it) idea. In commercial You get client. Only client advance ideas, fixes, features. Before implementing each of them was discussed with architects, payed (by client), approved by project leader and they MUST be implemented. When developer send code to review there is almost no option to discard or delete change. Submiting is the ultimate purpose - always. There is no need to rate this code they are acceptable or not. There is no opportunity to discard review because they only need improvement. Commercial review have very quasistate workflow Submit/Reject nothing more. No discard, no ship it, no sink it, no dammit ranking cause this code only need improvement - nothing more. They can't be discarded because client want it and payed this code.
Jan, this is not an Open Source vs. Commercial thing here. I think you're misunderstanding the history of Review Board and how most people use it. Review Board itself is open source, yes, but was designed to work at companies, and the majority of companies using it do make use of Ship It. Many have asked for a Don't Ship It or some customizable names that can mean something in-between. The usage at your company is clearly very different from most companies I've talked to. In most companies using Review Board, there's no "client" involved. Rather, there's a group of developers, and they work on code (features, bug fixes, architecture improvements, whatever). Sometimes that's on code that their team owns, sometimes it's code other teams own. When they put it up for review, they're asking for a new set of eyes on the code, especially when it's code that they don't own. Other developers mark Ship It when they are comfortable with the code going into the product. That's not much different than how it's used in open source, too. People submit code for review. The core developers and active contributors look over it and mark Ship It when they're comfortable with it. The difference between the two cases is who actually submits the code, but that's not something that Review Board needs to care about. In companies, code gets discarded/deleted all the time. Not all code is good and destined for the product. Sometimes a better idea comes around, or the feature is no longer needed, or it's substantially different, or it needs to be broken up into smaller changes, or senior engineers disapprove of the change, or the intern working on the code misunderstood requirements, or something. Saying that *all* code is destined to be submitted, and never discarded, simply doesn't reflect the reality at most companies. Your company appears to work quite differently, and isn't really the model that we're aiming for with Review Board. It sounds more like a contractor-based setup, where you don't have the say in what does and does not go in (at least on a high level). We can try to do things to improve the workflow there, but it is more of a special case, not the common case.
I work only in one company in my life and this my only experience. I'm surprised what You are talking about but also I'm skewed by my company. Without comparsion this is hard to distinct. Sorry for this misunderstanding.
How about instead of "Ship It", "Don't Ship It", we use +1 / 0 / -1? Examples: I've just finished reviewing some code, and I feel it's not exactly ready for the prime time. I put in my final comments regarding the review request, and then give the review request a "-1". Back in the Dashboard (or while viewing the review request), I can see that the current revision for this review request has a field in the table showing: "0 / -1" meaning that there are no +1's, and a single -1. Another reviewer has a different opinion, however, and after their review, they give a +1. So now, in the Dashboard I see that the latest revision for this review request displays a "score" of "+1 / -1". A third reviewer comes in, and agrees with me, so now we see: "+1 / -2". The reviewee modifies their review request. A new revision is uploaded, so the score is now reset at 0 / 0. Note that a reviewer can also mark a review with "0", in the event that they have I know that there was some concern that this would turn into a vote. I'm pretty sure the Ship It is already a "vote" though. There's just no way to vote the other way. Thoughts?
My preference would be to add a "Hold" button. This would be a generic indicator that could be used for any review to flag at a high level that something is amiss. Comments should accompany the indication detailing why a hold was being requested such as "I want to review but need more time", "Issue below on line xxx needs discussion", or "I think another solution is needed". The semantics of how this flag is used could be defined by the organization. There will also need to be a process to release the Hold.
[Hold] is good. Per-reviewer flag, can be cleared by anyone (leaving a paper trail). Any further semantic process needs to be handled by humans once the review has gotten into this sort of exception state.
All the discussion above highlights a very nice enhancement, but I just thought to put down what I've been doing as a possible work around. In order for code being valid for "shipping" it needs x no of "Ship It!"'s and no "open issues". For example: Dev A comes along and says "Ship It!", but then Dev B finds a problem and opens and issue. Because there are still issues open, the "Ship It!" is not valid and simply comments on the fact that Dev A found it ok to go. This prevents code from being shipped despite someone having clicked "Ship It!". A delete 'Ship It!' would still be nice for accidental clicks, and it would be cool to see the no of open issues on the dashboard.
Here's what I've been thinking: A 3-way toggle between: "Ship it!", "Undecided", and "Hold it!" per reviewer. (See attachment) A reviewer's default state is "Undecided". Maybe each review will also have a counter to indicate the number of "ship it"'s and "hold it"'s the review request has received, and possibly list the reviewers who have given it the ship it's and hold it's. Whoever is pushing the code will also be warned before pushing if the review request has any "hold it"'s, but this will not restrict them from pushing. This will allow people to undo any Ship it/Hold it actions done intentionally or accidentally, or change their stance after a request has been updated or after further review.
-
+ 
-
- Milestone-Release1.7 + Milestone-Release1.8
The whole 'karma' idea is fairly common... see e.g. gerrit, Fedora Bodhi, etc. Although I'm not sure that's actually necessary for RB, since we already have issues. If the presence of unresolved issues precluded a review showing as 'ready' (on dashboards particularly, I would say), I think that would generally suffice. Of course, it would also help if http://code.google.com/p/reviewboard/issues/detail?id=2723 and/or http://code.google.com/p/reviewboard/issues/detail?id=2817 were also implemented. (More generally, creation of an issue or uploading of a new revision should 'disable' any previous 'Ship It' so that they are not counted on the dashboard. This, plus 2723, would allow reviewers to override open issues, but merge-masters would know when this is happening and be able to verify that such an action is appropriate.)
For what it's worth, this situation would be handled fine if there were a "# of Open Issues" column on the dashboard.
When someone else does a "Ship It!" and I disagree, I simply post a review with comments/issues.
The Review Requester will be paying close enough attention to the review to know this information, but members of Review Groups would benefit from seeing "Oh hey, there's a ship it on this _and_ open issues, so there's some possible disagreement. Maybe I should weigh in."
We've made it clear in our ("commercial", btw) organization that some random person's "Ship It!" is not sufficient for you to check in your code. You have to get the "Ship It!" from the _right people_, and you need to give sufficient time for other people to chime in (even if you get a "Ship It!" immediately).
There are a lot of different use-cases of people using ReviewBoard, but adding another approval state "Hold It/Sink It" would only add confusion ("I have a couple of comments that need to be addressed, do I need to Hold It? Only when someone else has Ship It?"). And, the presence of open issues already means it's not ready (If they're not relevant, mark them dropped - otherwise, you need to fix them).
I would expect to get negative feedback from the people using the tool here if "Sink It/Hold It" were to appear (but, we could deal with it).
My $0.02
Good to see this is finally accepted. For our organization it would be important this 'hold it' 'sink it' toggle to be available through the api. Our workflow for illustration purposes: Our svn repository has contains lists of authorized reviewers for certain areas of the code base (developers who know at least a bit about that section of code). To be able to commit to these areas we have a svn precommit hook to check if the commit is authorized by any of these reviewers. due to the size of the project there are often more then one on many parts of the code. Currently we check reviewboard if there is a 'ship-it' by a appropriate person if it is the commit is allowed. (else its refused). The reason we need the hold it is because from time to time one of the reviewers reviews it and fails to find a bug present in the review and check the 'ship it'. if another reviewer finds a bug in the review before its committed (or wants to contest the entire commit somehow) there is currently no way (except email / messages) to prevent the commit. in our case a commit would always be blocked as long as there is at least one hold-it present. (it would basically used as a veto system)
-
- Milestone-Release1.8
Are there any known plugins for this feature available / is this this feature still under consideration?
@pvgodd: looks like this issue got forgotten. Last time it was touched by a project member, it was bound to the 1.8 milestone (after it was repeatedly postponed through every big version milestonse earlier). 1.8 never came, version number went straight to 2.0, which didn't contain the new feature. It is also missing from the release notes of 2.5 Beta 1. BTW, there's another feature request targeting the ship it function, but in a less complicated manner, see issue 3462.
It's not forgotten, just that this is a relatively tricky thing (literally everyone has an opinion about how ship-its should be improved/changed and whatever we do has to be very carefully thought out) and we have fairly limited resources.
Could you design multiple ship it models and have an administration-level option which one to use? It seems like there's a lot of valid use cases, and no single model will solve all the problems. It seems like you've had consistent complaints/frustrating work arounds for 5+ years with the current model. Lots of open issues and duplicate issues. If you added an optional (admin controlled) "Don't Ship It" boolean and an optional "new versions reset ship its/don't ship its" feature, 90% of the complaints I've seen would be addressed and no new ones would show up since it'd be optional?
Having multiple models and a toggle is not a good solution. It makes maintenance much harder (since now we have a bunch more code paths to test), it makes it confusing for users (who may have used Review Board elsewhere and be surprised by things working differently), and it doesn't account for the fact that multiple teams within the same company might have different preferences.
What we have now is obviously not an optimal design, but our time is very limited and we haven't had the chance to go through and really re-think this.