kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.58k stars 1.32k forks source link

Create conditions for onboarding new reviewer & maintainers on sub-areas of the projects #5456

Closed fabriziopandini closed 2 years ago

fabriziopandini commented 3 years ago

Growing the reviewers/maintainer base is something we’ve been trying to do for a while. That being said, as the codebase has been growing, we’re trying to think of the best way to onboard people to OWNERs files going forward and once thing that we’d like to see is more reviewersmaintainers of subareas of the project (credits to @CecileRobertMichon for this phrase 👏)

As of today there are following OWNERS files/Owner groups defining sub areas.

Is there a particular area people are more familiar with and would be interested in reviewing/maintaining to start?

IMO there are still too much under the top level approver owner files, and thus I propose the creation of two additional OWNER files/Owner groups:

I really would like to get to a state where the top level OWNERS are required only for changes impacting our public API/public libraries, while everything else could be managed by experts in sub areas...

Opinions?

/kind cleanup /area code-organization

fabriziopandini commented 3 years ago

I'm going to give visibility to this topic by proposing this as discussion topic in Oct 20th office hour meeting

sbueringer commented 3 years ago

To clarify:

Is there a particular area people are more familiar with and would be interested in maintaining to start?

Is this targeted towards reviewers or maintainers or both?

fabriziopandini commented 3 years ago

To clarify:

Is there a particular area people are more familiar with and would be interested in maintaining to start? Is this targeted towards reviewers or maintainers or both?

This is target to anyone is interested in moving up the contributor ladder in CAPI

killianmuldoon commented 3 years ago

Will the process for maintainership in the specific areas be the same as the overall Kubernetes community guidelines?

sbueringer commented 3 years ago

To answer your question. Adding the docs/OWNERS and test/OWNERS (or aliases more or less) sounds good to me.

I'm not entirely sure about controllers/OWNERS. Is the idea more or less "core controller/webhooks" vs the kubeadm bootstrap and control plane provider?

Is there a particular area people are more familiar with and would be interested in reviewing/maintaining to start?

