Test-infra: k8s-ci-robot doesn't remove `do-not-merge/work-in-progress` label from draft PRs

Created on 26 Mar 2019 · 16Comments · Source: kubernetes/test-infra

What happened:
A PR I made in kubernetes/sig-release was started as a draft (waiting for another PR to merge). When I pressed the "ready for review" button, k8s-ci-robot did not remove the do-not-merge/work-in-progress so despite approval, the PR was not merged.

What you expected to happen:
When draft PRs are ready for review, the do-not-merge/work-in-progress label should be removed by k8s-ci-robot

How to reproduce it (as minimally and precisely as possible):
Create a draft PR on a kubernetes repo. Then make it "ready for review"

Please provide links to example occurrences, if any:
https://github.com/kubernetes/sig-release/pull/565
Anything else we need to know?:
You're awesome :1st_place_medal:

good first issue help wanted kinbug

Source

onyiny-ang

👍3

All 16 comments

I would like to work on this, if anyone else isn't.

anuragprafulla on 27 Mar 2019

🎉1

/lifecycle active

Please do!

fejta on 28 Mar 2019

@fejta I am having a little bit of trouble finding a starting point to fix this issue. This is my first issue of test-infra. Can you tell me where should I go looking first?

anuragprafulla on 29 Mar 2019

Shouldn't this have resolved itself now that GitHub is publishing the correct data?

stevekuznetsov on 29 Mar 2019

GitHub delivers webhooks to the hook server and we digest them:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/hook/server.go#L69-L71

When we see an event for a PR, we run our handlers for it:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/hook/server.go#L107-L115

Handlers are dynamically registered on startup based on which plugins are configured; we dispatch to them at the time we get the webhook:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/hook/events.go#L174-L176

The wip plugin registers this handler for pull request events:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/plugins/wip/wip-label.go#L54-L56

The wip plugin determines if the pull request event was delivered for a draft PR by looking at the webhook payload:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/plugins/wip/wip-label.go#L86

The plugin then determines if the label needs to exist by checking draft status and title content:

https://github.com/kubernetes/test-infra/blob/f7e3d4332d3531ad6507603554d438d84f2fd6e3/prow/plugins/wip/wip-label.go#L115

The reason we had a bug was that GitHub was not publishing pull request events when the status changed from draft to normal, so we never got a chance to revisit the PR.

stevekuznetsov on 29 Mar 2019

@stevekuznetsov is it possible to provide a link to the reference of github publishing the correct data? If the issue is on github's end and their pull request events code has changed in the last 4 days, it might be useful to link anything associated with that as well--as it's unclear where to look for that.

onyiny-ang on 29 Mar 2019

Have we tried toggling a PR from draft and back to validate this works yet?

fejta on 29 Mar 2019

Ah, it's a new type of event, pull_request.ready_for_review as mentioned here. As communicated here the events should now be sent. So we need to update the pull request event types to add the new one and update the plugin to respond to that type.

stevekuznetsov on 29 Mar 2019

👍1

@stevekuznetsov Thanks a lot, for the help. I'll start working on it right away.

anuragprafulla on 29 Mar 2019

❤1

@stevekuznetsov @fejta, I have made changes in github/types.go and hook/events.go to handle such pull request. I feel that the plugin should be able now handle cases where the draft status is not set and remove the do-not-merge/work-in-progress plugin. I think that I should be able to send a PR. Before that, I needed to know about how I should perform the tests that verify if everything is working.

anuragprafulla on 1 Apr 2019

@anuragprafulla if you've got a GitHub repository you can have it try to post the webhook somewhere and validate that the structure we expect is correct. If we know the structure is right, you can use a file with the webhook payload and the phony tool in this repo to send a fake webhook to a locally-running hook instance and see your changes work.

stevekuznetsov on 1 Apr 2019

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 on 30 Jun 2019

/remove-lifecycle stale
/help
/good-first-issue

stevekuznetsov on 1 Jul 2019

@stevekuznetsov:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/remove-lifecycle stale
/help
/good-first-issue

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.