Pull request all the things!

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Pull request all the things!

jewzaam
Administrator
We've been using pull requests for some things for a little bit right now and I think in general it's working well.  I would like to see us start doing pull requests for all changes.  The main reasons are we have now cut our first release of some components (V0.9.0) and it makes review and changes in a review much cleaner.

For the release rational, it's much easier to manage cutting the next release if there are not changes being pushed directly to master as the release happens.  This hit me briefly yesterday when I cut 0.9.0 of lightblue-mongo.  It wasn't a big deal, but the changes didn't really need to be part of the release.  It would be nice to have a gate to manage that.

Each PR is build automatically by travis-ci and we get nice info on that on the github PR.

If you develop like me you end up with lots and lots of little commits.  This makes it hard for others to review changes.  Putting it into a PR (and therefore a separate branch) makes it very easy to review.  And if there is a problem the fixes can be pushed to the branch for the PR and automatically roll into a build with travis-ci.

While I think we'll have exceptions to any rule, in general it's a good idea to do a PR for everything we can.  I have updated waffle.io columns to put new PR from collaborators into "In Progress" instead of "Ready For Review".  This will allow us to start some new functionality and develop in that feature branch and have it tracked by lightblue.  When the developer thinks it is ready for review, simply move it to that column.  This specifically is for cases when we start some change that does not have an associated issue.  So instead of creating a new issue AND a PR, just create the PR and make the changes.

Having a PR and ensuring that each and every PR is reviewed by someone different than the owner (aka developer) of the PR means everything has extra eyes on it.  We've looked at many tools out there to facilitate this, but nothing has really fit.  Doing these PR's will make the review possible while also keeping the master branch clean.

For non-collaborators should we move it to "In Progress" as well?  Or is "Ready" still appropriate?

This is a discussion, not a mandate.. And it will never be set in stone!  If things need to change, they should change!
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
I dont know , like maybe we will need another approach in case the majority of non-collaborators works in certain way (for example, maybe they just see the issue, drop a comment on nabble and then send a PR, so it would need our help to move the issue in waffle/github, just a single possible scenario).

Well, if everything works well, I would like the process be the same for non-collaborators and collaborators (so there would be more simple (less error prone) and people might feel close to the rest to the team and could help more ). But that is just my 2 cents =) I think it can be in a different way and maybe we can even change after a while, the point is to make sth productivity for us and the community (I think) =)
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
+1 to simplicity and consistency

Should the non-collaborators (simply meaning not a writer to the repo) have PR land in "Ready For Review" then?  I think typically those PR's will be at a done state, ready to be picked up.

And what is the restriction for folks to move cards around?  Do you have to have write to the repo?  Guessing so to add labels given my issues in other repos haven't allowed assigning tags.  So, if that's the case, managing the PR state (column on waffle.io) is not a concern for those PR's.  But we should treat them the same.  Except the trigger for reviewing has to be managed by a collaborator.

Any way we can simply add more people as collaborators?  What would we want to have more strict control on for now?  Pushes to master is all I can think of, maybe tag and branch creation too.


On Tue, Aug 19, 2014 at 11:17 AM, lcestari [via lightblue-dev] <[hidden email]> wrote:
I dont know , like maybe we will need another approach in case the majority of non-collaborators works in certain way (for example, maybe they just see the issue, drop a comment on nabble and then send a PR, so it would need our help to move the issue in waffle/github, just a single possible scenario).

Well, if everything works well, I would like the process be the same for non-collaborators and collaborators (so there would be more simple (less error prone) and people might feel close to the rest to the team and could help more ). But that is just my 2 cents =) I think it can be in a different way and maybe we can even change after a while, the point is to make sth productivity for us and the community (I think) =)


If you reply to this email, your message will be added to the discussion below:
http://lightblue-dev.1011138.n3.nabble.com/Pull-request-all-the-things-tp59p61.html
To start a new topic under lightblue-dev, email [hidden email]
To unsubscribe from lightblue-dev, click here.
NAML

Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
In reply to this post by jewzaam
An example of exceptions can be seen with recent updates to travis configs to setup deploying artifacts to sonatype snapshot repos.  It's also an example of something that really can only be verified by doing it, so a PR isn't practical in my opinion.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
I agree =)
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
One annoying thing when doing a PR for an issue is the issue hangs around until auto-closed by merging the PR.  Is there an alternative?  Should we move the issue forward to "ready for review" with the PR?  I don't think closing the issue is right.  If the PR is rejected then we would loose the need to fix the issue still.  But it makes it look like much more is in progress.  Maybe a new column?
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

