Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of Pull Requests #47

Open
zkessin opened this issue Feb 1, 2015 · 40 comments
Open

Handling of Pull Requests #47

zkessin opened this issue Feb 1, 2015 · 40 comments

Comments

@zkessin
Copy link
Contributor

zkessin commented Feb 1, 2015

This project has a few Pull Reqs that are quite old. I would like to suggest a rule that Pull Reqs should be dealt with quickly, be they accepted or rejected, but they should not be left to hang around.

Comments please

@MirkoBonadei
Copy link
Contributor

Hi @zkessin, I agree with your suggestion.
I am interested in this project even if I am out of the game in this period. I think that it is not easy for @krestenkrab to approve pull requests right away because this project is not in his priorities and mind context switch among projects is not easy at all.
@krestenkrab what can we do to help you in the approval phase?

@zkessin
Copy link
Contributor Author

zkessin commented Feb 2, 2015

@krestenkrab would you be interested in having a few of us help?
the zeromq community has a good set of rules that can be adopted for how to grow triq http://rfc.zeromq.org/spec:22 (though I think leaving the license as is would be good)

@ghost
Copy link

ghost commented Feb 3, 2015

Reasonable suggestion if a proper dev process (review and branching model) is used, in order to avoid a chaotic or unstable master branch.

@zkessin
Copy link
Contributor Author

zkessin commented Feb 4, 2015

take a look at what I posted for a model, the 0MQ folks have done very well with that model.

@essen
Copy link
Contributor

essen commented Feb 4, 2015

That's a very heavy model for fixing responsiveness issues. Also I think someone summed it up as "merge first, ask questions later"? Probably referring to Maintainers SHALL NOT make value judgments on correct patches.

A much better model would be the original git model. Kresten could get one or more trusted contributor to handle the PRs, merge them in their own fork and then submit the update to this very repository. At this point Kresten just has to take a quick glance and click merge in the large majority of the cases.

@hintjens
Copy link

hintjens commented Feb 4, 2015

Changing culture can be heavy, yes. However we've found that merging rapidly gives new contributors the incentive to learn and stay. It's worked very well in dozens of projects now. The only drawback seems to be some vulnerability to trolls, though this is easily managed, it turns out.

If you're seeing PRs sitting unmerged, due to lack of time to review them, I'd definitely check out C4, and ask the zeromq-dev list (mostly users of ZeroMQ, so critical of any faults in the process) how they like the process.

@zkessin
Copy link
Contributor Author

zkessin commented Feb 5, 2015

The real issue is that I would like to add some enhancements to Triq, but am not going to do so unless I have some confidence that Pull Reqs will actually get merged

@essen
Copy link
Contributor

essen commented Mar 9, 2015

Hello @zkessin, I saw your invite and am wondering if you could explain a little more about it, like whether you talked about it with Kresten and what are your plans for it? Thanks.

@krestenkrab
Copy link
Owner

Folks, I'm somewhat hung up .. so if one of you cool erlangers want to co-administer this you're most welcome.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

I want to manage the open-triq branch under the zeromq rules, I have invited @essen @krestenkrab @MirkoBonadei and a few others to the project as committers, (and will invite anyone else after a few merged Pull Reqs)

I hope this way Triq can move forward and @krestenkrab does not need to be involved if he does not have time

@krestenkrab
Copy link
Owner

I hope we can keep triq under Apache license.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

It will stay under the same licence

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

Readme changed to make the licence clear

@ghost
Copy link

ghost commented Mar 9, 2015

Given the size of Triq, a dual branch model is sufficient to allow:

  • quick merges of good patches
  • keeping a stable master
  • merging to master on schedule, assuming it's in a regression-free state

Also due to Triq's size there's no need for collecting topic branches into a next branch and merge only stable topics to master.

So, if we change the project settings to merge pull requests to a staging branch (with immutable history), we can have co-administered merges of patches.

Kresten has offered to assign permissions as required, so I don't see a need for a separate project.

