Reviews on Github

[Anastasia Macmood]

On 16/09/16 08:02, Ian Booth wrote:
> Another data point - in the past, when we have had PRs which touch a lot of
> files (eg change the import path for a dependency), review board paginates the
> diff so it's much easier to manage, whereas I've seen github actually truncate
> what it displays because the diff is "too large". Hopefully this will no longer
> be an issue, or else we won't be able to review such changes in the future.
This is perfect to reduce the size of our proposals to manageable :)
>
> On 16/09/16 07:53, Menno Smits wrote:
>> Although I share some of Ian's concerns, I think the reduced moving parts,
>> improved reliability, reduced maintenance, easier experience for outside
>> contributors and better handling of file moves are pretty big wins. The
>> rendering of diffs on Github is a whole lot nicer as well.
>>
>> I'm +1 for trialling the new review system on Github for a couple of weeks
>> as per Andrew's suggestion.
>>
>> On 16 September 2016 at 05:50, Nate Finch <nate.finch at canonical.com> wrote:
>>
>>> Reviewboard goes down a couple times a month, usually from lack of disk
>>> space or some other BS.  According to a source knowledgeable with these
>>> matters, the charm was rushed out, and the agent for that machine is down
>>> anyway, so we're kinda just waiting for the other shoe to drop.
>>>
>>> As for the process things that Ian mentioned, most of those can be
>>> addressed with a sprinkling of convention.  Marking things as issues could
>>> just be adding :x: to the first line (github even pops up suggestions and
>>> auto-completes), thusly:
>>>
>>> [image: :x:]This will cause a race condition
>>>
>>> And if you want to indicate you're dropping a suggestion, you can use :-1:
>>>  which gives you a thumbs down:
>>>
>>> [image: :-1:] I ran the race detector and it's fine.
>>>
>>> It won't give you the cumulative "what's left to fix" at the top of the
>>> page, like reviewboard... but for me, I never directly read that, anyway,
>>> just used it to see if there were zero or non-zero comments left.
>>>
>>> As for the inline comments in the code - there's a checkbox to hide them
>>> all.  It's not quite as convenient as the gutter indicators per-comment,
>>> but it's sufficient, I think.
>>>
>>> On Wed, Sep 14, 2016 at 6:43 PM Ian Booth <ian.booth at canonical.com> wrote:
>>>
>>>>
>>>> On 15/09/16 08:22, Rick Harding wrote:
>>>>> I think that the issue is that someone has to maintain the RB and the
>>>>> cost/time spent on that does not seem commensurate with the bonus
>>>> features
>>>>> in my experience.
>>>>>
>>>> The maintenance is not that great. We have SSO using github credentials so
>>>> there's really no day to day works IIANM. As a team, we do many, many
>>>> reviews
>>>> per day, and the features I outlined are significant and things I (and I
>>>> assume
>>>> others) rely on. Don't under estimate the value in knowing why a comment
>>>> was
>>>> rejected and being able to properly track that. Or having review comments
>>>> collapsed as a gutter indicated so you can browse the code and still know
>>>> that
>>>> there's a comment there. With github, you can hide the comments but
>>>> there's no
>>>> gutter indicator. All these things add up to a lot.
>>>>
>>>>
>>>>> On Wed, Sep 14, 2016 at 6:13 PM Ian Booth <ian.booth at canonical.com>
>>>> wrote:
>>>>>> One thing review board does better is use gutter indicators so as not
>>>> to
>>>>>> interrupt the flow of reading the code with huge comment blocks. It
>>>> also
>>>>>> seems
>>>>>> much better at allowing previous commits with comments to be viewed in
>>>>>> their
>>>>>> entirety. And it allows the reviewer to differentiate between issues
>>>> and
>>>>>> comments (ie fix this vs take note of this), plus it allows the notion
>>>> of
>>>>>> marking stuff as fixed vs dropped, with a reason for dropping if
>>>> needed.
>>>>>> So the
>>>>>> github improvements are nice but there's still a large and significant
>>>> gap
>>>>>> that
>>>>>> is yet to be filled. I for one would miss all the features reviewboard
>>>>>> offers.
>>>>>> Unless there's a way of doing the same thing in github that I'm not
>>>> aware
>>>>>> of.
>>>>>>
>>>>>> On 15/09/16 07:22, Tim Penhey wrote:
>>>>>>> I'm +1 if we can remove the extra tools and we don't get email per
>>>>>> comment.
>>>>>>> On 15/09/16 08:03, Nate Finch wrote:
>>>>>>>> In case you missed it, Github rolled out a new review process.  It
>>>>>>>> basically works just like reviewboard does, where you start a review,
>>>>>>>> batch up comments, then post the review as a whole, so you don't just
>>>>>>>> write a bunch of disconnected comments (and get one email per review,
>>>>>>>> not per comment).  The only features reviewboard has is the edge case
>>>>>>>> stuff that we rarely use:  like using rbt to post a review from a
>>>> random
>>>>>>>> diff that is not connected directly to a github PR. I think that is
>>>> easy
>>>>>>>> enough to give up in order to get the benefit of not needing an
>>>> entirely
>>>>>>>> separate system to handle reviews.
>>>>>>>>
>>>>>>>> I made a little test review on one PR here, and the UX was almost
>>>>>>>> exactly like working in reviewboard:
>>>>>> https://github.com/juju/juju/pull/6234
>>>>>>>> There may be important edge cases I'm missing, but I think it's worth
>>>>>>>> looking into.
>>>>>>>>
>>>>>>>> -Nate
>>>>>>>>
>>>>>>>>
>>>>>> --
>>>>>> Juju-dev mailing list
>>>>>> Juju-dev at lists.ubuntu.com
>>>>>> Modify settings or unsubscribe at:
>>>>>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>>>>>
>>>> --
>>>> Juju-dev mailing list
>>>> Juju-dev at lists.ubuntu.com
>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/
>>>> mailman/listinfo/juju-dev
>>>>
>>> --
>>> Juju-dev mailing list
>>> Juju-dev at lists.ubuntu.com
>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/
>>> mailman/listinfo/juju-dev
>>>
>>>