Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Mon, Jun 12, 2017 at 9:37 AM Dave Barnes  wrote:

> Proposal as written says "...for changes that would require peer review
> before committing...".
> If this means that minor changes (in my case, typo repairs are the common
> case) can be checked in without going through a PR process, I can go with
> the group decision.
>

The proposal only changes the method for items that would have normally
gone through a review board. It isn't proposing a change to the items that
are reviewed.

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Mon, Jun 12, 2017 at 7:57 AM Bruce Schuchardt 
wrote:

> It places an unnecessary burden on committers


Considering committers need to use PR to commit changes from non-committers
how does reducing the number of review systems increase the burden on
committers?


> and git history is the
> definitive record of changes to the code so github pr history isn't
> really very useful.


All unsquashed commit history is preserved through a PR.

Re: [DISCUSS] Using Pull Requests over Review Board

[Galen M O'Sullivan]

One feature I like about PRs on GitHub that I haven't figured out how to do
on Review Board is breaking up a large changeset (which I would like to
have reviewed together) into multiple commits. It can be a useful way to
tell a story about your changes, but keep the review for them in one place.

I'm +0.5 for using GitHub for all code reviews. It's not open source, but
it's free as in beer (well, sort of), and it works well. Having one system
instead of two would be nice and could help make our lives simpler.

On Thu, Jun 8, 2017 at 2:41 PM, Jacob Barrett  wrote:

> On Thu, Jun 8, 2017 at 2:24 PM Nabarun Nag  wrote:
>
> > Also, IMHO feature branches from which the PRs are created should be in
> our
> > personal fork rather than the main geode git repo.
> >
>
> +1 - I had planned to bring this up in a separate discussion but yes, I
> think all work should happen out of your personal fork as though you are a
> non-committer contributor for consistency. The JIRA issue didn't cross my
> mind because I have a filter to delete all JIRA notifications that aren't
> for new tickets or ticket I am watching.
>
> -Jake
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Thu, Jun 8, 2017 at 2:24 PM Nabarun Nag  wrote:

> Also, IMHO feature branches from which the PRs are created should be in our
> personal fork rather than the main geode git repo.
>

+1 - I had planned to bring this up in a separate discussion but yes, I
think all work should happen out of your personal fork as though you are a
non-committer contributor for consistency. The JIRA issue didn't cross my
mind because I have a filter to delete all JIRA notifications that aren't
for new tickets or ticket I am watching.

-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[Nabarun Nag]

+1 on using PR.
We can use the @tags and the github notification page[Participating tag] to
check the PRs that need our attention.

Also, IMHO feature branches from which the PRs are created should be in our
personal fork rather than the main geode git repo. Because when we push a
branch called feature/GEODE- into the main apache geode repo, the JIRA
system links the ticket to that branch and every commit/merge operation on
that branch is logged in the comment section of ticket. In my opinion, the
JIRA tickets should have clean and concise history. It should not contain
history / commits of branch that will be deleted eventually , or commits
that will be eventually squashed to a single commit message before merging
to the main develop.

Regards
Naba




On Thu, Jun 8, 2017 at 12:39 PM Jacob Barrett  wrote:

> On Thu, Jun 8, 2017 at 12:35 PM Mark Bretl  wrote:
>
> > I like the functionality we get with GitHub, including the Travis CI
> > integration.
> >
> > Do we have a proposed workflow change for committers?
>
>
> The proposed workflow change to committers is to use the same workflow as
> contributors. The only difference is after sufficient review of the PR you
> could commit it yourself.
>
> -Jake
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Thu, Jun 8, 2017 at 12:35 PM Mark Bretl  wrote:

> I like the functionality we get with GitHub, including the Travis CI
> integration.
>
> Do we have a proposed workflow change for committers?


The proposed workflow change to committers is to use the same workflow as
contributors. The only difference is after sufficient review of the PR you
could commit it yourself.

-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[Mark Bretl]

I like the functionality we get with GitHub, including the Travis CI
integration.

Do we have a proposed workflow change for committers?

--Mark

On Thu, Jun 8, 2017 at 11:20 AM, Jacob Barrett  wrote:

> Some more fuel on this fire... PR's get checked against Travis CI
> automatically and the results show up in the PR. This is a good safety
> check even for committers before merging their branch into the development
> branch.
>
> -Jake
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

Some more fuel on this fire... PR's get checked against Travis CI
automatically and the results show up in the PR. This is a good safety
check even for committers before merging their branch into the development
branch.

-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Thu, Jun 8, 2017 at 10:20 AM Dan Smith  wrote:

> One thing that can
> help is if you add your github id to your apache profile - the PR will then
> show that it is coming from an apache member.


To illustrate what Dan is saying, notice the "Member" tag on my comment in
the screen cap below.
[image: Screen Shot 2017-06-08 at 11.08.22 AM.png]


-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[Ernest Burghardt]

+1 for just PRs

+1 for trying the functionality Jared has pointed out in Github

On Thu, Jun 8, 2017 at 10:19 AM, Jared Stewart  wrote:

>
> On Jun 8, 2017, at 8:32 AM, Bruce Schuchardt 
> wrote:
>
> I also don't see any way to push a PR to specific individuals for review.
> In Reviewboard there is a nice queue of pending reviews that I can go
> through.  On github they're all mixed together and it's difficult to tell
> whether any of them are relevant to me.
>
>
> Github has functionality for this (notice the “assignees” section on the
> right hand side of this PR).  I believe it may be a setting in our
> repository that is preventing us from actually making such assignments.
>
>
>
> I like the idea of a single source of history for reviews but I don't much
> like the idea of having to create PRs on a read-only system and then merge
> my changes to ASF's repo.  Being able to commit directly seems like a
> committer perk that your idea would take away from us.
>
> Can you expand a bit about what you mean by this?  It would seem to me
> that whether we use ReviewBoard or PRs the flow is essentially the same for
> committers:
> 1) Post review request (either from a diff on Reviewboard or from a branch
> on a PR)
> 2a) Receive feedback
> 2b) Update review request (with a new diff on Reviewboard or by pushing
> new commits to the branch of your PR)
> 3) Merge your changes into develop and push them to the ASF repo
>
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Dan Smith]