@hintjens
Copy link

hintjens commented Mar 9, 2015

The use of a single master branch for all work is aggressive, yet it has
proven successful in multiple projects now, of all sizes and maturities.
Merging to master forces errors into public view sooner, and puts the
economic onus on contributors, rather than maintainers. It has reduced the
error count in our projects dramatically, and made it much easier for new
participants to join.

On Mon, Mar 9, 2015 at 4:56 PM, Tuncer Ayaz notifications@github.com
wrote:

Given the size of Triq, a dual branch model is sufficient to allow:

  • quick merges of good patches
  • keeping a stable master
  • merging to master on schedule, assuming it's in a regression-free
    state

Also due to Triq's size there's no need for collecting topic branches into
next and merge only stable topics.

So, if we change the project settings to merge pull requests to a staging
branch (with immutable history), we can have co-administered merges of
patches.

Kresten has offered to assign permissions as required, so I don't see a
need for a separate project.


Reply to this email directly or view it on GitHub
#47 (comment).

@essen
Copy link
Contributor

essen commented Mar 9, 2015

I would agree with that. I do not see much point in a branch or even less a fork. This project just needs someone with time to review and merge changes.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

Well ideally what it needs is a few people who can collectively deal with pull requests. I don't want a bunch of pull requests to pile up because one person goes on holiday or gets swamped at work etc

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

Just wait for Travis - CI to run the tests before you merge a pull request

@hintjens
Copy link

hintjens commented Mar 9, 2015

We had a few significant breakthroughs in the ZeroMQ process. One was to
split off reviewing PRs from merging them. We simply do not do blocking
reviews. This sounds insane yet produces better code. To merge a PR takes
just a quick sanity check, and even that is redundant. If someone breaks
master, that is significant and useful data. And fixing the break is good
learning, either for the contributor, or for others.

This only works if you (a) use a single branch, (b) promote contributors
aggressively to maintainer and (c) have tools for regulating bad actors
(thus a well written contract of sorts).

On Mon, Mar 9, 2015 at 5:19 PM, Zachary Kessin notifications@github.com
wrote:

Well ideally what it needs is a few people who can collectively deal with
pull requests. I don't want a bunch of pull requests to pile up because one
person goes on holiday or gets swamped at work etc


Reply to this email directly or view it on GitHub
#47 (comment).

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

I also like the idea of having it under an organizational ownership, not @krestenkrab 's personal account

@essen
Copy link
Contributor

essen commented Mar 9, 2015

This project barely has any tests, relying on Travis or similar is suicidal at best.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

well we can add more, but it might as well pass the tests that it has

@ghost
Copy link

ghost commented Mar 9, 2015

@hintjens, how do you avoid introduction and subsequent fixing of bad APIs, which means more maintenance overhead due to extra deprecations and compat code, without code reviews?

@hintjens
Copy link

hintjens commented Mar 9, 2015

With libzmq we also had few test cases. We enabled Travis, and said,
"breaking existing test cases is a reason to revert (or not merge) a
patch", and with this incentive, new contributors added a lot of test
cases. This was very positive.

However I don't usually wait for Travis before merging. Like I said,
breaking master is good data.

On Mon, Mar 9, 2015 at 5:28 PM, Zachary Kessin notifications@github.com
wrote:

well we can add more, but it might as well pass the tests that it has


Reply to this email directly or view it on GitHub
#47 (comment).

@hintjens
Copy link

hintjens commented Mar 9, 2015

@Tuncer here's my latest proposal for that: http://hintjens.com/blog:85

Basically you allow APIs to evolve naturally, and you tag them RAW, DRAFT, STABLE, LEGACY, RETIRED depending on use. This gives you a tool for fixing design errors (during RAW and DRAFT), while not breaking user space (STABLE onwards).

