Allow /approval variant for Prow approve plugin.

[wlynch]

It's common within internal Google code review to see "LGTM, Approval"
as a response to approved code reviews, so I've been somewhat conditioned to use
/approval with Prow, only to discover later that it didn't work.

This adds the ability for Prow to accept both /approve and /approval
as valid commands.

This also makes it easy to add other variants (e.g. /approved?) later
if we need to.

[k8s-ci-robot]

Welcome @wlynch!

It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/test-infra has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @wlynch. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wlynch
To complete the pull request process, please assign cjwagner
You can assign the PR to them by writing /assign @cjwagner in a comment when ready.

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

Needs approval from an approver in each of these files:

• prow/OWNERS

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

[alvaroaleman]

cool with me
/ok-to-test
/assign @fejta
(as I can not tell if the claim of what you are using internally is accurate or not :) )

[fejta]

Why not just leave an approving github review so you don't have to use /foo commands at all here? An example of my doing this: #18189 (review)

I am not sure the costs associated with this sort of change are worth it:

• Nobody leaves /approval nor /approve comments inside Google. We click one of a couple buttons/check boxes.
• We've tended to try and maintain a single way of doing things and expect people to learn this.
  • Humans are flexible and learn at learning a diversity of language constructs. Computers are terrible about this.
  • Once we allow this, why just these two and not also /approved, /approving? Why only English? What about what is common in other companies?
    • The marginal returns on identifying and addressing all these edge cases rapidly exceeds the costs.

But the most fundamental thing here is you should just leave an approving github review to approve, which you cannot get wrong.

To me the current behavior is clear:

• Approve by leaving an approving github review.
• Alternatively, you can leave an /approve comment.
• Those are the only two options (personally I would remove the case insensitivity too)

The two config knobs for this are:

test-infra/prow/plugins/config.go

Lines 268 to 271 in 49b063e

	// IgnoreReviewState causes the approve plugin to ignore the GitHub review state. Otherwise:
	// * an APPROVE github review is equivalent to leaving an "/approve" message.
	// * A REQUEST_CHANGES github review is equivalent to leaving an /approve cancel" message.
	IgnoreReviewState *bool `json:"ignore_review_state,omitempty"`

test-infra/prow/plugins/config.go

Lines 310 to 312 in 49b063e

	// ReviewActsAsLgtm indicates that a GitHub review of "approve" or "request changes"
	// acts as adding or removing the lgtm label
	ReviewActsAsLgtm bool `json:"review_acts_as_lgtm,omitempty"`

[wlynch]

But the most fundamental thing here is you should just leave an approving github review to approve, which you cannot get wrong.

The only counterpoint I have is that the GitHub code review Approve button only shows up in the Review Changes reply under /files

and not under any replies in the base pull request view (this is a consequence of this PRs being represented as an Issue):

though perhaps this is more a feature request for GitHub to implement. :)

Once we allow this, why just these two and not also /approved, /approving? Why only English? What about what is common in other companies?

That's fair. I'll close this out. Thanks!