galaxyproject / training-material

A collection of Galaxy-related training material
https://training.galaxyproject.org
MIT License
294 stars 846 forks source link

Add CODEOWNERS file for automatic PR review requests #539

Closed bebatut closed 6 years ago

bebatut commented 6 years ago

As suggested by @erasche in #489, this PR add a CODEOWNERS to have automatic PR review requests of the maintainers of each topic

Can you check if it is ok with you?

bgruening commented 6 years ago

+/- 0 from my side. I don't think we have (currently) a problem that people do not review PRs. Moreover, assigning multiple persons to PRs can have a counter effect that no one feels responsible. I tend to trust people, that they feel responsible for a certain topic - if a PR is really open for a long time we can still manually ping people. My feeling is that this generates a lot of unneeded spam, without clear guidelines (how many assigned people need to agree?) and (currently) unneeded.

Either way, this file should be auto generated imho if we really need this.

martenson commented 6 years ago

I am with @bgruening. This repository does not seem to have a problem with reviewing PRs so this feels like premature optimization to me.

To be frank I think this repo could benefit from a bit more anarchy e.g. allowing trusted folks directly pushing.

-0

shiltemann commented 6 years ago

I dunno, I like it, not everybody watches this repo that closely maybe and they may appreciate the ping. And I feel like a lot of the reviewing is done by B&B rather than topic maintainers currently ..as long as people can opt out of being added here if they don't want to be? ..and I'm not sure it would make people feel less responsible, it could also make them feel more invited

I could even imagine people appreciating this on a tutorial-level ..if you contributed a tutorial you may like to be notified of proposed changes without having to watch the repo and see /all/ the activity/noise coming by. Maybe if we autogenerate this from the metadata files we can let people set their preferences in the contributors.yaml file orso (with whatever default we decide is best)

shiltemann commented 6 years ago

ping also @Slugger70 @Stortebecker @dpryan79 @joachimwolff as you're listed as topic maintainers but I don't seem to be able to request a review from you for some reason

martenson commented 6 years ago

@shiltemann they do not have write access (good limitation of codeowners to know)

shiltemann commented 6 years ago

@martenson ah yes, good to know indeed, maybe we should give topic maintainers write access anyway so they can merge PRs in the topics they're responsible for? The tutorial-level idea will be harder unless we go bioconda style and just add everybody (which may get you the anarchy you wanted ;)) but let's see if there is any demand for any of this first, I have no idea

hexylena commented 6 years ago

Granting write to topic maintainers might be good. @joachimwolff asked me to look at #540 before we realised that neither he, nor @dpryan79 (the other methylseq maintainer), nor I (one of the admin stuff maintainers) had the ability to merge.

bgruening commented 6 years ago

it could also make them feel more invited

Just speaking for myself, I don't feel invited if I get spammed from a bot. In contrast to be friendly pinged by a community member.

if you contributed a tutorial you may like to be notified of proposed changes

This is different from Maintainers, right? This PR will not solve this problem for Contributors.

repo and see /all/ the activity/noise coming by ...

I don't see where IUC or our other repos are different from this one here. And we never had this issue in others. This feels way over engineered and less human for me. But maybe I'm getting too conservative ;)

A simply rule how to name PRs and people can filter messages, all people not only maintainers.

martenson commented 6 years ago

Also labeling goes a long way.

screenshot 2017-09-19 14 07 12

screenshot 2017-09-19 14 07 18

shiltemann commented 6 years ago

@bgruening sure, fair enough.

Github notification just feel a bit all-or-nothing for me sometimes (e.g. I would love it if I could get notifications only for the mothur recipe in bioconda but not all the rest) and I do miss things in busy repos sometimes because of this, especially in periods when I'm a bit less active on GH (guess what I really want is more fine-grained control over github notifications)

..for me personally this PR doesn't add much as I already keep a close eye on the repo, and if all of the maintainers feel the same we are trying to solving a non-existent problem, agreed

This is different from Maintainers, right? This PR will not solve this problem for Contributors.

oh sure, this was just me thinking we could add tutorial-level rules here for contributors (if they so desired), but that is probably overengineering it, yes (and before I realized this only works for people with write access)

@martenson good point about the labels

Stortebecker commented 6 years ago

For me, it does not matter much if people are pinged automatically or personally. And yes, getting write access to update tutorial-related stuff more frequently would be nice.

martenson commented 6 years ago

I have invited @Slugger70 @Stortebecker @dpryan79 @joachimwolff as collaborators with write access to this repo.

Thank you very much for your contributions!

shiltemann commented 6 years ago

thanks @martenson! ..don't forget @erasche :)

martenson commented 6 years ago

@shiltemann does he deserve it? 🤔

(🤣)

jokes aside, @erasche has access through the committers group, he has maaaaaany repos to push to

dpryan79 commented 6 years ago

@martenson Thanks much!

Stortebecker commented 6 years ago

@martenson Merci!

shiltemann commented 6 years ago

@martenson are you sure? there is a grey checkmark above in front of @erasche's name indicating no write access at time of review (and confirmed by comment) ..looks like you don't get automatic write access from being committer?

martenson commented 6 years ago

@shiltemann I think you do, but in the meantime @bgruening restricted pushing to master of this repo (so now almost nobody does), please talk to him

bgruening commented 6 years ago

@martenson ? @erasche was simply not added. I added him now.

hexylena commented 6 years ago

as of just now ... utvalg_208

(Apologies to everyone else who is getting constant emails from this that they don't care about)

martenson commented 6 years ago

@bgruening that is not true, you restricted master pushing to yourself, B! and Saskia. I told you it affects PRs. ;)

bgruening commented 6 years ago

@martenson look at the grey checkmark above its green now in Erics case. Thats what I'm talking.

martenson commented 6 years ago

@bgruening that just means the CI did not report error, doesn't it?

edit: I see which checkmark you mean now, but they still cannot merge to master because of the protected status

hexylena commented 6 years ago

Ooh! I have a merge button now. Thanks BB!&S <3