The underlying change theory is, deliver code rapidly to real people, and allow them to express irritation in the form of pull requests rather than issues. Done right, you get a smooth and fast cycle of change that iterates through refinements until APIs and internals are good, and stable, and the result sits for a year or two before being superseded by a better abstraction. You then make a new draft, and start to deprecate the old APIs (e.g. remove from documentation). The library can support them for long, the cost is usually low.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

anyone know how to get travis-ci to work with a new organization? It does not turn up in my travis-ci screen

@hintjens
Copy link

hintjens commented Mar 9, 2015

You need to sign into Travis with the credentials of that organization, I
think.

On Mon, Mar 9, 2015 at 5:46 PM, Zachary Kessin notifications@github.com
wrote:

anyone know how to get travis-ci to work with a new organization? It does
not turn up in my travis-ci screen


Reply to this email directly or view it on GitHub
#47 (comment).

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

Ok, so who here likes the idea of using the ZeroMQ Model, please leave a +1 comment so we can get a sense.

Obviously if there are any questions then ask them

@ghost
Copy link

ghost commented Mar 9, 2015

@hintjens, how do you convey what stage an Erlang function is at to a random consumer of the API?

@essen
Copy link
Contributor

essen commented Mar 9, 2015

I would guess as part of the documentation.

Anyway I'm not too interested in a process that breaks master all the time, I like me git bisect.

@ghost
Copy link

ghost commented Mar 9, 2015

@zkessin, I can understand why a separate org is preferred, but:

  • the org name reads as if krestenkrab/triq is somehow "closed"
  • the org name is too long for my taste

As krestenkrab/triq is under a personal (not company) profile, I don't mind keeping the canonical url unchanged, but I'm okay with a separate org that has a better name :). It has to be worth it, though, as it will take a very long time for users to realize the new url, to mention one problem.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

the problem was that the name triq was not available, if you have a better name for the org i would be happy to take suggestions

@ghost
Copy link

ghost commented Mar 9, 2015

I totally understand the benefits of the proposed model and can relate to the issues the other models impose on everybody, but merging everything that doesn't look totally broken to master doesn't appeal to me in the proposed form. First of all, you need enough qualified contributors on call to unbreak the branch in a matter of hours max. That means, a project short of contributors suddenly is in need of more contributors. Second of all, doing no review of interface changes means more often than not the maintainers will have to do the cleanup or refactoring rather than the patch author, who may never reappear, and the maintainers may have to guess and fill in the blanks, or not have access to certain operating environments. Finally, in the merge-quickly scenario it becomes more important that CI includes the project being tested against as many existing users (projects) of it as possible, which usually requires a nightly run.

So, I think there has to be a healthy middle-ground between merge-anything and pending-for-review-since-last-year.

@ghost
Copy link

ghost commented Mar 9, 2015

@zkessin if a separate org is the way to go, I'm sure we'll find a better name. Let's ignore the name for now.

@zkessin
Copy link
Contributor Author

zkessin commented Mar 9, 2015

ok, we will leave it as @open-triq if we think of a better one later we can change it

@hintjens
Copy link

hintjens commented Mar 9, 2015

I'll throw in a few more comments:

  • the process is meant to, and does, increase the number of contributors
  • it reduces the workload on existing maintainers (really, that's why I made it, as maintainer)
  • patches are reduced in size (1 problem, 1 change) so are easy to correct or revert
  • it's merge-anything yet combined with significant improvement on change size and focus
  • you can use that latter part alone, with some, not all, the benefits
  • using an org lets you apply fork/merge systematically, no special rights for anyone

Anyhow, this ain't a sale pitch, just material for thought. :)

@jtmoulia
Copy link

Hey all, is the issues tracker meant to be closed for open-triq/triq?

@zkessin
Copy link
Contributor Author

zkessin commented Apr 19, 2015

No, I will fix that

@ghost
Copy link

ghost commented Apr 19, 2015

@krestenkrab, what's the plan?

@ghost
Copy link

ghost commented Aug 22, 2015

@krestenkrab ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants