Re: Extended Ship it feature, Defaults for diff base path
On Mon, Jan 26, 2009 at 3:23 AM, Sebastian Kurfuerst sebastian.kurfue...@gmail.com wrote: Hey Christian, thanks for your super-fast response! Right now, we don't have anything in the code to enforce policies such as requiring two ship its before you can close out a review request. Since Review Board doesn't actually commit reviews, we can't enforce anything like that. Yep, that was a slight misunderstanding. We neither need such a policy to _enforce_ something. It's a common request so I thought I'd spend a minute going into that :) When you talk about a global status for a review, what exactly do you mean? Can you describe how this would best work for you? OK. Currently, we have a mailing list for posting patches to, and as said, we require two +1s - at least one from a core developer. Now, many patches are still lying around as it sometimes takes a lot of time to review complicated changes. Currently, it is basically impossible to get a list of issues which still need one review - or a list of issues which are ready to commit. What I mean with global status: - In a basic version, it would be nice to display the summed-up ranking of a feature in the summary of the feature - and as a column in the overview. So, if a feature has 3 times Ship it, it should display +3 (or something similar). - In an advanced version, it'd be nice to implement own strategies (as a Python module f.e.) for calculation of this Status value - so I could have a strategy which also implements the above-described policy. As Paul Scott mentioned, you could write something to do some of this now, but you'd have to write custom code to use the API. Another feature planned after 1.0 is support for custom third-party extensions, so in time you'd be able to make such a thing part of Review Board while keeping it specific to your setup. - Is it possible to have a default diff base path depending on which repository you select? (We require all diffs to be made from a certain directory). Thus, our diff base path is in most cases /. We certainly could use a better default. There is none right now. OK - I'd just need to add this default in the Models.py, right? Or would you make it configurable? You'd modify diffviewer/forms.py (maybe reviews/forms.py) to add a default for the base diff path. Doing it properly means a few things: 1) Adding a per-repository default, which means a new field in scmtools/models.py 2) Add a field to DiffSet in diffviewer/models.py for specifying what the base diff path was for that upload, and then re-using it the next time the user goes to upload something. - Is it possible to have default reviewers selected depending on the Repository selected? Sort of. We have support for default reviewers for file paths, but they're global across repositories. We could expand this for the next release, unless someone wants to do the work for this one. I could again do that, but I am quite unfamiliar with Python - so I doubt I'd get it right in the first place. Essentially, it would be enough to have a second field which is called Repository RegEx, which is checked as well if non-empty. Right? We'd need a ManyToManyField in reviews/models.py for DefaultReviewer. A particular DefaultReviewer should be able to be configured with one or more repositories that way. Then we'd just need to check the repositories in the code that uses DefaultReviewer. Review Board's pretty much a part-time project of David Trowbridge and mine, and we're in general quite busy, with a large to-do list, so I can't promise we'll get to all of those right away. Many people who use Review Board and need new behavior contribute patches, which is the fastest way to get such things in the code base. I should also point out that we're entering a feature-freeze period soon for our 1.0 release, so we'll soon start pushing new features back. Yep. When do you enter it? It'd be nice if some stuff (like the summation of Ship it) could already enter trunk :-) We won't be able to get that in for 1.0. We're entering freeze within a week or two, I imagine. There's a few key things we're going to try to get in, but the Ship It stuff is going to require more thought and time than I feel comfortable doing right now. Christian --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Extended Ship it feature, Defaults for diff base path
Hi Sebastian. On Mon, Jan 26, 2009 at 2:37 AM, Sebastian Kurfuerst sebastian.kurfue...@gmail.com wrote: Hello Reviewboard Developers! We at the TYPO3 Project are currently evaluating ReviewBoard for our patch workflow. As far as I see it, I am really impressed! There are just minor details which would be nice to have: - In our workflow, we require that there are at least 2 +1s (or ship it), where one of these +1's has to come from a special user group (core developers). It would be nice to have some global status for each Review, which displays the overall status of this review. (Which should also be shown in the Report listings in the table) Right now, we don't have anything in the code to enforce policies such as requiring two ship its before you can close out a review request. Since Review Board doesn't actually commit reviews, we can't enforce anything like that. When you talk about a global status for a review, what exactly do you mean? Can you describe how this would best work for you? - We also have something like -1 - I am strongly opposed to letting this feature come into the core. Could this be done as well? (However, that's not as important as the first one.) This has been requested by other people as well. We won't be getting to it for 1.0, but it's planned for the following release. Right now, we just suggest requiring reviewers to state that in the review. Since Review Board is used in different companies/projects with different policies, it's difficult to really start implementing a policy model that works for everyone, so we tend to suggest that people enforce their own policies using text in the review. If a developer has a review from a core developer stating that it's not ready to go in, it just shouldn't go in. Again, since Review Board doesn't commit changes, this has to be enforced internally anyway. We can certainly try to help where we can, though. My thoughts on the -1 is to give administrators the ability to specify a list of states. By default, there could be Ship It and Don't ship it, but companies could add such things as Wrong direction or Hold off for now. Each would have a weight, and we'd reflect that in the dashboard and on the review request. - Is it possible to have a default diff base path depending on which repository you select? (We require all diffs to be made from a certain directory). Thus, our diff base path is in most cases /. We certainly could use a better default. There is none right now. What I'd strongly recommend is using post-review to handle creating review requests and updating diffs. It will hide the whole base path from you, and makes it much easier to work with Review Board. See http://www.review-board.org/docs/Using_PostReview. - Is it possible to have default reviewers selected depending on the Repository selected? Sort of. We have support for default reviewers for file paths, but they're global across repositories. We could expand this for the next release, unless someone wants to do the work for this one. Thanks for your ideas! It will be a real pleasure to work with your product! I hope some of the above will find its way into the trunk, and then we'll definitely use it. I hope you do. :) Review Board's pretty much a part-time project of David Trowbridge and mine, and we're in general quite busy, with a large to-do list, so I can't promise we'll get to all of those right away. Many people who use Review Board and need new behavior contribute patches, which is the fastest way to get such things in the code base. I should also point out that we're entering a feature-freeze period soon for our 1.0 release, so we'll soon start pushing new features back. If you can use Review Board without the -1 support and with using post-review for now to get a better feel for your needs, file bugs/feature requests, and maybe continue posting more useful e-mails like this, then that would help us to design these features for the next release. For other people on the mailing list who also want these capabilities and have specific needs, let us know too! Thanks, Christian --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Extended Ship it feature, Defaults for diff base path
Hey Christian, thanks for your super-fast response! Right now, we don't have anything in the code to enforce policies such as requiring two ship its before you can close out a review request. Since Review Board doesn't actually commit reviews, we can't enforce anything like that. Yep, that was a slight misunderstanding. We neither need such a policy to _enforce_ something. When you talk about a global status for a review, what exactly do you mean? Can you describe how this would best work for you? OK. Currently, we have a mailing list for posting patches to, and as said, we require two +1s - at least one from a core developer. Now, many patches are still lying around as it sometimes takes a lot of time to review complicated changes. Currently, it is basically impossible to get a list of issues which still need one review - or a list of issues which are ready to commit. What I mean with global status: - In a basic version, it would be nice to display the summed-up ranking of a feature in the summary of the feature - and as a column in the overview. So, if a feature has 3 times Ship it, it should display +3 (or something similar). - In an advanced version, it'd be nice to implement own strategies (as a Python module f.e.) for calculation of this Status value - so I could have a strategy which also implements the above-described policy. Again, since Review Board doesn't commit changes, this has to be enforced internally anyway. Definitely. We can certainly try to help where we can, though. My thoughts on the -1 is to give administrators the ability to specify a list of states. By default, there could be Ship It and Don't ship it, but companies could add such things as Wrong direction or Hold off for now. Each would have a weight, and we'd reflect that in the dashboard and on the review request. That'd be really perfect! - Is it possible to have a default diff base path depending on which repository you select? (We require all diffs to be made from a certain directory). Thus, our diff base path is in most cases /. We certainly could use a better default. There is none right now. OK - I'd just need to add this default in the Models.py, right? Or would you make it configurable? What I'd strongly recommend is using post-review to handle creating review requests and updating diffs. It will hide the whole base path from you, and makes it much easier to work with Review Board. Seehttp://www.review-board.org/docs/Using_PostReview. Thanks, I'll look at this. - Is it possible to have default reviewers selected depending on the Repository selected? Sort of. We have support for default reviewers for file paths, but they're global across repositories. We could expand this for the next release, unless someone wants to do the work for this one. I could again do that, but I am quite unfamiliar with Python - so I doubt I'd get it right in the first place. Essentially, it would be enough to have a second field which is called Repository RegEx, which is checked as well if non-empty. Right? I hope you do. :) Me too! Review Board's pretty much a part-time project of David Trowbridge and mine, and we're in general quite busy, with a large to-do list, so I can't promise we'll get to all of those right away. Many people who use Review Board and need new behavior contribute patches, which is the fastest way to get such things in the code base. I should also point out that we're entering a feature-freeze period soon for our 1.0 release, so we'll soon start pushing new features back. Yep. When do you enter it? It'd be nice if some stuff (like the summation of Ship it) could already enter trunk :-) If you can use Review Board without the -1 support and with using post-review for now to get a better feel for your needs, file bugs/feature requests, and maybe continue posting more useful e-mails like this, then that would help us to design these features for the next release. Thanks a lot :-) Btw - I tried post-review on Mac, and it does not find simpleJSON. Does someone have a mac here, as well, and knows how to make it work with the shipped python? Thanks a lot for your response, Sebastian --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
RE: Extended Ship it feature, Defaults for diff base path
Sebastian, OK. Currently, we have a mailing list for posting patches to, and as said, we require two +1s - at least one from a core developer. Now, many patches are still lying around as it sometimes takes a lot of time to review complicated changes. Currently, it is basically impossible to get a list of issues which still need one review - or a list of issues which are ready to commit. Assuming that the Review Board (json?) APIs are still in a good state, you could probably use them to get all the data you need to generate a report/dashboard which presents the still needs one review list you're looking for. Regardless of how Review Board's standard dashboard works, you can use that page to present the information in a way that fits best with your group's policies, and calculate your numerical status value via whatever scheme you want. (And if you can pull in information from a database with a list of your core developers, you should be able to come up with your ready for checkin page too.) I know here at VMware we're planning on using data pulled from Review Board in both our more global CM dashboard and our checkin control system (which will likely do actual enforcement of a policy not too dissimilar from yours on some of our more locked-down release branches). Paul Scott SCM Engineer VMware --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
