Reviews on Github

Andrew Wilkins andrew.wilkins at canonical.com
Thu Sep 22 01:02:19 UTC 2016 

On Thu, Sep 22, 2016 at 7:59 AM Horacio Duran <horacio.duran at canonical.com>
wrote:

> 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
>

I usually rebase and force-push after addressing review comments too. If we
stick with GitHub reviews, the squashing probably should be done just
before merging (or by the bot).


> 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> 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
>>> > 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.
>>>>
>>>
Indeed, this isn't great for when you're OCR.


> 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> wrote:
>>>>
>>>>> On Wed, Sep 21, 2016 at 5:53 AM Menno Smits <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> 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> 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> 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> 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> 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
>>>>>>>> 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
>>>>>>
>>>>>
>>>>> --
>>>>> Juju-dev mailing list
>>>>> Juju-dev at lists.ubuntu.com
>>>>> Modify settings or unsubscribe at:
>>>>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Reed O'Brien
>>>> ✉ reed.obrien at canonical.com
>>>> ✆ 415-562-6797
>>>>
>>>>
>>>> --
>>>> Juju-dev mailing list
>>>> Juju-dev at lists.ubuntu.com
>>>> Modify settings or unsubscribe at:
>>>> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>>>>
>>>>
>>>
>>
>>
>> --
>> Reed O'Brien
>> ✉ reed.obrien at canonical.com
>> ✆ 415-562-6797
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160922/6c19a3f3/attachment.html>

More information about the Juju-dev mailing list