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

[Meta] PR sign-off policy #1994

Closed
kailuowang opened this issue Oct 26, 2017 · 14 comments
Closed

[Meta] PR sign-off policy #1994

kailuowang opened this issue Oct 26, 2017 · 14 comments
Labels

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Oct 26, 2017

Looks like we are blocked from making progress towards RC1 due to the fact that we are down to 2 active maintainers together with the 2 maintainer sign-offs policy - PRs submitted by one maintainer often have to wait for a long time for a 3rd maintainer to review and approve.

This has been an on-going issue for a while and luckily we had some awesome new maintainers and what a difference they've made. But now we are down to 2 active maintainers again.

I see three ways going forward (there could be other ones that didn't occur to me):

  1. find a third active maintainer (or "reactivate" one of our existing ones). By "active", I mean, dedicate at least 2 hours+/week for cats PR review. That is about 25 minutes/day during weekdays.
  2. The "at least 2 maintainers sign-offs" policy was installed when cats was undergoing vigorous development with more active maintainers. Now cats is more stable and mature we could change the sign-off policy so that for PRs submitted by maintainers, only one more maintainer sign-off is required for merging. We still require two maintainer sign-offs for PR merging, it's just that a maintainer authoring the PR counts as one sign-off. TBC, this loosening does NOT apply to the technical decision process. For significant technical decisions (or any significant decisions regarding the project), we still require meticulous discussions and strong consensus among the team and the community. That has been the tradition in the past and will remain so in the future. The loosening in this proposal ONLY applies to the more mechanic code review process on the implementation of the technical decisions.
  3. It is what it is, everyone chill.

Thoughts? Opinions? Suggestions?

@tpolecat
Copy link
Member

I'm fine with (2) but will also try to find time to look at PRs more often. A problem with having a bunch of committers is that it's really easy to assume someone else will do X, forAll X; especially since we're all so busy.

@iravid
Copy link
Contributor

iravid commented Oct 26, 2017

@alexandru has expressed interest in helping with maintainership of cats a while back (I cannot recall where exactly). Perhaps he could help?

(apologies @alexandru if I am misrepresenting you... :-))

@johnynek
Copy link
Contributor

As a note, I review most PRs that have green CI and I think should be merged most mornings. I do spend probably 15 minutes per day on cats (sometimes more sometimes less).

I would prefer option 1 or 3. Reducing the number of sign offs I fear will reduce the stability of the library. I think it should be pretty hard to get things merg d and make changes at this stage.

@LukaJCB
Copy link
Member

LukaJCB commented Oct 26, 2017

I think 2 is a good idea, because it'll allow us to move more quickly. Alternatively we could be less aggressive with invalidating existing reviews on every commit?

To address the point made by @tpolecat, maybe we should use the request-review-feature more extensively.

I'm going to be very busy in November with work + uni and 3 conferences so I really hope we can get RC1 out ASAP :)

@kailuowang
Copy link
Contributor Author

@johnynek a side topic, since you are here, it seems all your feedbacks on #1927 and #1837 have been addressed, do you want to give a quick sign-off and/or merge on those two or you need more time w/ them? That would unblock RC1 progress.
Sorry for distracting the conversation, I tried to reach to you through gitter but seems you don't go there often.

@rossabaker
Copy link
Member

Model (2) has treated http4s well.

I am willing to reactivate. I focused on the ecosystem while it needed more help than cats, but now the ecosystem needs a stable cats. But I am worried about missing tribal wisdom that may have emerged during my dormancy: is the guidelines document reasonably up-to-date?

I would weigh in on any ticket that requested my review, but I'm unsure who would request it: a contributor wouldn't know who to pick from a large list, and I don't know how best to express capacity to a Review Scheduler.

@kailuowang
Copy link
Contributor Author

Thank you @johnynek for quickly reviewed 3 PRs. Now we are possibly 1 PR away from RC1.

@kailuowang
Copy link
Contributor Author

kailuowang commented Oct 26, 2017

@rossabaker and @tpolecat thank you for offering more reviews. It will make a big difference.
The guidelines seems up-to-date but just as incomplete as they used to be. I think it terms of conventions in the code bases, not much changed though. If you guys want, I can send PR review requests your way from time to time. No pressure on keeping up with them though, if you don't have the time simply reassign it back to me (if I am still assignable) or just remove yourself.

@rossabaker
Copy link
Member

👍 I will happily accept review assignments, and generally try to watch this repo and cats-dev a bit closer. Thank you for your very hard work getting us this close!

@alexandru
Copy link
Member

I can accept review assignments as well.

I'm not sure if I can commit to 25 minutes per day, due to my other commitments, but if it's needed for getting to Cats 1.0, then I can help. Please make me a member of the typelevel org.

And thanks for your hard work @kailuowang and for keeping us on track, you're awesome.

@kailuowang
Copy link
Contributor Author

Thanks @alexandru for your offering and kind words. With @rossabaker and @tpolecat reviewing PRs, I think we should be good for 1.0. It would be awesome if you'd still like to help maintain Cats going forward. Let me know - I'll be more than happy to start the process (nothing required on your end though).

@ceedubs
Copy link
Contributor

ceedubs commented Dec 2, 2017

First of all, I want to really thank @kailuowang @LukaJCB and @johnynek for keeping Cats moving. You've put in hard work, and it's greatly appreciated.

I tend to agree with @johnynek's comments. I know that the slow pace can be frustrating, but I think that it's better than not getting several sets of eyes on changes. It also sounds like we have some people who are willing to step up and spend some more time on Cats.

@kailuowang
Copy link
Contributor Author

@ceedubs yeah, I agree we no longer need to change this policy. I think the issue has been mostly addressed thanks to more maintainers engaging in reviewing. We have been able to move fast since then.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 9, 2017

@kailuowang do you think that we should close this out?

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

No branches or pull requests

8 participants