cluster-api
cluster-api copied to clipboard
kubernetes-sigs
🌱 add mboersma and richardcase as MachinePool reviewers
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:
- https://github.com/kubernetes-sigs/cluster-api/issues/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 #
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
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.
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).
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
Note: Machine pool will end up into /api/core folder with https://github.com/kubernetes-sigs/cluster-api/pull/12262
@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?
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
/hold We want to merge https://github.com/kubernetes-sigs/cluster-api/pull/12262 first (probably we're merging 12262 tomorrow)
PR needs rebase.
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.
I think the proof is in the pudding that Richard and Matt have been effective without any of these changes. :)