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

Commit access after the first pull request #126

Closed
nathany opened this issue Mar 8, 2016 · 16 comments
Closed

Commit access after the first pull request #126

nathany opened this issue Mar 8, 2016 · 16 comments

Comments

@nathany
Copy link
Contributor

nathany commented Mar 8, 2016

Though I adopted Chris Howey's project and did some work on the API, fsnotify isn't "my project". It's a plumbing project to support other libraries that we rely on.

I've been fairly liberal with giving out commit access so far, but want to propose making it more official. That means updating the contributing document and me not being the only one to give out the commit bit. This particular policy I first saw @evanphx apply with Rubinius.

We do have all the git history distributed across multiple machines if somebody makes a mess, though that seems unlikely to happen after submitting a pull request that is merged.

What do you all think about this policy? Should there be any additional criteria? Is there a better approach?

@evanphx
Copy link
Contributor

evanphx commented Apr 7, 2016

I'm in favor, obviously. 😁

@omeid
Copy link

omeid commented Apr 7, 2016

I suggest that commits be merged by someone other than the author to allow for code review. This is specially important as a bad commit on master can break builds.

@nathany
Copy link
Contributor Author

nathany commented Apr 7, 2016

@omeid I agree. We should make that clear in the CONTRIBUTING document and also look into GitHub's branch protection for master.

@omeid
Copy link

omeid commented Apr 8, 2016

Sounds pretty reasonable things to do!

@nathany
Copy link
Contributor Author

nathany commented Apr 8, 2016

I enabled a few things:

  • The master branch is protected from force pushes.
  • Tests must pass before a branch can be merged to master.
  • Branches must be up-to-date with master before merging.

If the tests past on a branch and it is up-to-date with master, it should still be possible to push its contents to master directly. It may or may not work with the "hub apply mail" workflow I've been using.

Though I'd prefer squash merging, merge commits are also enabled for now.

@nathany
Copy link
Contributor Author

nathany commented Apr 21, 2016

This whole idea could be impacted by what we end up doing for #136.

@nathany
Copy link
Contributor Author

nathany commented Jan 8, 2018

Perhaps this policy should be documented in CONTRIBUTING.md.

@nhooyr
Copy link
Contributor

nhooyr commented Feb 23, 2018

Also, in https://github.com/fsnotify/fsnotify/blob/c2828203cd70a50dcccfb2761f8b1f8ceef9a8e9/CONTRIBUTING.md#maintainers

we should not require being able to run the test suite on all those platforms. CI should be doing that for us.

@arp242
Copy link
Member

arp242 commented Jul 30, 2022

Go to any open source project and you will invariably find a small group (or singular person) of regular contributors who fix random bugs, respond to issues, review PRs, etc. and a very long tail of people who contribute one or a few things, usually to fix some issue they were experiencing or adding some feature they wanted, and then never come back again. fsnotify is no different.

There is nothing wrong with contributing just one fix or feature: I have a long list of projects where I've done exactly that. These are often the best kind of bug reports and feature requests: "I encountered this bug and oh, here's how to fix it", "I think this feature would be nice, so I added it".

But do people really need commit access? Not so sure about that. You don't need commit access to contribute, and I don't really see the advantage of giving out commit access to one-time contributors.

The issue I have is that I do think people need to be vetted to some degree; e.g. right now it's a bit unclear to me who can and can't push to the main branch for example (I can, not sure about others). Can people make a new release with a crypto miner (maybe "releasing" a new branch they created?) Dunno. You can set up protections and such, but it seems rather complex,cumbersome, and error-prone without any real advantages. I'm not overly paranoid about this, but you never know: people's accounts can get compromised, they can have a mental breakdown (e.g. that Aaron Swartz conspiracy repo, and the "block all Russians from using it"-repo), or whatnot.

Furthermore, I'd like to be able to use @fsnotify/fsnotify to ping people for reviews and such, and pinging 36 people – some of whom probably have no interest in it – isn't too great. I pinged people here because just a "you've been removed from fsnotify" without context is probably a bit harsh.


I wrote a small script to count some contributions of the current people with commit access; quite a few people last contributed years ago, or sometimes never at all.

