test-infra: ok-to-test not detected in review approval

Switching from the needs-ok-to-test label to a positive, ok-to-test label would allow the trigger plugin to check if a PR is ok to test by listing labels instead of listing issue comments, reviews, and review comments which would save API tokens. Using a positive label instead of a negative one avoids all the race conditions that force us to list comments and check for /ok-to-test instead of just checking for a label.

Now that the KEP has been merged, I will start implementing it soon… stay tuned.

We should just save all [PR | Issue] state in GitHub labels always 😃 Who needs etcd / CRDs when you have GitHub? /s

On Wed, Apr 18, 2018 at 5:45 PM Cole Wagner notifications@github.com wrote:

Switching from the needs-ok-to-test label to a positive, ok-to-test label would allow the trigger plugin to check if a PR is ok to test by listing labels instead of listing issue comments, reviews, and review comments which would save API tokens. Using a positive label instead of a negative one avoids all the race conditions that force us to list comments and check for /ok-to-test instead of just checking for a label.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/3827#issuecomment-382573374, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq9jl2e2yqqEg3uUU-3Hcb3KvPXsWks5tp94vgaJpZM4OpCto .

I’m not entirely convinced that /lgtm should serve as an /ok-to-test. Historically /lgtm meant “I will test this commit but do not trust your next one, which will also not have an LGTM label”

We may want to keep this behavior

KEP implementation tracked in #9754

/kind bug xref #7801 was a dupe reporting this by @xiangpengzhao

/help I still see people using /ok-to-test in PR reviews (as comments, aka no approval or change requested) and nothing happening

@ixdy Good point. The way Tide is configured now that workflow isn’t possible.

With the munger, we remove lgtm if any commits are changed, so it’s possible to approve a change to be merged but automatically require reapproval if anything changes.

The Submit Queue just handles this by ignoring needs-ok-to-test when triggering tests. With the SQ if a PR is untrusted, but LGTMed, it will be tested once at the time of /lgtm and then the SQ will comment with /test all to trigger retesting. The munge bot is an org member so the /test all triggers testing even if the PR has needs-ok-to-test.

I think we can solve this by changing the Tide query to allow triggering tests on PRs that have needs-ok-to-test and also employing the lgtm-after-commit munger (hopefully a plugin soon https://github.com/kubernetes/test-infra/issues/3795). That would allow us to stop testing a PR if it changes (because lgtm will be removed and it will no longer match the Tide query), but it would allow Tide to trigger retests as needed so long as the PR branch hasn’t changed.

I think a fix requires trigger plugin to register a plugins.ReviewEventHandler and check for /ok-to-test there as well.

@spxtr thoughts? I can volunteer to fix this if you see fit.