Repository navigation
Allow self approving PRs and add the ability to separate the intention to approve or lgtm #715
Description
It is useful to allow an owner to add a lgtm and not an approve, allowing someone else (which may be better suited) to approve.
Currently this is not possible, as any lgtm from an owner will translate to an approve.
In addition, if we consider an /approve to mean "As a maintainer, I agree to maintain this change/code", then self approving seams reasonable, assuming the approver requires enough reviews (lgtms) from trusted reviewers.
With this in mind, a default behavior should probably be adopted from the prow bot, where a merge requires both an lgtm and approve.
Self approving is a not ideal, but it allows SIGs with single maintainers to operate. Otherwise, they would need root maintainers all the time.
Activity
dbasunag commentedon Apr 17, 2025
The idea of self approving a pr seems a bit strange. One, theoretically, won't create a pr, if they thought they were not doing the right thing. If we assume they are 100% correct, then their PRs should directly get merged, without waiting on any approvals. By the way github does not allow owner of a pr to approve their own PR either, so making the webhook work around this, seems not right.
sig's with one owner should work on growing the team and possibly consider adding another sigowner (from a different sig as a temporary approver) or work with the root approvers till their sig grows.
Your main issue is, network sig requires root approval (by default it is needed). You would need to work with the maintainers to remove that obstacle.
EdDev commentedon Apr 17, 2025
The K8S community disagrees. gerrit users also would disagree, we worked like this in the past.
But all this is not a good enough argument.
The argument that I think is relevant was given in the original message.
It is also inaccurate, because it is up to an approver to take into account reviewers. In this case, lgtm is still needed.
IIUC, @dbasunag , you acted as a sole maintainer for a long time, therefore you had to self merge your PRs anyway.
History shows it failed, we barely managed to have one in our projects. Maybe when the SIGs mature enough for this to happen, then self-approving can be removed, but until then it is a blocker that slows down progress.
In my case, sig-network needs room to operate and have progress. Merging things in their codebase is under their responsibility, successes and failures are on them to own.
They cannot do that if their approver is only a single one, or at least, they cannot progress as they would like to.
But I think this will be true for every other SIG if the goal is to have distributed ownership and less centralization.
dbasunag commentedon Apr 17, 2025
Do you have any pointers around this? I would love to see some information on how they handled people's own bias against their own code.
I did and I don't wish it on anyone. It was too risky, stressful and there was no one to ask me questions or give good advices. It reduces quality of code drastically over a period of time. At one point I had to start reaching out to Meni for reviews, when others did not review my PRs despite of pings, even though he was busy and was not part of the org. This is a problem for the management to solve. Working around it, only makes the problem seem less important - in my case I never got another full time maintainer.
This should be the opposite way in my opinion. If the sig is not mature enough, they should not have merge rights. But when they do, they should not require root approvals anymore.
You should be able to do that if root_approval is turned false. You just need to work Ruth to get to that point.
EdDev commentedon Apr 17, 2025
The most relevant information is here.
In summary, self lgtm is not allowed, but self approve is.
The reason is simple, once one reached approval status, they are trusted and they are accountable of anything they approved. So even if they are wrong, they are the ones who will also fix it.
No one can mature without first owning things and be accountable.
Projects that want to survive growth and progress must delegate ownership, otherwise they will collapse. At least, that is my take. But I need to remind myself that we are talking about self approving, which is a maintability problem if not provided at this stage.
For the short to medium period there most likely will be only one approver per SIG (not only in network), therefore the problem is partially still in place.
This needs to be solved and we cannot continue with defensive approaches because they have not been proven to work well so far.
rnetser commentedon Apr 19, 2025
I am ok with the proposed approach with one disclaimer:
Looking at https://www.kubernetes.dev/docs/guide/owners/#quirks-of-the-process
A pr can be self-approved but should have 2 reviewers
lgtmitmyakove commentedon Apr 22, 2025
I'm OK with adding this support.
myakove commentedon Apr 24, 2025
#720