cherrypickapproved: Add option to approve PRs without lgtm or approved labels

[xmudrii]

This PR extends the cherrypickapproved plugin with two new configuration options: allow_missing_approved_label and allow_missing_lgtm_label. Those two fields are bools defaulted to false. If the option is set to true, the respective label will not be needed to successfully cherry-pick approve the PR.

This is very useful for projects where approvers are also cherry-pick-approvers. At the moment, this plugin is not much useful to those projects, because you either have to approve the PR two times or manually remove the cherry-pick-not-approved label. This change should make the user experience a bit better for this use case.

/assign @saschagrunert @cpanato @puerco
cc @kubernetes-sigs/release-engineering

[netlify]

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	f5ae2b6
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-prow/deploys/66b37796afaec20008aca3f1
😎 Deploy Preview	https://deploy-preview-220--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[saschagrunert]

/assign @cjwagner
/lgtm

[cjwagner]

/approve
/hold
Holding for the two required_reviewers listed in the OWNERS file:

• @MadhavJivrajani
• @Priyankasaggu11929

[Priyankasaggu11929]

@xmudrii, I have a few questions (please correct me if I'm misunderstanding this plugin):

The cherrypickapproved plugin (before this PR) requires the approved / lgtm labels from code/component OWNERS. Once these labels are in place, and a cherry-pick-approvers approves the PR, the cherry-pick-not-approved label is removed, and cherry-pick-approved is applied.

With the new config options (allow_missing_approved_label & allow_missing_lgtm_label) set to True:

• Where is the check to ensure that the person approving the cherry-pick is both (i) listed as a cherry-pick-approvers and (ii) a code OWNERS for the repo/path? Currently, we only check if they are in cherry-pick-approvers (here), not a code/component OWNERS.

• Does this mean that with these config options enabled, any cherry-pick-approvers can apply the cherry-pick-approved label (and remove cherry-pick-not-approved) regardless of their OWNERS status?

At the moment, this plugin is not much useful to those projects, because you either have to approve the PR two times or manually remove the cherry-pick-not-approved label.

I'm not sure how easy/ hard this is to implement, but an alternative could be to add additional check if the person approving the cherry-pick is listed both in OWNERS and cherry-pick-approvers. If so, all labels (lgtm, approved, cherry-pick-approved) would be applied (and cherry-pick-unapproved removed) instead of skipping the lgtm and approved labels check.

[xmudrii]

@Priyankasaggu11929 Thank you for the review! I'll do my best to answer all the concerns.

The cherrypickapproved plugin (before this PR) requires the approved / lgtm labels from code/component OWNERS. Once these labels are in place, and a cherry-pick-approvers approves the PR, the cherry-pick-not-approved label is removed, and cherry-pick-approved is applied.

Yes, that's correct.

With the new config options (allow_missing_approved_label & allow_missing_lgtm_label) set to True:

Where is the check to ensure that the person approving the cherry-pick is both (i) listed as a cherry-pick-approvers and (ii) a code OWNERS for the repo/path? Currently, we only check if they are in cherry-pick-approvers (here), not a code/component OWNERS.

Does this mean that with these config options enabled, any cherry-pick-approvers can apply the cherry-pick-approved label (and remove cherry-pick-not-approved) regardless of their OWNERS status?

Yes, that's correct and I have done it intentionally this way. I've been long thinking if there should be some check of the OWNERS status, but then I realized that it might not fit different situations.

For example, let's say that I'm responsible for approving all cherry-picks, including those cherry-picks for which I'm not code/component OWNER. I could still approve the cherry-pick from the "administrative side" (e.g. it satisfies the cherry-pick criteria), but leave it to the code OWNER to review the PR and approve/lgtm it later. Because of that, even if we would implement check of the OWNERS status, I would still make it optional.

This combination as-is (both options set to True) allows both the case that I described in the PR description and this use case explained in this comment. If this is risky for some organizations, they can always leave allow_missing_approved_label to false until we don't come up with a "better" implementation.

Additionally, it's important to clarify two things. This doesn't have any impact to the Kubernetes projects, as we'll not use this option. This change is being made for other Prow users that have similar processes in place as Kubernetes, but yet a little bit different, making these options (allow_missing_approved_label & allow_missing_lgtm_label) required.

The second thing is even if I'm not a code OWNER, but cherry-pick approve the PR, it's only going to affect the cherry-pick-unapproved/cherry-pick-approved label. The PR is not going to get merged until an OWNER doesn't lgtm/approve the PR.

Finally, I'm not really sure how we could implement this case, it potentially requires a lot of work as I'm not sure if that part of the code is reusable from the approvers plugin or we need to reimplement it from scratch. There are many different cases as well, including handling OWNERS_ALIASES, teams, etc...

[Priyankasaggu11929]

For example, let's say that I'm responsible for approving all cherry-picks, including those cherry-picks for which I'm not code/component OWNER. I could still approve the cherry-pick from the "administrative side" (e.g. it satisfies the cherry-pick criteria), but leave it to the code OWNER to review the PR and approve/lgtm it later. Because of that, even if we would implement check of the OWNERS status, I would still make it optional.

Yes. Though, to me at-least, it feels that the original design of the cherrypickapproved plugin is intended to ensure that code review and approval from "all code owners" are completed before the changes are approved for cherry-picking.

Finally, I'm not really sure how we could implement this case, it potentially requires a lot of work as I'm not sure if that part of the code is reusable from the approvers plugin or we need to reimplement it from scratch. There are many different cases as well, including handling OWNERS_ALIASES, teams, etc...

Yea, it would be somehow partly integrating approve plugin handler (or similar) before the cherrypickapproved plugin performs its own check for right labels.
The approve plugin take care of all different potential cases of approvers from multiple OWNERS files (on different paths, including root owners).

Is there an example to see how Prow currently reacts, when both approve & cherrypickapproved plugins are configured for a repo & someone listed as both cherry-pick-approvers and appropriate code OWNERS adds the approve/lgtm labels?

Unblocking the merge, since the config options are not to be used by Kubernetes project repos and doesn't change default behavior.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, Priyankasaggu11929, saschagrunert, xmudrii

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/plugins/OWNERS [cjwagner]

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

[xmudrii]

Is there an example to see how Prow currently reacts, when both approve & cherrypickapproved plugins are configured for a repo & someone listed as both cherry-pick-approvers and appropriate code OWNERS adds the approve/lgtm labels?

cherrypickapproved plugin does nothing in that case. I don't have an example in the Kubernetes org at the moment, but here are some examples from other org with this setup: kubermatic/kubeone#3326, kubermatic/kubeone#3335, kubermatic/kubeone#3264

In the first two examples, I resorted to applying the label manually. In the last example, I tried approving the PR again which also did the trick.

Unblocking the merge, since the config options are not to be used by Kubernetes project repos and doesn't change default behavior.

Can I remove the hold from this PR or should I leave it for @MadhavJivrajani to take a look?

[Priyankasaggu11929]

cherrypickapproved plugin does nothing in that case. I don't have an example in the Kubernetes org at the moment, but here are some examples from other org with this setup: kubermatic/kubeone#3326, kubermatic/kubeone#3335, kubermatic/kubeone#3264

In the first two examples, I resorted to applying the label manually. In the last example, I tried approving the PR again which also did the trick.

Thanks for the example links, @xmudrii.

yea, good to unhold from my end.

[xmudrii]

Thank you, @Priyankasaggu11929!
/hold cancel

[xmudrii]

/retest