skavanagh
In reply to this post by jewzaam
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
Lots of commits get rolled up nicely in a PR, basically I was saying it's a reason to do a PR.  Then you don't have to squash.  A few points:
* a PR allows you to keep all the history
* a PR requires care to not put random changes into the request
* github reports on a PR that it cannot be cleanly merged

I saw a lightning talk recently about an IRC bot someone developed against trello that did things like nagged you if a card wasn't getting attention.  I'd be stunned if something similar didn't exist for github, but I haven't looked.  I think the problem we have right now is reminders that we have things that need attention.  We have the freenode channel that could be used.  The travis-ci bot already spams us on there quite nicely.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
I didnt find an bot that does that. I think we could make it (a very simple version that scrape github page periodically)
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
Maybe there's another notification platform other than IRC that does have this type of feature.  I know there are lots of things out there, but I know nothing about them.
I put a request into waffle.io to add this feature directly to notify when an issue has been sitting in a column too long.  One question from them is if we use only IRC or other things.  They said they get requests for things like this for Slack and Flowdock.


On Fri, Aug 22, 2014 at 11:11 AM, lcestari [via lightblue-dev] <[hidden email]> wrote:
I didnt find an bot that does that. I think we could make it (a very simple version that scrape github page periodically)


If you reply to this email, your message will be added to the discussion below:
http://lightblue-dev.1011138.n3.nabble.com/Pull-request-all-the-things-tp59p101.html
To start a new topic under lightblue-dev, email [hidden email]
To unsubscribe from lightblue-dev, click here.
NAML

Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
These are some of new tools people are using nowadays. I know some companies using Slack and saying it a very nice tools, it seems to have integration with many other things like github,dropbox,heroku, etc.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
Luan, have you looked at any in any capacity?  Even over someone's shoulder is more than I've gotten.


On Fri, Aug 22, 2014 at 11:57 AM, lcestari [via lightblue-dev] <[hidden email]> wrote:
These are some of new tools people are using nowadays. I know some companies using Slack and saying it a very nice tools, it seems to have integration with many other things like github,dropbox,heroku, etc.


If you reply to this email, your message will be added to the discussion below:
http://lightblue-dev.1011138.n3.nabble.com/Pull-request-all-the-things-tp59p107.html
To start a new topic under lightblue-dev, email [hidden email]
To unsubscribe from lightblue-dev, click here.
NAML

Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
AFAIK, Slack is some kind of instant message system, which have both web and desktop clients. You can create a free account and create private chat rooms which you have to invite other people to join. In those rooms you can send images, snippets, files and etc. The code send will be auto-highlighted. This seems a good tools to manage internal projects. I'm not familiar with the other integration that Slack have, but I would guess it is similar to a bot on IRC which you would send a command that could open an issue on github repository, for example.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
In reply to this post by jewzaam
A note on feature development.  The default behavior for a PR is to go against the master branch, but it's possible to set it up to hit any branch.  Just talking to Burak on IRC about this and will be how we can do the association feature development.

https://help.github.com/articles/using-pull-requests#changing-the-branch-range-and-destination-repository
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
In reply to this post by jewzaam
After doing this with collaborator PR's landing in "In Progress" instead of "Ready for Review" I feel this is pretty counter productive.. Most of the time I issue a PR it's ready to be reviewed.

Anybody feel strongly either way?  I'm thinking maybe switching it back so a new PR is automatically ready for review.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
Yeah, I think PR should be always on Ready for Review, I will open a new thread to discuss this more.

About the other subject you talked earlier, there is another messaging web system some projects are adopting to use called gitter. There are some notification about the project and it can inport public repositories , like https://gitter.im/lightblue-platform
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator
I'm changing the default for collaborator PR's to land in "Ready for Review" based on this thread + comments and just realizing that I react to a PR as though it's ready to get merged.  I think this is the intent of all new PR's.
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
Cool. By the way, so documentation will not need PR, right?
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

jewzaam
Administrator

I think not, unless documentation is likely to need review before inclusion. Maybe if we use the doc to discuss process changes? Dunno

On Oct 3, 2014 9:41 AM, "lcestari [via lightblue-dev]" <[hidden email]> wrote:
Cool. By the way, so documentation will not need PR, right?


If you reply to this email, your message will be added to the discussion below:
http://dev.forum.lightblue.io/Pull-request-all-the-things-tp59p168.html
To start a new topic under lightblue-dev, email [hidden email]
To unsubscribe from lightblue-dev, click here.
NAML
Reply | Threaded
Open this post in threaded view
|

Re: Pull request all the things!

lcestari
OK. For me that sounds good. The only thing that I concern is we could just review the whole documentation once every major change (or minor changes that change a lot of things) to update the information there
12