I would definitely be interested in becoming a maintainer, but that's probably not only a matter of being interested in it ;). From our current areas I think I'm probably most familiar with test/*. Everything else (except ClusterClass) is more like "reviewed a lot of PRs, implemented a few fixes almost everywhere, but nothing major".

fabriziopandini commented 3 years ago

I'm not entirely sure about controllers/OWNERS. Is the idea more or less "core controller/webhooks" vs the kubeadm bootstrap and control plane provider?

Now controller/ are under the responsibility of top level maintainers, and this makes it difficult to add new reviewers/maintainers specialised in this (huge) part of the code base. A possible solution is to define an OWNERS file for controller/ , or eventually we can even break it down more, e.g having subareas for machine health checks, machinedeployments/machineset etc. But given that this is for helping new people to step up, it will be great to have some feedback about if this makes sense and the right level of granularity we should aim to

sbueringer commented 3 years ago

I was thinking about doing it like this: Adding the new OWNER_ALIASES: cluster-api-core-maintainers and cluster-api-core-reviewers and then using them in controllers/OWNERS, (once Prow supports it) top-level go.mod/go.sum, Makefile, ..., possible top-level webhooks/OWNERS (in case we add that package) and similar files/folders which belong to the core provider. (I would also include the api package as that belongs to the core provider, like the kubeadm APIs to those providers)

Then we would have separate aliases for all providers.

Afaik it's currently not possible to specific reviewers/approvers for individual files in a folder via regexp (https://github.com/kubernetes/test-infra/issues/7690). So if we want separate reviewers/approvers for the individual controllers we would have to move the controllers in separate packages or wait until the Prow feature has been implemented.

vincepri commented 3 years ago

/milestone v1.0

elmiko commented 3 years ago

docs/OWNERS and test/OWNERS make a lot of sense to me. i have less opinion on controllers/OWNERS, but that seems like a logical place unless we think we might have folks who want to specialize in one controller over another, which sounds like over-specialization to me.

randomvariable commented 3 years ago

I thought I was a reviewer of the book for some reason - though that meant I could only lgtm not approve.

I would volunteer for the following:

enxebre commented 3 years ago

+1 for docs/OWNERS and test/OWNERS @elmiko you interested in any of these? controllers/OWNERS seems reasonable to me cc @JoelSpeed in case you are interested.

Additionally we are most likely moving the operator work into its own repo to favour sane and independent dev workflow/release cadence so this would be another area of interest for new contributors/reviewers/maintainers cc @alexander-demichev

I'm concern this issue might not have enough visibility so I'll share the proposed areas structure via slack/google groups/anything else? so we wildly communicate that help and new contributors of any level are very welcome to the project and so everyone has the chance to contribute and focus in any particular area.

Once everyone have had the chance to speak I'll put a PR with the new updates and we can do some lazy consensus or so.

As a follow up for this issue I'd like us to possibly start discussing and brain storm to come up with a more structure program to reduce barriers for new joiners, e.g groomed targeted backlog, templates with regular tasks, mentorship assignments...

/assign

JoelSpeed commented 3 years ago

controllers/OWNERS definitely does make sense to me, but I am curious about how this might work when people have different expertise. Seems I have similar concerns to @sbueringer. The controllers folder contains a lot of different subjects (machine, cluster, machinedeployment, machineset, machinehealthcheck, the different subfolders) and I fear it's hard for one person to become an SME on all of these, and so, while it might be suitable to add them as an OWNER for machinehealthecheck, it might not be so suitable for the others? Is there a way we can make this better short of moving everything into individual folders?

Additionally we are most likely moving the operator work into its own repo to favour sane and independent dev workflow/release cadence so this would be another area of interest for reviewers/maintainers cc @alexander-demichev

This feels like a very sensible move to me, if we don't do it, we should definitely have separate owners for it.

As a follow up for this issue I'd like us to possibly start discussing and brain storm to come up with a more structure program to reduce barriers for new joiners, e.g groomed backlog, templates with regular tasks, mentorship assignments...

Would love to see something like this, let me know where I can help

fabriziopandini commented 3 years ago

Thanks for the valuable feedbacks! 2 comments from me at this stage.

Also, it seems there is a consensus to have docs/OWNERS and test/OWNERS, so let's get this rolling + let's document sub areas in the contributing guide

vincepri commented 3 years ago

Let's start small folks and try to fill in the roles we already have, creating further segmentation of the codebase it's fine but I'd want to remain within logical boundaries and areas of responsibilities. I'd also point out that, usually, too many steps or boundaries has the opposite effect on contributions, my vote is to keep things smooth and easy to explain to newcomers.

enxebre commented 3 years ago

Also related I created https://github.com/kubernetes-sigs/cluster-api/pull/5503

Let's start small folks and try to fill in the roles we already have, creating further segmentation of the codebase it's fine but I'd want to remain within logical boundaries and areas of responsibilities. I'd also point out that, usually, too many steps or boundaries has the opposite effect on contributions, my vote is to keep things smooth and easy to explain to newcomers.

Makes sense to me. For folks who already showed interest here how do we want to proceed and get some of the subareas started? Should we have farther discussion about the concrete roles people seek so then I can put a PR adding everyone in one go? Do we want each individual putting a single PR? cc @sbueringer, @randomvariable, @elmiko, @killianmuldoon, @JoelSpeed, @vincepri, @fabriziopandini, @CecileRobertMichon.

sbueringer commented 3 years ago

@enxebre I think individual PRs would be best so they can be approved separately.

alexander-demicev commented 3 years ago

We are definitely interested in operator development. If making it a separate project will help to speed up the development, I'll support this decision.

fabriziopandini commented 2 years ago

@vincepri @enxebre @CecileRobertMichon @sbueringer any objection about closing this now? I think that the initial steps for using the sub-area approach has been completed, the contributor guide updated, so now we can reconsider add OWNERS file case by case when the need arise

sbueringer commented 2 years ago

+1 for closing this issue

As a follow up for this issue I'd like us to possibly start discussing and brain storm to come up with a more structure program to reduce barriers for new joiners, e.g groomed targeted backlog, templates with regular tasks, mentorship assignments...

@enxebre A follow-up for this point would be nice.

fabriziopandini commented 2 years ago

/close let's open other issues to track additional onboarding efforts

k8s-ci-robot commented 2 years ago

@fabriziopandini: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/5456#issuecomment-971436760): >/close >let's open other issues to track additional onboarding efforts Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
enxebre commented 2 years ago

@enxebre A follow-up for this point would be nice.

@sbueringer let me draft something and I'll create a follow up issue