k8s-ci-robot behaves inconsistently with approved/needs-ok-to-test labels

[vllry]

What happened:
When the needs-ok-to-test label is present, k8s-ci-robot will run tests if a PR is approved, but will not allow users to manually run tests.

What you expected to happen:
Either tests should not yet run, or ok-to-test should be added automatically.

How to reproduce it (as minimally and precisely as possible):

• Create a PR that has needs-ok-to-test label
• Approve the PR
• Try test/retest commands.

Please provide links to example occurrences, if any:
kubernetes/kubernetes#78385

[stevekuznetsov]

Technically, adding /lgtm will allow tests to run once and only once. So, this is working as implemented, but definitely not obvious what is going on behind the scenes.

/cc @cjwagner @fejta @spiffxp @cblecker

[cblecker]

Yeah, it's not the approve that's doing anything, it's the lgtm.

I wonder if we even need the "run only once" functionality at all.

[cjwagner]

I wonder if we even need the "run only once" functionality at all.

I think it is nice since PR reviewers may not remember to include a /test all on non-member PRs after they have reviewed, especially if they mainly review member PRs. I don't think there is a case where a reviewer would use /lgtm, but not want to let the tests run so inferring /test all from /lgtm on a non-member PR seems like a convenient behavior that wouldn't cause problems.

That being said I don't think we have any need for this functionality and I certainly agree that it is non-obvious to users. If we do remove it we would want to also update number 4 here: https://github.com/kubernetes/test-infra/blob/master/prow/cmd/tide/maintainers.md#best-practices

Perhaps the best option would be to make Prow comment with an explanation in cases where /lgtm triggers tests? I'm a bit hesitant to generate more comment spam though.
/shrug

[vllry]

My 2 cents would be to auto-add ok-to-test on lgtm/approve, since lgtm/approve implies that the PR is not spam or malicious.

[cjwagner]

My 2 cents would be to auto-add ok-to-test on lgtm/approve, since lgtm/approve implies that the PR is not spam or malicious.

That could be a nice option to have for some repos to opt into, but I don't think that we can do that globally due to security concerns.

The reason we have the distinction between lgtm/approved and ok-to-test is because ok-to-test is sticky to the PR when changes are made while lgtm is removed whenever the PR changes. If we made lgtm auto-add ok-to-test then someone could change their PR to include a bitcoin miner (or something even more malicious) after the PR is LGTMed and the presubmits would happily run it.
So we only use ok-to-test in cases where we have decided we trust the external contributor enough to not do that. Otherwise we just lgtm and the tests will only be run and rerun on the PR so long as the PR doesn't change from what was LGTMed.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.