+1 to just using PRs.

I think the benefits to new people to the project make it worth it to
switch. New people will see PRs from committers when they are creating
their PRs, which will help provide good examples. It's one less system to
sign up on as a contributor so it's easier for new people to find and
comment on committer PRs.

Personally I've switched from reviewboard to PRs and it works well for me.
You can just mention people in comments to ask for a review. You can get
your nice "queue" view, just go to https://github.com/pulls/mentioned to
see all of the open PRs where you are mentioned.

I agree it would help to have a convention to know if a PR is from a
committer or not. So far I think we've just relied on the fact that the
committers generally know who the other committers are. One thing that can
help is if you add your github id to your apache profile - the PR will then
show that it is coming from an apache member.

-Dan



On Thu, Jun 8, 2017 at 10:05 AM, Jacob Barrett  wrote:

> On Thu, Jun 8, 2017 at 8:32 AM Bruce Schuchardt 
> wrote:
>
> > One thing I find confusing in PRs is whether the person submitting the
> > request is a committer or not.  Non-committers need someone to merge the
> > PR while committers are just looking for a review and will merge the
> > changes to develop themselves.
> >
>
> I don't see how PR complicates the process for committers since all work
> should be on a topic branch and would be merged by the committer after
> review (PR or Review Board). The process of merging the changes does not
> change.
>
>
> > I also don't see any way to push a PR to specific individuals for
> > review.  In Reviewboard there is a nice queue of pending reviews that I
> > can go through.  On github they're all mixed together and it's difficult
> > to tell whether any of them are relevant to me.
> >
>
> While PR doesn't have a nice queue concept it does allow you call out
> individuals by mentioning them in comments.
>
>
> > I like the idea of a single source of history for reviews but I don't
> > much like the idea of having to create PRs on a read-only system and
> > then merge my changes to ASF's repo.  Being able to commit directly
> > seems like a committer perk that your idea would take away from us.
> >
>
> Yes, I wish the PR system was not read only with ASF but having used this
> proposed method extensively with geode-native I really find it not to have
> a consistent single place to do reviews of bother peer committers and
> contributors.
>
> Another argument for this proposal is that individual contributors don't
> have to learn two different review methods to review committer's works nor
> do they have to change review methods if they are voted in as committers.
>
> -Jake
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Thu, Jun 8, 2017 at 8:32 AM Bruce Schuchardt 
wrote:

