🌱 add mboersma and richardcase as MachinePool reviewers

[jackfrancis]

What this PR does / why we need it:

This PR reorganizes the MachinePool controller code organizations to match other controllers (e.g., Machine) in order to better tag @mboersma and @richardcase as reviewers as we work on the v1beta2 MachinePool efforts:

• Tracking issue for v1Beta2 - MachinePools #12178

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

This PR is currently missing an area label, which is used to identify the modified component when generating release notes.

Area labels can be added by org members by writing /area ${COMPONENT} in a comment

Please see the labels list for possible areas.

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-sigs/prow repository.

[fabriziopandini]

TBH, considering that consistency across different API objects is a major theme of the ongoing work for v1beta2, I would prefer that we keep the current model that ensured that any change to the API surface is vetted from top level approvers; same goes for the contract documentation.

I have no objection instead to adding separated owner files to other folders where most of the work will happen (MP controllers, MP web hooks, Machine pool implementation CAPD).

Also, as a data point, according to https://cluster-api.sigs.k8s.io/contributing#contributors-ladder we introduced sub-level owner files in 2021, revamped it in 2022; based on what I saw in those years, the major benefit that sub-level files are producing is to provide a little bit of visibility and public recognition to active contributors; so far instead impacts on PR speed are not relevant or none at all (in other words, I don't see the owners file structure as a limitation or a blocker for the work in front of us).

[sbueringer]

but should enable more rapid collaboration for work items that are isolated to that code.

I'm not sure if approver bandwith is a problem at this point (at least I"m not aware of PRs that suffer from that at the moment).

I think it won't slow down the effort if PRs are reviewed between MachinePool reviewers and then a top-level approver adds a final approve.

Please also note that usually approver access is granted after folks demonstrate continuous contributions (including reviews) in the corresponding area over time and not ahead of time.

I'm fine with adding owner files to MachinePool directories and I would start with reviewer permissions. Even that is usually only done after a continuous history of reviews

[fabriziopandini]

Note: Machine pool will end up into /api/core folder with #12262

[jackfrancis]

@fabriziopandini @sbueringer appreciate the detailed feedback, that all makes sense.

Given that there aren't really MP-specific directories, there is probably not an OWNERS file solution here. Another benefit besides PR velocity (which you both have outlined these changes won't really improve) is auto-notifications, when a MP-affecting change lands, it'd be convenient to automatically tag Richard and Matt for reviews. But it sounds like the current arrangement of maintainers manually tagging them will work fine as well.

I don't see a way that we could modify this PR and feel confident that it will materially improve the v1beta2 effort. Do we all agree we should close this one?

[sbueringer]

I think one thing we can do is move some code around to move machinepool and CAPD DockerMachinePool controller code into machinepool directories and add Owner files there

We already have this folder structure for most of our other controllers: https://github.com/kubernetes-sigs/cluster-api/tree/8ca7179f801a2e9f6d39f4c1bb8a2e92e1f10dd0/internal/controllers
(So this would be a good thing to do anyway and should be low effort)

This should get Richard and Matt pinged for most MP PRs and it's also still a good way to signal ownership (I would just start with reviewer and then later promote to approver)

We also gradually wanted to get rid of the exp folders anyway. I can provide more details of what I would move around exactly if there's interest to do that

[jackfrancis]

/retitle add mboersma and richardcase as MachinePool reviewers

[fabriziopandini]

If I got Stefan's comment right, this should be "sigs.k8s.io/cluster-api/internal/controllers/machinepool" (without exp)

Same for CAPD

[sbueringer]

Yup

[sbueringer]

If we're okay with that

[sbueringer]

CAPD should be test/infrastructure/docker/internal/controllers/dockermachinepool/

[jackfrancis]

I took @sbueringer's suggestion

I think one thing we can do is move some code around to move machinepool and CAPD DockerMachinePool controller code into machinepool directories and add Owner files there

[jackfrancis]

I didn't realize we were ready to move out of exp, happy to do that if we want to (and can rename to dockermachinepool)

[sbueringer]

I let Fabrizio decide

[fabriziopandini]

CAPD doesn't have yet a folder for each controller, let's move the content of test/infrastructure/docker/exp/internal/controllers to test/infrastructure/docker/internal/controllers and rename files as dockermachinepool as suggested by Jack (we can add Matt and Richard into cluster-api-test-reviewers too)

Also, what about the webhooks folders

[sbueringer]

Not sure if we need that fine-granular owner ship in webhooks too. We have the same question for APIs and then the next question is: what if any of that code uses shared code.

I think for now I would mostly stick to the MP controller code

In general OWNERS might also allow ownership on a per-file level. But IIRC this broke a few years ago and I'm not sure if that's fixed already

We also have MP code sprinkled around the rest of the code base. E.g. e2e tests. I think for those cases top-level maintainers can always help out with an approve. Shouldn't prevent MP reviewers/approvers to do initial reviews / approves

Like we do in other repositories where we own config, but not actually have approver permissions everywhere (e.g. test-infra)

[sbueringer]

/hold
We want to merge #12262 first (probably we're merging 12262 tomorrow)