triq icon indicating copy to clipboard operation
triq copied to clipboard

Handling of Pull Requests

Open zkessin opened this issue 10 years ago • 40 comments

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

zkessin avatar Feb 01 '15 08:02 zkessin

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?

MirkoBonadei avatar Feb 01 '15 16:02 MirkoBonadei

@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)

zkessin avatar Feb 02 '15 11:02 zkessin

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

ghost avatar Feb 03 '15 18:02 ghost

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

zkessin avatar Feb 04 '15 12:02 zkessin

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.

essen avatar Feb 04 '15 12:02 essen

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.

hintjens avatar Feb 04 '15 17:02 hintjens

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

zkessin avatar Feb 05 '15 16:02 zkessin

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.

essen avatar Mar 09 '15 14:03 essen

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

krestenkrab avatar Mar 09 '15 15:03 krestenkrab

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

zkessin avatar Mar 09 '15 15:03 zkessin

I hope we can keep triq under Apache license.

krestenkrab avatar Mar 09 '15 15:03 krestenkrab

It will stay under the same licence

zkessin avatar Mar 09 '15 15:03 zkessin

Readme changed to make the licence clear

zkessin avatar Mar 09 '15 15:03 zkessin

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.

ghost avatar Mar 09 '15 15:03 ghost

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 [email protected] 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 https://github.com/krestenkrab/triq/issues/47#issuecomment-77881331.

hintjens avatar Mar 09 '15 16:03 hintjens

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.

essen avatar Mar 09 '15 16:03 essen

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 avatar Mar 09 '15 16:03 zkessin

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

zkessin avatar Mar 09 '15 16:03 zkessin

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 [email protected] 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 https://github.com/krestenkrab/triq/issues/47#issuecomment-77886476.

hintjens avatar Mar 09 '15 16:03 hintjens

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

zkessin avatar Mar 09 '15 16:03 zkessin

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

essen avatar Mar 09 '15 16:03 essen

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

zkessin avatar Mar 09 '15 16:03 zkessin

@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?

ghost avatar Mar 09 '15 16:03 ghost

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 [email protected] 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 https://github.com/krestenkrab/triq/issues/47#issuecomment-77888631.

hintjens avatar Mar 09 '15 16:03 hintjens

@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.

hintjens avatar Mar 09 '15 16:03 hintjens

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

zkessin avatar Mar 09 '15 16:03 zkessin

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 [email protected] 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 https://github.com/krestenkrab/triq/issues/47#issuecomment-77892544.

hintjens avatar Mar 09 '15 16:03 hintjens

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

zkessin avatar Mar 09 '15 16:03 zkessin

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

ghost avatar Mar 09 '15 17:03 ghost

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.

essen avatar Mar 09 '15 17:03 essen