> One thing I find confusing in PRs is whether the person submitting the
> request is a committer or not.  Non-committers need someone to merge the
> PR while committers are just looking for a review and will merge the
> changes to develop themselves.
>

I don't see how PR complicates the process for committers since all work
should be on a topic branch and would be merged by the committer after
review (PR or Review Board). The process of merging the changes does not
change.


> I also don't see any way to push a PR to specific individuals for
> review.  In Reviewboard there is a nice queue of pending reviews that I
> can go through.  On github they're all mixed together and it's difficult
> to tell whether any of them are relevant to me.
>

While PR doesn't have a nice queue concept it does allow you call out
individuals by mentioning them in comments.


> I like the idea of a single source of history for reviews but I don't
> much like the idea of having to create PRs on a read-only system and
> then merge my changes to ASF's repo.  Being able to commit directly
> seems like a committer perk that your idea would take away from us.
>

Yes, I wish the PR system was not read only with ASF but having used this
proposed method extensively with geode-native I really find it not to have
a consistent single place to do reviews of bother peer committers and
contributors.

Another argument for this proposal is that individual contributors don't
have to learn two different review methods to review committer's works nor
do they have to change review methods if they are voted in as committers.

-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[Dave Barnes]

I've used both PRs and Review Boards for doc changes.
The Review Board's targeted reviewer list, as Bruce points out, is a plus.
It would be great if PRs could do that.

On Thu, Jun 8, 2017 at 9:28 AM, John Blum  wrote:

> +1 to Bruce's comments.
>
> PRs are for contributors that do not have commit privileges.  ReviewBoard
> is a tool for "reviewing" changes.
>
> However, what is also common practice on open source projects, even for
> committers, is to create a topic branch containing the commit with the
> desired changes (labeled with an appropriate JIRA ticket + description).
> Then a reviewer can then pick up the topic branch, review the code changes,
> polish things up and even merge the topic branch back into the mainline
> (e.g. develop) and close the ticket.  IDEs, more than ReviewBoard/FishEye,
> etc, have better tooling for reviewing diffs, making change, running tests,
> etc.
>
> -John
>
>
>
> On Thu, Jun 8, 2017 at 8:32 AM, Bruce Schuchardt 
> wrote:
>
> > One thing I find confusing in PRs is whether the person submitting the
> > request is a committer or not.  Non-committers need someone to merge the
> PR
> > while committers are just looking for a review and will merge the changes
> > to develop themselves.
> >
> > I also don't see any way to push a PR to specific individuals for review.
> > In Reviewboard there is a nice queue of pending reviews that I can go
> > through.  On github they're all mixed together and it's difficult to tell
> > whether any of them are relevant to me.
> >
> > I like the idea of a single source of history for reviews but I don't
> much
> > like the idea of having to create PRs on a read-only system and then
> merge
> > my changes to ASF's repo.  Being able to commit directly seems like a
> > committer perk that your idea would take away from us.
> >
> >
> > On 6/7/17 6:42 PM, Jacob Barrett wrote:
> >
> >> All,
> >>
> >> I would like to discuss transitioning all code reviews to pull requests
> >> over using review board. For non-committer community members we ask them
> >> to
> >> make pull requests against the mirrored GitHub repo. Some committers use
> >> pull requests for their own work reviews while others use review board.
> I
> >> propose that we just use on and that the one we use be pull requests. It
> >> would give us a single source of history for reviews, a single model to
> >> understand for reviewers and committers and keep workflow consistent
> with
> >> all contributors, committers or not.
> >>
> >> -Jake
> >>
> >>
> >
>
>
> --
> -John
> john.blum10101 (skype)
>

Re: [DISCUSS] Using Pull Requests over Review Board

[Jacob Barrett]

