add `granular_approval` flag to `approve` plugin

[ykakarap]

Context for the approve2 plugin:

• TODO: add links here

This is a WIP PR.
More details on how the plugin works will be added soon.

This PR needs a KEP for the new approve2 plugin.

TLDR' on approve2 plugin: This plugin is an updated approve plugin that supports granular approvals and regex in the OWNERS files

This commit adds a new PR status section to the approval notifier message. This section shows the status of approvals of the folders involved in the PR. It shows if all the files under a folder are completely approved, partially approved or unapproved. The folders used are the ones where the owners files associated with the original PR reside.

[alvaroaleman]

@ykakarap thanks for your work! Unfortunately, a 7k LOC PR is very time-consuming to review.

Could you please try to de-duplicate the tests and run them against both versions, denoting expected failures in the current approval plugin as applicable?

[alvaroaleman]

Could you please try to de-duplicate the tests and run them against both versions, denoting expected failures in the current approval plugin as applicable?

Or alternatively, maybe one commit that just copies the old approval plugin, then another commit with your changes on top of that?

[ykakarap]

Or alternatively, maybe one commit that just copies the old approval plugin, then another commit with your changes on top of that?

I will do this 🙂 Will make the review process easier.

[ykakarap]

Hey @alvaroaleman, I split the PR into 2 commits. The first commit simply duplicates the old approve as an approve2 plugin. The second commit has the all the changes that make up the new plugin, including registration of handlers and integration.

Hope this helps with the review. 🙂

PS: I am also working on a KEP that goes with this. This PR will probably remain as WIP till the KEP is hashed out.

/cc @nikhita

[nikhita]

/cc @mrbobbytables @spiffxp

[BenTheElder]

While rolling this out eventually likely needs review from contribex, I don't think this has to do a KEP.

I think we can make some pretty small changes comparatively based on the previous sig testing meeting discussion:

1. Put this in the approve plugin, gate it behind a config option instead of configuring as a different plugin name entirely, but it can and should still have distinct codepaths for the logic
2. Drop the 2 from the command regexes, for a more natural transition as repos phase in/out of enabling it.

Otherwise just needs cleanup and PR review. I know some people want this quite badly!

[ykakarap]

@BenTheElder ack!

I will be back working on this in the next few weeks.

+1 for the 2 points you made. :)

[ykakarap]

While doing the clean up I will also address the test pipeline failures on this PR.

[ykakarap]

/retest

[ykakarap]

@BenTheElder @spiffxp Hi 👋🏼 I have addressed both the comments on the PR and the comments I received from the SIG Testing meeting.

• I moved the granular approval behavior behind a granular_approval flag in the approve plugin and dropped the use of a separate approve2 plugin.
  • I found that the easiest and cleanest way to do this was to keep the granular approval variant of the approval plugins logic in a separate package and different from the original plugin. This is because the granular approval has a lot of changes and would introduce duplicates of almost all the functions in the approve/approvers package. Hence it seemed best to keep the behavior in separate packages. Hence granular approval logic sits in approve/approvers2 package.
• The second feedback point was to show the list of files that are yet to be approved and are already approved. The feedback was that having just the number of files that are yet to be approved might not give the full picture. The "status of the PR" section now will also show the list of files that are yet to be approved and approved (approved files are striked out). Please look at the below screenshot for an example:

The files list is under a collapsable section that is closed by default (the pkg/registry section in the above example was manually expanded). This is to have a cleaner look and avoid huge comments for large PRs.

PS: This PR does not change the configs of the approve plugin to use granular approvals for any of the repos. I was planning on rolling out this feature to only to 1 or 2 repos (is the k/test-infra repo a good candidate?) in the beginning to collect user feedback. Any advice on a different roll-out strategy is welcome 🙂 .

cc @dims @alvaroaleman @nikhita

[ykakarap]

/assign @alvaroaleman

[spiffxp]

Pinging for attention. I'd like to see us roll this out after enhancements freeze

[ykakarap]

/retest

[BenTheElder]

we discussed this in today's SIG meeting but honestly I've been a bit out of it and don't recall the outcome clearly ... cc @spiffxp @alvaroaleman

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[ykakarap]

/remove-lifecycle stale

[BenTheElder]

so this will need a rebase, but before we ask OP to do so, is this something we're willing to move forward with? cc @stevekuznetsov @alvaroaleman @chaodaiG @cjwagner

[cjwagner]

so this will need a rebase, but before we ask OP to do so, is this something we're willing to move forward with? cc @stevekuznetsov @alvaroaleman @chaodaiG @cjwagner

The idea SGTM assuming we have interest in this feature, but I don't think I can personally commit the time to reviewing this PR now.

[MadhavJivrajani]

/cc @michelle192837

[MadhavJivrajani]

/test pull-test-infra-unit-test

Not able to reproduce the unit test failure locally and the logs also seem to have been cleaned up. Failure is probably due to the merge conflict. But in case its not!

[MadhavJivrajani]

yep test failures are due to the merge conflict. @ykakarap once this is rebased, the failures should go away as you suggested!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ykakarap Once this PR has been reviewed and has the lgtm label, please ask for approval from spiffxp by writing /assign @spiffxp in a comment. For more information see:The Kubernetes 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

[ykakarap]

I addressed some of the comments. The broader items left that need to be address are:

• Break the commits further into logical block
• Refactor some of the tests
• ~Support IsAutoApproveUnownedSubfolders in approvers2~

[alvaroaleman]

/uncc

[MadhavJivrajani]

@spiffxp 👋🏼

I've worked on your review comments and tried to keep the commits as small as independent as possible for ease of review. Here is a summary mapping what commits map to which review comments and what the purpose of commits that lie outside this mapping is:

