Reviews on Github

Horacio Duran horacio.duran at canonical.com
Wed Sep 21 23:59:16 UTC 2016 

Well I like to do my squashing because I tend to have some meaningless
commit messages in the middle, I don't think project history cares about me
going to the supermarket, we could make it use the gh PR message though

On Wednesday, 21 September 2016, Reed O'Brien <reed.obrien at canonical.com>
wrote:

> On Wed, Sep 21, 2016 at 4:43 PM, Horacio Duran <
> horacio.duran at canonical.com
> <javascript:_e(%7B%7D,'cvml','horacio.duran at canonical.com');>> wrote:
>
>> I disagree, once the discussion is over and the merge is ready, an
>> amend/squash is in order to avoid useless tree nodes.
>>
> You make a good point and I agree with you, but I think the landing bot
> should squash the commits at merge. I suppose if GH improves the UI around
> comments, reviews, and commits it would be less surprising. I can also live
> with it once I know how to recognized it happened TTW.
>
> Obviously we need to use gerrit. /me ducks...
>
>
>>
>> On Wed, Sep 21, 2016 at 8:41 PM, Reed O'Brien <reed.obrien at canonical.com
>> <javascript:_e(%7B%7D,'cvml','reed.obrien at canonical.com');>> wrote:
>>
>>> Two things I've noticed today while doing OCR duties are:
>>> 1. There's no way to tell if a PR has a review when looking at the list
>>> of open PRs. I may be missing something or it may be an oversight on the
>>> part of GH and will likely be remedied soon. When there are comments it
>>> shows little comment clouds and a count, but not if there's only a review
>>> nothing shows there for me.
>>> 2. When someone amends a commit and force pushes, the review remains but
>>> isn't attached to a commit any longer. You can see that there was an update
>>> because the commit appears later in the timeline than the review on the PR
>>> details view. Personally, I think that once you make a PR and involve
>>> someone else we shouldn't rewrite history anymore.
>>>
>>>
>>> On Wed, Sep 21, 2016 at 4:37 AM, Andrew Wilkins <
>>> andrew.wilkins at canonical.com
>>> <javascript:_e(%7B%7D,'cvml','andrew.wilkins at canonical.com');>> wrote:
>>>
>>>> On Wed, Sep 21, 2016 at 5:53 AM Menno Smits <menno.smits at canonical.com
>>>> <javascript:_e(%7B%7D,'cvml','menno.smits at canonical.com');>> wrote:
>>>>
>>>>> Some of us probably got a little excited (me included). There should
>>>>> be discussion and a clear announcement before we make a signigicant change
>>>>> to our process. The tech board meeting is today/tonight so we'll discuss it
>>>>> there as per Rick's email. Please contribute to this thread if you haven't
>>>>> already and have strong opinions either way on the topic.
>>>>>
>>>>
>>>> We discussed Github reviews vs. Reviewboard at the tech board meeting
>>>> today, and we all agreed that we should go ahead with a trial for 2 weeks.
>>>>
>>>> There are pros and cons to each; neither is perfect. You can find the
>>>> main points of discussion in the tech board agenda.
>>>>
>>>> Please give it a shot and provide your criticisms so we decide on the
>>>> best path forward at the end of the trial.
>>>>
>>>> Cheers,
>>>> Andrew
>>>>
>>>> Interestingly our Github/RB integration seems to have broken a little
>>>>> since Github made these changes. The links to Reviewboard on pull requests
>>>>> aren't getting inserted any more. If we decide to stay with RB
>>>>>
>>>>> On 21 September 2016 at 05:54, Rick Harding <
>>>>> rick.harding at canonical.com
>>>>> <javascript:_e(%7B%7D,'cvml','rick.harding at canonical.com');>> wrote:
>>>>>
>>>>>> I spoke with Alexis today about this and it's on her list to check
>>>>>> with her folks on this. The tech board has been tasked with he decision, so
>>>>>> please feel free to shoot a copy of your opinions their way. As you say, on
>>>>>> the one hand it's a big impact on the team, but it's also a standard
>>>>>> developer practice that not everyone will agree with so I'm sure the tech
>>>>>> board is a good solution to limiting the amount of bike-shedding and to
>>>>>> have some multi-mind consensus.
>>>>>>
>>>>>> On Tue, Sep 20, 2016 at 1:52 PM Katherine Cox-Buday <
>>>>>> katherine.cox-buday at canonical.com
>>>>>> <javascript:_e(%7B%7D,'cvml','katherine.cox-buday at canonical.com');>>
>>>>>> wrote:
>>>>>>
>>>>>>> Seems like a good thing to do would be to ensure the tech board
>>>>>>> doesn't have any objections and then put it to a vote since it's more a
>>>>>>> property of the team and not the codebase.
>>>>>>>
>>>>>>> I just want some consistency until a decision is made. E.g. "we will
>>>>>>> be trying out GitHub reviews for the next two weeks; all reviews should be
>>>>>>> done on there".
>>>>>>>
>>>>>>> --
>>>>>>> Katherine
>>>>>>>
>>>>>>> Nate Finch <nate.finch at canonical.com
>>>>>>> <javascript:_e(%7B%7D,'cvml','nate.finch at canonical.com');>> writes:
>>>>>>>
>>>>>>> > Can we try reviews on github for a couple weeks? Seems like we'll
>>>>>>> > never know if it's sufficient if we don't try it. And there's no
>>>>>>> setup
>>>>>>> > cost, which is nice.
>>>>>>> >
>>>>>>> > On Tue, Sep 20, 2016 at 12:44 PM Katherine Cox-Buday
>>>>>>> > <katherine.cox-buday at canonical.com
>>>>>>> <javascript:_e(%7B%7D,'cvml','katherine.cox-buday at canonical.com');>>
>>>>>>> wrote:
>>>>>>> >
>>>>>>> >     I see quite a few PRs that are being reviewed in GitHub and not
>>>>>>> >     ReviewBoard. I really don't care where we do them, but can we
>>>>>>> >     please pick a direction and move forward? And until then, can
>>>>>>> we
>>>>>>> >     stick to our previous decision and use RB? With people using
>>>>>>> both
>>>>>>> >     it's much more difficult to tell what's been reviewed and what
>>>>>>> >     hasn't.
>>>>>>> >
>>>>>>> >     --
>>>>>>> >     Katherine
>>>>>>> >
>>>>>>> >     Nate Finch <nate.finch at canonical.com
>>>>>>> <javascript:_e(%7B%7D,'cvml','nate.finch at canonical.com');>> writes:
>>>>>>> >
>>>>>>> >     > 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
>>>>>>> <javascript:_e(%7B%7D,'cvml','Juju-dev at lists.ubuntu.com');>
>>>>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
>>>>>>> an/listinfo/juju-dev
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Juju-dev mailing list
>>>>>> Juju-dev at lists.ubuntu.com
>>>>>> <javascript:_e(%7B%7D,'cvml','Juju-dev at lists.ubuntu.com');>
>>>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
>>>>>> an/listinfo/juju-dev
>>>>>>
>>>>>>
>>>>> --
>>>>> Juju-dev mailing list
>>>>> Juju-dev at lists.ubuntu.com
>>>>> <javascript:_e(%7B%7D,'cvml','Juju-dev at lists.ubuntu.com');>
>>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
>>>>> an/listinfo/juju-dev
>>>>>
>>>>
>>>> --
>>>> Juju-dev mailing list
>>>> Juju-dev at lists.ubuntu.com
>>>> <javascript:_e(%7B%7D,'cvml','Juju-dev at lists.ubuntu.com');>
>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
>>>> an/listinfo/juju-dev
>>>>
>>>>
>>>
>>>
>>> --
>>> Reed O'Brien
>>> ✉ reed.obrien at canonical.com
>>> <javascript:_e(%7B%7D,'cvml','reed.obrien at canonical.com');>
>>> ✆ 415-562-6797
>>>
>>>
>>> --
>>> Juju-dev mailing list
>>> Juju-dev at lists.ubuntu.com
>>> <javascript:_e(%7B%7D,'cvml','Juju-dev at lists.ubuntu.com');>
>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
>>> an/listinfo/juju-dev
>>>
>>>
>>
>
>
> --
> Reed O'Brien
> ✉ reed.obrien at canonical.com
> <javascript:_e(%7B%7D,'cvml','reed.obrien at canonical.com');>
> ✆ 415-562-6797
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160921/3aaa05f1/attachment-0001.html>

More information about the Juju-dev mailing list