New way of handling Issues and Pull Requests in GitHub

[Louis Landry]

Hi all,

So I've been brainstorming with Ian and Andrew about how to manage the pull request queue and such better.  We are trying to find ways to reduce the signal to noise ratio for maintainers, as well as do a better job of communication about intent with the pull requests.  Here's what we'd like to try:

Open/Closed

Going forwards we are going to treat open/closed a little differently than we do currently.  If a pull request is ready to be reviewed (by maintainers and non-maintainers) it will be left open.  If there are changes that need to be made, unit tests that need to be fixed, code style issues to be cleaned up, missing documentation, etc. then comments will be left and the pull request will be closed until the developer has completed the changes.  Our expectation is that once the changes have been made developers will re-open the pull request for review.

What this effectively does is ensure that at all times we know what is ready to be looked at, and what is still being worked on.  It is important to understand that just because we close it doesn't mean that it is rejected, but we are simply saying that it needs some work before it can be considered for further review.

Milestones

Because of the change in the way we treat Open/Closed state for pull requests (and for housekeeping purposes) we are also going to be utilizing the issue tracker milestones feature going forward.  If we assign a milestone for a pull request you can consider the pull request accepted for the given release (milestone) assuming that we work out all the kinks.  If there are changes we'd like made, or test/style fixes than we will assign a milestone and close the issue.  Hopefully this will help us track what is being worked on in a given release, what was done in given releases, and communicate better with all of you about if and when we want to include the code you've submitted for review.

Browsing Issues

Those of you wanting to see what is going on in a given release would be better served viewing things through the issue tracker in this way of working.  That will give you an ability to look at everything attached to a given milestone.  Those of you who want to help us review code (and please do) you can trust that everything in the open pull request queue is ready for review.  Should make it much easier to find things to look at and comment on.

Nothing is perfect, but our hope is that this will help us manage things a bit better and make it easier for all of you to understand what is going on and what is being worked on in a given release.  To those of you submitting code, hopefully it will be more clear if/when we expect to get your code merged into the codebase.

Thoughts?

- Louis

[Chad Windnagle]

I for one am very very in favor of this. I think it's an appropriate to use Github's core features to better help you as maintainers organize and keep track of things, and it helps set expectation levels for developers submitting pull requests. 

In short, bravo!

I believe the hardest communication problem that you will need to overcome is the conveying that a closed request isn't a rejected request. It will be important to state when closing a request that it is acceptable, with the exception that necessary changes are made. 

Thanks for taking a look at this. I appreciate your efforts!

-Chad

Regards,
Chad Windnagle
Fight SOPA

[Nick Savov]

+1, yes, please leave a comment letting the developer know that he or she
should re-open the pull request after the changes have been made.
Obviously, use a snippet to save yourselves time ;)

Kind regards,
Nick

> I for one am very very in favor of this. I think it's an appropriate to
> use
> Github's core features to better help you as maintainers organize and keep
> track of things, and it helps set expectation levels for developers
> submitting pull requests.
>
> In short, bravo!
>
> I believe the hardest communication problem that you will need to overcome
> is the conveying that a closed request isn't a rejected request. It will
> be
> important to state when closing a request that it is acceptable, with the
> exception that necessary changes are made.
>
> Thanks for taking a look at this. I appreciate your efforts!
>
> -Chad
>
> Regards,
> Chad Windnagle

> Fight SOPA <https://www.google.com/landing/takeaction/>

>
>
> On Mon, Oct 8, 2012 at 9:43 PM, Louis Landry <louis...@gmail.com>
> wrote:
>
>> Hi all,
>>
>> So I've been brainstorming with Ian and Andrew about how to manage the
>> pull request queue and such better. We are trying to find ways to
>> reduce
>> the signal to noise ratio for maintainers, as well as do a better job of
>> communication about intent with the pull requests. Here's what we'd
>> like
>> to try:
>>

>> *Open/Closed*

>>
>> Going forwards we are going to treat open/closed a little differently
>> than
>> we do currently. If a pull request is ready to be reviewed (by
>> maintainers
>> and non-maintainers) it will be left open. If there are changes that
>> need
>> to be made, unit tests that need to be fixed, code style issues to be
>> cleaned up, missing documentation, etc. then comments will be left and
>> the
>> pull request will be closed until the developer has completed the
>> changes.
>> Our expectation is that once the changes have been made developers will
>> re-open the pull request for review.
>>
>> What this effectively does is ensure that at all times we know what is
>> ready to be looked at, and what is still being worked on. It is
>> important
>> to understand that just because we close it doesn't mean that it is
>> rejected, but we are simply saying that it needs some work before it can
>> be
>> considered for further review.
>>

