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

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

Closed
fabriziopandini opened this issue Oct 20, 2021 · 22 comments
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone

Comments

@fabriziopandini
Copy link
Member

fabriziopandini commented Oct 20, 2021

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.

  • clusterctl --> cmd/clusterctl/OWNERS
  • CABPK --> bootstrap/kubeadm/OWNERS
  • KCP --> controlplane/kubeadm/OWNERS
  • CAPD --> test/infrastructure/docker/OWNERS
  • ClusterClass/managed topologies --> controllers/topology/OWNERS

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

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 20, 2021
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Oct 20, 2021
@fabriziopandini
Copy link
Member Author

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

@sbueringer
Copy link
Member

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
Copy link
Member Author

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

@fabriziopandini fabriziopandini changed the title Create conditions for onboarding new maintainers on sub-areas of the projects Create conditions for onboarding new reviewer & maintainers on sub-areas of the projects Oct 20, 2021
@killianmuldoon
Copy link
Contributor

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

@sbueringer
Copy link
Member

sbueringer commented Oct 20, 2021

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
Copy link
Member Author

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
Copy link
Member

sbueringer commented Oct 20, 2021

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 (kubernetes/test-infra#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
Copy link
Member

/milestone v1.0

@k8s-ci-robot k8s-ci-robot added this to the v1.0 milestone Oct 20, 2021
@elmiko
Copy link
Contributor

elmiko commented Oct 20, 2021

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
Copy link
Member

randomvariable commented Oct 20, 2021

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:

  • test framework & e2e
  • KCP
  • And probably bootstrap/kubeadm/internal/cloudinit

@enxebre
Copy link
Member

enxebre commented Oct 21, 2021

+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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

enxebre commented Oct 26, 2021

Also related I created #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
Copy link
Member

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

@alexander-demicev
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member

+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
Copy link
Member Author

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

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: Closing this issue.

In response to this:

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@enxebre
Copy link
Member

enxebre commented Nov 17, 2021

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

No branches or pull requests

10 participants