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

[cblecker]

Example: https://github.com/kubernetes/kubernetes/pull/49872

Other commands like /lgtm are

[cblecker]

/area prow

[nlandolfi]

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.

[spxtr]

SG. @cjwagner did this for other plugins.

[cjwagner]

Yeah the lgtm plugin uses review events and review comment events.

[xiangpengzhao]

xref: #2621

[cjwagner]

2621 is different. That is about making a github PR review that is submitted as "APPROVED" be considered as the same thing as /lgtm.

This issue is about making commands usable from PR review bodies and comments.

[kargakis]

@cjwagner is this fixed now?

[cjwagner]

No this hasn't been fixed.
The Trigger plugin has to list all comments to see if an /ok-to-test command exists every time it wants to check that a non-member PR is trusted. Checking reviews and review comments for this command would require at least 2 additional API tokens every time. Is that worth it?

[kargakis]

Can't we start handling review_comments?

[cjwagner]

We could, but that doesn't allow us to skip listing the reviews and review comments. We could check review_comment events and remove the do-not-merge/needs-ok-to-test label if the comment contains /ok-to-test, but we would still need to list all reviews and review comments when checking if a non-member PR is trusted to prevent race conditions with other events. See the discussion in the comments at the end of this PR https://github.com/kubernetes/test-infra/pull/5246.

[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]

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

[fejta]

/remove-lifecycle stale

[ixdy]

I think this is illustrating a workflow that's currently missing in prow and tide.

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.

AFAIK, this isn't possible with prow or tide.
I can /lgtm something, but that won't actually allow it to be merged unless I also /ok-to-test it. But if I do that, there's nothing to prevent it from being merged if anything changes.

It's also frustrating that I can't approve a PR to be merged without allowing all subsequent commits to be automatically tested, either - I have to /ok-to-test a PR to get tide to merge it.

[cjwagner]

@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.

[spiffxp]

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

[BenTheElder]

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

This includes me. Intuitively I should at least skim a PR's contents before /ok-to-test so it makes sense to leave this as a comment from the "files changed" UI, which makes a review comment instead of a normal comment :|

[cjwagner]

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.

[BenTheElder]

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 [e-mail]
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
.

[cblecker]

I understand the idea, but also, ugghhhhh.. label overload. Having a label just to say testing is okay.. bleh. Ugly. (No, I don't have a better suggestion)

[BenTheElder]

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

[matthyx]

Hi, I also like the suggestion of @cjwagner to switch to a positive ok-to-test instead of the negative one... but we have to plan the migration path:

1. fix this bug (as we anyway need to parse correctly the reviews)
2. add support for ok-to-test in trigger and maybe other plugins concerned (so basically supporting both old and new behavior)
3. add ok-to-test to all active PR that don't have need-ok-to-test
4. remove need-ok-to-test flags to PR
5. remove code that relies on need-ok-to-test

What do you think?

[matthyx]

/remove-help

[cjwagner]

fix this bug (as we anyway need to parse correctly the reviews)

We aren't currently fetching the reviews, just the comments. Fetching the reviews and review comments would use a lot more API tokens. If we switch to a positive label we don't need to check anything except the labels though.

add support for ok-to-test in trigger and maybe other plugins concerned (so basically supporting both old and new behavior)

I'm not sure how we would support both behaviors simultaneously (or that we would need to). How would we handle a PR missing both needs-ok-to-test and ok-to-test?

I think this is reasonable, but I also agree with @cblecker. Positive labels seem worse for contributor experience. Negative process labels are less intrusive because they disappear as the user satisfies the requirements so they can be addressed individually and incrementally. In a positive label world, users need to know what labels are not present in order to address unsatisfied processes. Additionally, the label set becomes more cluttered as progress is made instead of less.

For this use case I think a positive label is the right choice though. It would simplify a lot of code, reduce API token usage, and avoid all the race conditions. More importantly, we are already leaving a comment on the PR if the author needs /ok-to-test from a member, so authors are already receiving specific instructions on how to proceed if the ok-to-test label is absent.
If the only downside of this change is that more PRs have a label related to testing permissibility I think that is worth it, but this change definitely needs to be floated by more members of sig-testing and sig-contributor-experience.

[cblecker]

@cjwagner Do you envision ok-to-test be needed on all PRs, or only those from non-org members? e.g. prow looks for the label, or IsMember

[BenTheElder]

One more downside to positive labels in the short term is that they're
likely to be accidentally removed when users with write access edit labels
in the GitHub UI 🙄

On Wed, Jun 20, 2018, 20:40 Christoph Blecker [e-mail]
wrote:

@cjwagner https://github.com/cjwagner Do you envision ok-to-test be
needed on all PRs, or only those from non-org members? e.g. prow looks
for the label, or IsMember

—
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-398966502,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4Bq2DVxO0HxqLZYkn07vzavyLBogz_ks5t-xXLgaJpZM4OpCto
.

[cjwagner]

Do you envision ok-to-test be needed on all PRs, or only those from non-org members? e.g. prow looks for the label, or IsMember

@cblecker I was imagining all PRs (that would save the most API tokens), but only requiring the label on non-member PRs seems like a good compromise. If we went that route the label bloat would only affect a small fraction of PRs and even though we would be using more API tokens than if we always required the ok-to-test label, we would still be using fewer than we use currently.
That seems like a good starting point even if we end up deciding later that we want to require the label on all PRs.

[matthyx]

Alright, should we ask sig-contributor-experience to get their opinion on requiring ok-to-test for non-members PRs?

[matthyx]

/assign

[cjwagner]

Alright, should we ask sig-contributor-experience to get their opinion on requiring ok-to-test for non-members PRs?

Yeah that sounds like the next step. Maybe post in the sig-testing and sig-contribex slack channels?

[matthyx]

Sorry I have been delayed by verify-owners issue... I will prepare the message and post it Monday morning (California time) I think it will have more visibility than during the weekend.

[cblecker]

The more I think about this, the more I'm in favour of moving in this direction.

It's a big change though.. I think the first step for this would be a design proposal, both incorporating the comments from myself and @cjwagner above, as well as addressing how we would migrate from one system to another.. what that process would look like (label migrations, any tide changes, etc).

[cblecker]

And once we have that design proposal, we should send it out for comment to the contribex and testing e-mail lists, rather than just Slack.

[matthyx]

Okay, I can start working on that and drive it if you wish... but where should it be written? Do we have a space for RFCs or something similar?

[matthyx]

I think I've found it... gonna write it in https://github.com/kubernetes/community/tree/master/contributors/design-proposals/testing

[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

[BenTheElder]

/remove-lifecycle stale

[matthyx]

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

[matthyx]

KEP implementation tracked in #9754