• fileNamesUnfiltered should be in a separate commit: https://github.com/kubernetes/test-infra/pull/21398#discussion_r943018706
  • Commit: https://github.com/kubernetes/test-infra/pull/21398/commits/2a0209e7b714d440be392a0c448973cd8a634022
  • Note: This is re-added later as part of a different review comment, will describe below.
• IsAutoApproveUnknownSubfolders is not fully added back yet: https://github.com/kubernetes/test-infra/pull/21398#discussion_r1000020172
  • Commits:
    • Re-add the implementation https://github.com/kubernetes/test-infra/pull/21398/commits/a1e6146a8ca8e14cb3ad81cb05b1fda9f5a580ad
    • Re-add original tests to test with granular approval: https://github.com/kubernetes/test-infra/pull/21398/commits/a57ff07670ff33bca4adcb8e0a868f2c52d4889b
    • Add new tests specific to granular approval: https://github.com/kubernetes/test-infra/pull/21398/commits/b6f8d8d94d1cac3b01d7ed159d24b4b006e02722
  • fileNamesUnfiltered is added back as part of this to keep the implementation as consistent as possible.
• Order of things in which the approve commands are processed (state not overridden correctly by comment): https://github.com/kubernetes/test-infra/pull/21398#discussion_r947202827
  • Commit: https://github.com/kubernetes/test-infra/pull/21398/commits/bed5fb6169709ff9879c9e3d3d0efb6cbe9633d0
• Redundant boilerplate due to bridge between approve and approve2: https://github.com/kubernetes/test-infra/pull/21398#discussion_r947222177
  • Commit: https://github.com/kubernetes/test-infra/pull/21398/commits/1ac52297714bb6bf2e33abd60b90de3f8898e788
• Using ** to indicate recursive matching: https://github.com/kubernetes/test-infra/pull/21398#discussion_r947264343
  • Commit: https://github.com/kubernetes/test-infra/pull/21398/commits/6b70454f0ebbde1e30213d07bb16fa8282e318b7

---

Interlude 1

While working on refactoring tests, I discovered a few pit-falls in the implementation and testing (or at least that's what my understanding is). Post fixing these, approve2 is compliant with approve except the message format. Here's a summary of these changes:

• GetApprovers() correction: https://github.com/kubernetes/test-infra/pull/21398/commits/ee02b8978944c2e9d3ce0c327ea59dfadd87603a
• GetLeafApprovers() correction: https://github.com/kubernetes/test-infra/pull/21398/commits/9902a80ad876c0a929ea8d9af7508558eb642539
• NoIssueApprovals were not taken into account while calculating state of approval: https://github.com/kubernetes/test-infra/pull/21398/commits/2f415e2b4066ee4aaa47cd36d49c3b7c46b90c79
• Small modification to the approver suggestion algorithm specific to approve2: https://github.com/kubernetes/test-infra/pull/21398/commits/f3d6d4c0e6d2be3deba211c1600016d8868dc33a

---

• Test refactoring: https://github.com/kubernetes/test-infra/pull/21398#discussion_r943009419
  • Commits:
    • Add helper function to unify tests of simple and granular approver: https://github.com/kubernetes/test-infra/pull/21398/commits/575f160eb56c9f4ff970459d3f6b51e07c0ed99f (this commit did not turn out as cleanly as I hoped it would, sorry about that!)
    • Test granular approval with test cases of simple approval to ensure it does everything simple approve does: https://github.com/kubernetes/test-infra/pull/21398/commits/98d30b9021a5e4d928f3009f128a2f995e333322
    • Remove test cases that are already present in baseline test from the granular approve test: https://github.com/kubernetes/test-infra/pull/21398/commits/1258aed48b5fcc2f3d3bff578439044df44f0261

---

Interlude 2

According to my understanding, approve2 interpreted no-issue as something that only bypasses the no-issue requirement but does not actually approve anything. This is different from simple approve where no-issue also results in approval. I guess it might make sense to have it such that it does not approve anything since that is a blanket approval but considering that no-issue hasn't been used in a while (https://github.com/kubernetes/test-infra/pull/6373) and no repos use it either, I'd like to keep the behaviour consistent across granular and simple approve until we sunset it (if at all).

Commits relating to this:

• Fix tests:
  • https://github.com/kubernetes/test-infra/pull/21398/commits/e3b9ffb08ed1603a8a2128889082fa9fe3ff9b9c
  • https://github.com/kubernetes/test-infra/pull/21398/commits/d2dc63199da00b1e11c956d4cafca8630209d45d

---

• Misc commits relating to changes to message format/tests added at a later point that need to adapt to the changes made here:
  • https://github.com/kubernetes/test-infra/pull/21398/commits/0b6c7d5d2952c5f4b928b92c4e7b4234537f1845
  • https://github.com/kubernetes/test-infra/pull/21398/commits/3fdd5ce6b9584528ae141961aaf77c3bc9b1d9e0

Please lmk what you think and thank you again for your detailed review!

[droslean]

/retest

[droslean]

@ykakarap What is the status of this PR?

[MadhavJivrajani]

@droslean 👋🏼 We've addressed any existing review comments and the only thing required now is reviewer bandwidth which is unfortunately and understandably a scarce resource at the moment :/

[droslean]

@droslean 👋🏼 We've addressed any existing review comments and the only thing required now is reviewer bandwidth which is unfortunately and understandably a scarce resource at the moment :/

The PR is conflicted. Please make sure that this is resolved, otherwise, there is no point in reviewing it.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ykakarap Once this PR has been reviewed and has the lgtm label, please ask for approval from spiffxp. For more information see the Kubernetes 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:

• prow/plugins/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]

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/test-infra repository.