>> *Milestones*
>> *
>> *

>> Because of the change in the way we treat Open/Closed state for pull
>> requests (and for housekeeping purposes) we are also going to be
>> utilizing
>> the issue tracker milestones feature going forward. If we assign a
>> milestone for a pull request you can consider the pull request accepted
>> for
>> the given release (milestone) assuming that we work out all the kinks.
>> If
>> there are changes we'd like made, or test/style fixes than we will
>> assign a
>> milestone and close the issue. Hopefully this will help us track what
>> is
>> being worked on in a given release, what was done in given releases, and
>> communicate better with all of you about if and when we want to include
>> the
>> code you've submitted for review.
>>

>> *Browsing Issues*

[Andrew Eddie]

On 9 October 2012 11:43, Louis Landry <louis...@gmail.com> wrote:
>
> Open/Closed
>
> Going forwards we are going to treat open/closed a little differently than
> we do currently. If a pull request is ready to be reviewed (by maintainers
> and non-maintainers) it will be left open. If there are changes that need
> to be made, unit tests that need to be fixed, code style issues to be
> cleaned up, missing documentation, etc. then comments will be left and the
> pull request will be closed until the developer has completed the changes.
> Our expectation is that once the changes have been made developers will
> re-open the pull request for review.

I just realised we've actually had this arrangement in for over a year LOL :)

https://github.com/joomla/joomla-platform/wiki/Contributing-to-the-joomla-platform

That's funny.

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers

[Nick Savov]

At least you're consistent in your thinking :P

[Donald Gilbert]

Since we found out yesterday that contributers can't reopen pull requests / issues after they have been closed by a maintainer, I think we need to rethink this (again).

I seem to remember closing a pull request accidentally on a different project that I contributed to, and being able to reopen it myself. So the answer may be to request the submitter to close it themselves, and then let them make the required changes, and reopen when it is ready for review again.

[Chad Windnagle]

Louis and I talked about this problem last night. There is a potential way of getting around the problem that the maintainers are discussing. 

-Chad

Regards,
Chad Windnagle
Fight SOPA

[Andrew Eddie]

On 11 October 2012 02:00, Chad Windnagle <drmm...@gmail.com> wrote:
> Louis and I talked about this problem last night. There is a potential way
> of getting around the problem that the maintainers are discussing.

Regrettably some cats were injured in the process.

Regards,
Andrew Eddie

[Louis Landry]

So, until we figure out a better way to deal with it (and we are working on it) you can just leave a comment in the closed pull request and we will reopen it.  We've empowered Chad to help keep track of these things so you can also reference him @drmmr763 in the comment to get his attention.

Things are looking lively on in the pull request queue.  Thanks everyone!

- Louis

[Donald Gilbert]

Can someone explain the joomla-jenkins bot? It's pretty cool that it was able to re-open my pull request after making a comment, but I had no idea it was going to do that - so I mistakenly opened a second PR instead, which I ended up closing.

[Michael Babker]

This must be a new feature, first I've heard of the bot doing that.

If you look at joomla-jenkins user history, the bot has been the one merging the staging branch to master if all the tests come back without issue from the Jenkins CI server.  Aside from that, and the new feature you've stumbled on, I don't think the bot does anything else.

[Chad Windnagle]

This is a new feature that the platform team has started working on. By commenting 'reopen' or 'open' the PR is reopened for you. 

Louis might be able to give more info on how it works, but the basic reason for it is Github doesn't allow someone to reopen a PR if a maintainer closes it, so by bypassing Github and using the API to reopen it based on a comment, we save the developer from having to open a new PR, or there being a manual process (commenting "please reopen this" and waiting for a maintainer to reopen for you).

-Chad

Regards,
Chad Windnagle
Fight SOPA

[Don]

That is really awesome. My only issue was that a closed PR doesn't update the commit history when you push the requesting branch. As in I didn't see my updates tithe jtablefix branch until after the PR was reopened. But that's a github issue, not a platform issue. 

Sent from my iPhone

[Louis Landry]

So `joomla-jenkins` is simply an account we created that isn't a person, and can be used for various automated tools.  The account has push/pull access to the platform repository, and we use it for a number of things.  Ultimately I'd prefer it if we improved the `JGithub` class to be able to authenticate using OAuth, and then these types of things would be done on behalf of `joomla` the organization instead of some service account we registered.