On Thu, Jun 8, 2017 at 9:28 AM John Blum  wrote:

> PRs are for contributors that do not have commit privileges.  ReviewBoard
> is a tool for "reviewing" changes.
>

This isn't some law, it was our choice. What I am proposing is that we
re-evaluate this choice for consistency.


> However, what is also common practice on open source projects, even for
> committers, is to create a topic branch containing the commit with the
> desired changes (labeled with an appropriate JIRA ticket + description).
> Then a reviewer can then pick up the topic branch, review the code changes,
> polish things up and even merge the topic branch back into the mainline
> (e.g. develop) and close the ticket.  IDEs, more than ReviewBoard/FishEye,
> etc, have better tooling for reviewing diffs, making change, running tests,
> etc.
>

I feel like this argument justifies the PR model because it forces the
practice of topic branches. It creates consistency across all contributes,
committer or not.

-Jake

Re: [DISCUSS] Using Pull Requests over Review Board

[John Blum]

+1 to Bruce's comments.

PRs are for contributors that do not have commit privileges.  ReviewBoard
is a tool for "reviewing" changes.

However, what is also common practice on open source projects, even for
committers, is to create a topic branch containing the commit with the
desired changes (labeled with an appropriate JIRA ticket + description).
Then a reviewer can then pick up the topic branch, review the code changes,
polish things up and even merge the topic branch back into the mainline
(e.g. develop) and close the ticket.  IDEs, more than ReviewBoard/FishEye,
etc, have better tooling for reviewing diffs, making change, running tests,
etc.

-John



On Thu, Jun 8, 2017 at 8:32 AM, Bruce Schuchardt 
wrote:

> One thing I find confusing in PRs is whether the person submitting the
> request is a committer or not.  Non-committers need someone to merge the PR
> while committers are just looking for a review and will merge the changes
> to develop themselves.
>
> I also don't see any way to push a PR to specific individuals for review.
> In Reviewboard there is a nice queue of pending reviews that I can go
> through.  On github they're all mixed together and it's difficult to tell
> whether any of them are relevant to me.
>
> I like the idea of a single source of history for reviews but I don't much
> like the idea of having to create PRs on a read-only system and then merge
> my changes to ASF's repo.  Being able to commit directly seems like a
> committer perk that your idea would take away from us.
>
>
> On 6/7/17 6:42 PM, Jacob Barrett wrote:
>
>> All,
>>
>> I would like to discuss transitioning all code reviews to pull requests
>> over using review board. For non-committer community members we ask them
>> to
>> make pull requests against the mirrored GitHub repo. Some committers use
>> pull requests for their own work reviews while others use review board. I
>> propose that we just use on and that the one we use be pull requests. It
>> would give us a single source of history for reviews, a single model to
>> understand for reviewers and committers and keep workflow consistent with
>> all contributors, committers or not.
>>
>> -Jake
>>
>>
>


-- 
-John
john.blum10101 (skype)

Re: [DISCUSS] Using Pull Requests over Review Board

[Bruce Schuchardt]

One thing I find confusing in PRs is whether the person submitting the 
request is a committer or not.  Non-committers need someone to merge the 
PR while committers are just looking for a review and will merge the 
changes to develop themselves.


I also don't see any way to push a PR to specific individuals for 
review.  In Reviewboard there is a nice queue of pending reviews that I 
can go through.  On github they're all mixed together and it's difficult 
to tell whether any of them are relevant to me.


I like the idea of a single source of history for reviews but I don't 
much like the idea of having to create PRs on a read-only system and 
then merge my changes to ASF's repo.  Being able to commit directly 
seems like a committer perk that your idea would take away from us.



On 6/7/17 6:42 PM, Jacob Barrett wrote:

All,

I would like to discuss transitioning all code reviews to pull requests
over using review board. For non-committer community members we ask them to
make pull requests against the mirrored GitHub repo. Some committers use
pull requests for their own work reviews while others use review board. I
propose that we just use on and that the one we use be pull requests. It
would give us a single source of history for reviews, a single model to
understand for reviewers and committers and keep workflow consistent with
all contributors, committers or not.

-Jake