Test-infra: [approvers] Don't Notify Approver Until After LGTM
Approvers is intended to be a lightweight mechanism, but mentioning approvers immediately, they are subscribed to the issue and get all the review and discussion comments.
Potentially, wait to cc the approver until lgtm is applied.
grodrigues3
All 14 comments
Did you receive that many complaints? if the scope of approval and lgtm is so distinct, then you should be able to get one and then the other. Can we unsubscribe the approver when he approves? :D
apelisse
on 30 Mar 2017
I would much appreciate it if I wasn't cc'd as an approver until LGTM.
ixdy
on 30 Mar 2017
I don't like the idea, because it means PRs will take strictly longer to merge.
spxtr
on 30 Mar 2017
is that true? I generally don't approve PRs until they've been LG'd. I'm not sure what others do, though.
ixdy
on 30 Mar 2017
Can we unsubscribe the approver when he approves?
This doesn't solve the problem of the approver receiving all of the comments/discussion up until approval. I think you're imaging a situation where an approvers right away and is unbothered. Seems annoying to get a whole thread about something you're not very interested in.
I don't like the idea, because it means PRs will take strictly longer to merge.
I'm inclined to agree with @ixdy , but we have no data to back this up one way or another (that's another problem). I'd like to encourage authors/reviewers to reach out to any of the owners if the cc'd owner is unresponsive.
grodrigues3
on 31 Mar 2017
Gubernator is now properly parsing the approval blocks, so the dashboard will properly show PRs that need your approval.
However, the bot also assigns the approvers, meaning they receive all the notification spam. What if we just removed the assignment portion?
rmmh
on 13 Apr 2017
does the bot assign approvers? or just cc them?
ixdy
on 13 Apr 2017
I think it just cc's them. @apelisse made some changes recently though.
grodrigues3
on 13 Apr 2017
@kubernetes/sig-contributor-experience-feature-requests @Sevii @lukaszgryglicki
This is a pretty simple one which could really improve our contributor's experience.
grodrigues3
on 14 Apr 2017
@crimsonfaith91
You could also try this one out
grodrigues3
on 14 Apr 2017
Noted. I will have a look at it asap.
crimsonfaith91
on 14 Apr 2017
@spxtr @apelisse Yes, I complained shortly after the approval mechanism was turned on, and so did @thockin.
A direct result of the current approach is that I blackhole all github notifications, which I'm sure makes it take longer for PRs to merge than if I could actually pay attention to github notifications.
In addition to waiting until LGTM, we want the bot to not notify the approver at all, but post the approver suggestion to the author and/or reviewer. Something like (thanks to Tim for text): "This needs approval, we recommend thockin - assign it to him with a comment when you're ready"
bgrant0607
on 15 Apr 2017
PR can be found here: https://github.com/kubernetes/test-infra/pull/2538
The bot will not notify the approver. Instead, a suggestion will be given.
crimsonfaith91
on 20 Apr 2017
Closing this issue as done, see any recent PR in kubernetes/kubernetes for examples of what @k8s-merge-robot now says
spiffxp
on 30 Jun 2017
Related issues
zacharysarah
路
3Comments
spiffxp
路
3Comments
xiangpengzhao
路
3Comments
MrHohn
路
4Comments
Aisuko
路
3Comments
Most helpful comment
@spxtr @apelisse Yes, I complained shortly after the approval mechanism was turned on, and so did @thockin.
A direct result of the current approach is that I blackhole all github notifications, which I'm sure makes it take longer for PRs to merge than if I could actually pay attention to github notifications.
In addition to waiting until LGTM, we want the bot to not notify the approver at all, but post the approver suggestion to the author and/or reviewer. Something like (thanks to Tim for text): "This needs approval, we recommend thockin - assign it to him with a comment when you're ready"