You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Jaikiran Pai <ja...@gmail.com> on 2017/06/19 03:14:24 UTC
Process for handling GitHub PRs and closing them
We have (read only) github repos which back our main ASF git repos (consider the github ant-ivy repo which is a read-only mirror of ASF git repo). Users submit pull requests to our github repos and the process I follow for merging such PRs is the “rebase” approach which looks something like this:
- Fetch the PR locally (git fetch github pull/45/head:pr-45)
- Checkout to that branch locally (git checkout pr-45)
- Rebase that PR on top of latest ASF (upstream) repo (git rebase asf/master)
- Run a short build, verify and push to ASF repo (git push asf pr-45:master)
(assume 45 is the pull request id).
So essentially, I rebase the commits from the PR on top of the latest master and then push to the ASF repos. All this works fine and the ASF repos get those changes. However, this doesn’t “close” the pull request on github.
Apparently, the way to have the pull request closed is doing a actual “merge” of the pull request commits into the ASF repo instead of rebasing the commits.
Then the other approach, which isn’t that clean IMO, is to push a commit to the ASF repo with a commit message which includes “This closes #X” where X is the pull request id. The ASF github bot then notices this commit messages and goes and closes the open PR.
I usually prefer the rebase approach (the one I outlined above) for dealing with pull requests, since it gives a clean git commit tree. But clearly that doesn’t have a way to close the PR.
Is there any preferred approach that we should follow with PRs?
The other thing I had in mind, if we agree upon, is to have an enhancement raised with the ASF infra team to allow adding some specific comment on the open PR by a *committer* which then auto-closes the PR. Some comment like “This PR is merged”.
Any thoughts?
-Jaikiran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Process for handling GitHub PRs and closing them
Posted by Stefan Bodewig <bo...@apache.org>.
On 2017-06-19, Jaikiran Pai wrote:
> We have (read only) github repos which back our main ASF git repos
> (consider the github ant-ivy repo which is a read-only mirror of ASF
> git repo). Users submit pull requests to our github repos and the
> process I follow for merging such PRs is the “rebase” approach which
> looks something like this:
> - Fetch the PR locally (git fetch github pull/45/head:pr-45)
> - Checkout to that branch locally (git checkout pr-45)
> - Rebase that PR on top of latest ASF (upstream) repo (git rebase asf/master)
> - Run a short build, verify and push to ASF repo (git push asf pr-45:master)
I tend to download 45.patch and "git am" it, but that's almost the same
as your first two steps. Most of the time I don't need to rebase. OTOH I
haven't got any problem with merge commits for PRs :-)
> Apparently, the way to have the pull request closed is doing a actual
> “merge” of the pull request commits into the ASF repo instead of
> rebasing the commits.
I'm not sure whether merging actually closes the PR, I've almost always
been forced to use a "closes #45" commit.
Most of the time there is some additional commit you have to perform,
like keeping track of the change inside the changelog. I use this extra
commit to close the PR. Not sure whether this is a best practice or even
a good practice.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Process for handling GitHub PRs and closing them
Posted by Matt Sicker <bo...@gmail.com>.
What I've done on other Apache projects is essentially "git pull
github/pull/N/head", merging directly into master (this is after it's ready
to merge of course). After pushing that, GitHub will see that the branch is
merged and auto-closes the PR. If you rewrite history as with the rebase
example, GitHub (and git in general) doesn't notice that it's merged, so it
doesn't auto-close. If you really want to rebase, there's no way around
adding an additional commit to add the magic "closes #N" message.
On 19 June 2017 at 09:48, Jaikiran Pai <ja...@gmail.com> wrote:
>
> On 19-Jun-2017, at 2:43 PM, Nicolas Lalevée <ni...@hibnet.org>
> wrote:
>
> >
> >> Le 19 juin 2017 à 05:14, Jaikiran Pai <ja...@gmail.com> a
> écrit :
> >>
> >> We have (read only) github repos which back our main ASF git repos
> (consider the github ant-ivy repo which is a read-only mirror of ASF git
> repo). Users submit pull requests to our github repos and the process I
> follow for merging such PRs is the “rebase” approach which looks something
> like this:
> >>
> >>
> >> - Fetch the PR locally (git fetch github pull/45/head:pr-45)
> >> - Checkout to that branch locally (git checkout pr-45)
> >> - Rebase that PR on top of latest ASF (upstream) repo (git rebase
> asf/master)
> >> - Run a short build, verify and push to ASF repo (git push asf
> pr-45:master)
> >>
> >> (assume 45 is the pull request id).
> >>
> >> So essentially, I rebase the commits from the PR on top of the latest
> master and then push to the ASF repos. All this works fine and the ASF
> repos get those changes. However, this doesn’t “close” the pull request on
> github.
> >>
> >> Apparently, the way to have the pull request closed is doing a actual
> “merge” of the pull request commits into the ASF repo instead of rebasing
> the commits.
> >>
> >> Then the other approach, which isn’t that clean IMO, is to push a
> commit to the ASF repo with a commit message which includes “This closes
> #X” where X is the pull request id. The ASF github bot then notices this
> commit messages and goes and closes the open PR.
> >>
> >> I usually prefer the rebase approach (the one I outlined above) for
> dealing with pull requests, since it gives a clean git commit tree. But
> clearly that doesn’t have a way to close the PR.
> >>
> >> Is there any preferred approach that we should follow with PRs?
> >
> > I agree with the approach you have. I have been lazy though, I just use
> the command line suggested in the email notifications, which makes things
> quite straight forward.
> >
> > I would just insist on trying to get the PR closed. Even if it may
> pollute the commit log, we should put the "This closes #X" message. And it
> can be viewed as a marker, just like we do with Jira. Also, we could make
> thing more explicit by specifying the full path of the github project: «
> closes apache/ant-ivy#123 ».
> > See the github documentation about it: https://help.github.com/
> articles/closing-issues-via-commit-messages/ <https://help.github.com/
> articles/closing-issues-via-commit-messages/>
> >
>
>
> I wasn’t aware that this kind of commit message comes from and is
> supported by github itself. I was under the impression that this is
> something that the ASF github integration had introduced. Thanks for that
> link.
>
> What you suggest about making sure that the PR gets closed even if we end
> up with those mark commit messages, sounds fine to me and I’ll start doing
> that.
>
> -Jaikiran
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>
--
Matt Sicker <bo...@gmail.com>
Re: Process for handling GitHub PRs and closing them
Posted by Jaikiran Pai <ja...@gmail.com>.
On 19-Jun-2017, at 2:43 PM, Nicolas Lalevée <ni...@hibnet.org> wrote:
>
>> Le 19 juin 2017 à 05:14, Jaikiran Pai <ja...@gmail.com> a écrit :
>>
>> We have (read only) github repos which back our main ASF git repos (consider the github ant-ivy repo which is a read-only mirror of ASF git repo). Users submit pull requests to our github repos and the process I follow for merging such PRs is the “rebase” approach which looks something like this:
>>
>>
>> - Fetch the PR locally (git fetch github pull/45/head:pr-45)
>> - Checkout to that branch locally (git checkout pr-45)
>> - Rebase that PR on top of latest ASF (upstream) repo (git rebase asf/master)
>> - Run a short build, verify and push to ASF repo (git push asf pr-45:master)
>>
>> (assume 45 is the pull request id).
>>
>> So essentially, I rebase the commits from the PR on top of the latest master and then push to the ASF repos. All this works fine and the ASF repos get those changes. However, this doesn’t “close” the pull request on github.
>>
>> Apparently, the way to have the pull request closed is doing a actual “merge” of the pull request commits into the ASF repo instead of rebasing the commits.
>>
>> Then the other approach, which isn’t that clean IMO, is to push a commit to the ASF repo with a commit message which includes “This closes #X” where X is the pull request id. The ASF github bot then notices this commit messages and goes and closes the open PR.
>>
>> I usually prefer the rebase approach (the one I outlined above) for dealing with pull requests, since it gives a clean git commit tree. But clearly that doesn’t have a way to close the PR.
>>
>> Is there any preferred approach that we should follow with PRs?
>
> I agree with the approach you have. I have been lazy though, I just use the command line suggested in the email notifications, which makes things quite straight forward.
>
> I would just insist on trying to get the PR closed. Even if it may pollute the commit log, we should put the "This closes #X" message. And it can be viewed as a marker, just like we do with Jira. Also, we could make thing more explicit by specifying the full path of the github project: « closes apache/ant-ivy#123 ».
> See the github documentation about it: https://help.github.com/articles/closing-issues-via-commit-messages/ <https://help.github.com/articles/closing-issues-via-commit-messages/>
>
I wasn’t aware that this kind of commit message comes from and is supported by github itself. I was under the impression that this is something that the ASF github integration had introduced. Thanks for that link.
What you suggest about making sure that the PR gets closed even if we end up with those mark commit messages, sounds fine to me and I’ll start doing that.
-Jaikiran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Process for handling GitHub PRs and closing them
Posted by Nicolas Lalevée <ni...@hibnet.org>.
> Le 19 juin 2017 à 05:14, Jaikiran Pai <ja...@gmail.com> a écrit :
>
> We have (read only) github repos which back our main ASF git repos (consider the github ant-ivy repo which is a read-only mirror of ASF git repo). Users submit pull requests to our github repos and the process I follow for merging such PRs is the “rebase” approach which looks something like this:
>
>
> - Fetch the PR locally (git fetch github pull/45/head:pr-45)
> - Checkout to that branch locally (git checkout pr-45)
> - Rebase that PR on top of latest ASF (upstream) repo (git rebase asf/master)
> - Run a short build, verify and push to ASF repo (git push asf pr-45:master)
>
> (assume 45 is the pull request id).
>
> So essentially, I rebase the commits from the PR on top of the latest master and then push to the ASF repos. All this works fine and the ASF repos get those changes. However, this doesn’t “close” the pull request on github.
>
> Apparently, the way to have the pull request closed is doing a actual “merge” of the pull request commits into the ASF repo instead of rebasing the commits.
>
> Then the other approach, which isn’t that clean IMO, is to push a commit to the ASF repo with a commit message which includes “This closes #X” where X is the pull request id. The ASF github bot then notices this commit messages and goes and closes the open PR.
>
> I usually prefer the rebase approach (the one I outlined above) for dealing with pull requests, since it gives a clean git commit tree. But clearly that doesn’t have a way to close the PR.
>
> Is there any preferred approach that we should follow with PRs?
I agree with the approach you have. I have been lazy though, I just use the command line suggested in the email notifications, which makes things quite straight forward.
I would just insist on trying to get the PR closed. Even if it may pollute the commit log, we should put the "This closes #X" message. And it can be viewed as a marker, just like we do with Jira. Also, we could make thing more explicit by specifying the full path of the github project: « closes apache/ant-ivy#123 ».
See the github documentation about it: https://help.github.com/articles/closing-issues-via-commit-messages/ <https://help.github.com/articles/closing-issues-via-commit-messages/>
> The other thing I had in mind, if we agree upon, is to have an enhancement raised with the ASF infra team to allow adding some specific comment on the open PR by a *committer* which then auto-closes the PR. Some comment like “This PR is merged”.
I don’t think the ASF infra team can do something about it. The « This closes #x » is something that is supported by every github repository, this is not specific to the mirrored ASF repos.
Probably a simpler and better integration with github would be to make the PR tracker editable to ASF committers. And probably both github and asf teams have heard about that.
Nicolas