I went ahead and removed the commit access for anyone with an N, and retained it for people with Y, based on last activity + number of contributions. If anyone feels strongly that they should retain access then it can be restored, or if anyone wants to be removed then I can do that too (will prevent you from getting pinged on @fsnotify/fsnotify mentions).

User Last Total Last commit # commit Last comment # cmt
N @wadey None 0 None 0 None 0
N @zellyn None 0 None 0 None 0
N @adamwg None 0 None 0 None 0
N @theckman None 0 None 0 None 0
N @xenoscopic None 0 None 0 None 0
N @jessfraz None 0 None 0 None 0
N @isaacdavis None 0 None 0 None 0
N @ppknap 2015-11 13 2015-11 1 2017-03 12
N @illicitonion 2015-11 7 2015-11 1 2015-11 6
N @tiffanyfay 2016-02 1 2016-02 1 2016-02 1
N @evanphx 2016-04 5 2015-10 1 2016-04 4
N @PieterD 2016-04 69 2015-02 17 2016-04 52
N @oozie 2016-10 11 2016-10 1 2016-10 10
N @pattyshack 2016-10 19 2016-10 3 2016-10 16
N @zchee 2017-03 4 None 0 2017-03 4
N @markbates 2017-11 1 None 0 2017-11 1
N @gdey 2018-08 9 2018-08 1 2018-08 8
N @matthias-stone 2019-03 16 2018-08 1 2019-03 15
N @dylan-bourque 2019-10 17 None 0 2019-10 17
N @drewwells 2020-07 2 None 0 2020-07 2
N @ppreeper 2022-01 1 None 0 2022-01 1
N @marklr 2022-01 4 None 0 2022-01 4
N @Techassi 2022-01 3 None 0 2022-01 3
N @sagikazarmark 2022-02 5 None 0 2022-02 5
N @pbnjay 2022-03 2 None 0 2022-03 2
Y @cpuguy83 2020-06 30 2019-03 3 2020-06 27
Y @Code0x58 2021-08 22 2021-08 4 2021-08 18
Y @bep 2021-08 37 2016-08 5 2021-08 32
Y @r-darwish 2022-01 8 2022-01 1 2022-01 7
Y @nshalman 2022-03 59 2022-01 7 2022-03 52
Y @mattn 2022-06 26 2022-04 1 2022-06 25
Y @shogo82148 2022-07 26 2022-07 5 2022-07 21
Y @kolyshkin 2022-07 28 2022-07 1 2022-02 27
Y @nathany 2022-07 812 2022-07 164 2022-07 648
Y @arp242 2022-07 99 2022-07 12 2022-07 87
Y @horahoradev 2022-07 21 2022-07 1 2022-07 20

@arp242 arp242 closed this as completed Jul 30, 2022
@nathany
Copy link
Contributor Author

nathany commented Jul 30, 2022

That makes sense.

I went ahead and removed the commit access for anyone with an 🗶, and retained it for people with 🗸, based on last activity + number of contributions.

The icons didn't come through clearly.

@arp242
Copy link
Member

arp242 commented Jul 30, 2022

The icons didn't come through clearly.

How boring; I changed it to "N" and "Y". That should work for everyone.

@zchee
Copy link

zchee commented Jul 30, 2022

sorry for I can't helped 😢

@arp242
Copy link
Member

arp242 commented Jul 30, 2022

sorry for I can't helped

No need to apologise @zchee; everyone doesn't contribute to projects they use that need help (it would be impossible to do so, as there are too many!)

@zchee
Copy link

zchee commented Jul 30, 2022

@arp242 @nathany
Thanks!!


BTW, sorry for hijacked but I plan to reimplement fsevents without cgo, keeping backwards compatibility. Uses trampoline feature similar to x/sys/unix.

Are you interested in this proposal? Also, is this proposal acceptable?

@arp242
Copy link
Member

arp242 commented Jul 30, 2022

Sure, I'll definitely review and merge any PRs @zchee!

@zchee
Copy link

zchee commented Jul 30, 2022

@arp242 Thanks, I'll try to implements without cgo :D

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