Our main build server, `http://build.joomla.org`, is setup with `joomla-jenkins` credentials so that when a change is made to our staging branch and the subsequent build is successful the build server can push those successful changes to the master branch.  This is one of the ways we maintain high code quality in our master branch: nothing can get into the master branch unless it passes automated testing and style checking.

Additionally, a few of us are working on a serious update to the pull request tester that we've been using for a while.  You can find the old one at `http://developer.joomla.org/pulls`.  This lets us as maintainers know whether or not the pull requests sent pass unit tests and code style checks before we merge them into staging.  We'll have more on this as we get things built and deployed, but it is certainly something we are keen to improve.  One of the things we really want to be able to do is review code without having to worry about picking up on all those sorts of things.  We want to focus on reviewing architecture and ideas, not code style, etc.  This is a way to let an automated system monitor the things that can be automated.

As for the behavior you encountered, Donald, this is something new that I just setup yesterday.  You'll remember that after we announced this new way of triaging pull requests we found out that authors cannot reopen pull requests once they are closed by maintainers.  That causes trouble for the workflow we are trying to achieve, so we wanted to set something up in an automated way to fix it.  In the updated pull request tester application I mentioned above, we added a hook that listens for comments made on pull requests.  It will look to see if the author of a pull request leaves a comment containing the word `reopen` or `re-open` on a pull request that has been closed but not merged.  If it detects that event it will automatically reopen the pull request, thus putting it back in the queue for review.

This is a fairly simplistic implementation, but my hope is that it makes things easier and clearer for contributors going forward.  All this while allowing us to stick to a workflow in reviewing code that I believe will make life easier for maintainers.  With respect to the pull request not updating while it's closed, I feel that's something we'll have to deal with.  I don't think in the grand scheme of things it is a big deal, we'll see.

I hope that clears up questions you have.

- Louis

[Don]

Very much - thanks for the thorough explanation, Louis. 

I like the implementation a lot - it certainly will make things easier for those of us digging into the platform and improving the quality of the code there. 

Sent from my iPhone

[piotr_cz]

What about escaping the reopen word (like +reopen), so people don't
trigger this behavior unintentionally?

`This PR is crappy, please nobody reopen it.`

> > Fight SOPA <https://www.google.com/landing/takeaction/>

>
> > On Wed, Oct 17, 2012 at 6:34 PM, Michael Babker <michael.bab...@gmail.com>wrote:
>
> >> This must be a new feature, first I've heard of the bot doing that.
>
> >> If you look at joomla-jenkins user history, the bot has been the one
> >> merging the staging branch to master if all the tests come back without
> >> issue from the Jenkins CI server.  Aside from that, and the new feature
> >> you've stumbled on, I don't think the bot does anything else.
>

[Donald Gilbert]

That's a good idea piotr.

Also, what happens if one of my PR's is closed, and someone else goes in and leaves a comment with "reopen" in it? Does the jenkins bot have filtering in place to only reopen if it's requested by the submitter?

[Louis Landry]

From my previous email:

It will look to see if the author of a pull request leaves a comment containing the word `reopen` or `re-open` on a pull request that has been closed but not merged.  If it detects that event it will automatically reopen the pull request, thus putting it back in the queue for review.

That is exactly the behavior that has been built.  If the author of a pull request comments with "don't reopen this request" and it is opened, then the author has the ability to close it right there.  I appreciate the enthusiasm guys, but the point isn't to do natural language processing or make it perfect.  The point was to make it usable and simple.

Cheers.

- Louis

[Donald Gilbert]

So in short, we should rtfm.

I didn't catch the part about it only applying to the author; I read your post on my phone early this morning.

[piotr_cz]

What I meant is to avoid situations like one that happened to Don. He
closed the PR and unintentionally left a comment 'open' or 'reopen'.
This might be quite a common word in packages so escaping natural
language word could be helpful.

I kinda feel like trying to be too smart :)

On Oct 18, 6:21 pm, Louis Landry <louislan...@gmail.com> wrote:
> From my previous email:
>
> It will look to see if the author of a pull request leaves a comment
>
> > containing the word `reopen` or `re-open` on a pull request that has been
> > closed but not merged.  If it detects that event it will automatically
> > reopen the pull request, thus putting it back in the queue for review.
>
> That is exactly the behavior that has been built.  If the author of a pull
> request comments with "don't reopen this request" and it is opened, then
> the author has the ability to close it right there.  I appreciate the
> enthusiasm guys, but the point isn't to do natural language processing or
> make it perfect.  The point was to make it usable and simple.
>
> Cheers.
>
> - Louis
>

[Don]

That was also before I knew it was a feature.

Sent from my iPhone

[Ian]

If it ends up being an issue we can deal with it then.

Ian