backlog
backlog copied to clipboard
pullreminders
As a user, I would like to filter approved PRs out of team reminders (with or without using review requests)
Depending on project, sometimes approval doesn't necessarily mean 'merge now'. Often for some legacy projects, the merging is left up to the author. Sometimes there's coordination, etc to do. So that sometimes leaves approved PRs left 'open' too long, which causes them to get added to the reminder list.
Having a way to filter these out would help reduce the clutter of the reminders and distill them down to PRs that actually need attention.
@goodspark There's already a way to do this for DM reminders, look under "More settings". For team reminders, have you tried the new "Needed reviews" setting for team reminders?
Let me know if either of these addresses your needed. Another recommendation is to use a label like "noreminders" for PRs that are going to hang around for whatever reason, and have that label excluded in your reminder settings.
But that requires me to turn on the 'require review requests' option, right? Which isn't always the case.
@goodspark Correct, for team reminders that setting is currently only available with the review request setting turned on. Out of curiousity, what's your code review process such that sometimes review requests are used and sometimes they are not?
It really depends on the team, the project, etc. There's no consistent process and we don't feel the need to enforce any. As long as teams are achieving their desired velocity, review requests is largely a trivial detail.
However, for some larger code bases, sometimes we use review requests so there's a somewhat more targeted set of people.
My personal opinion is to never use review requests, as I want every single developer to be familiar with everything we work on such that anyone can hop on and provide insightful and valuable reviews. Obviously that's quite an ideal and, if even possible, will take some time and training.
@goodspark Gotcha. We can look into adding an approval filter for team reminders not using review requests... we're also going to be rolling out a new tool to assist with code review assignment that may be helpful to you and also alleviate this issue at the same time. In the meantime I'll tag this and add to the backlog.
👍 I'd like this feature as well.
Our team does use review_requests however, even after a PR has been approved I still get a reminder about it. E.g.
camallen requested review from zwolf and marten on 27 Nov 2018 marten approved these changes on 14 Dec 2018
Our use case for the pull reminders system is to get attention on the PR that need a review. I feel like the approved PRs should be dealt with via your 'Remind Authors' functionality instead.
@goodspark @camallen An interim solution would be to tag these PRs with a label like noreminders, then exclude that label in your reminder settings. Let me know what you think of this idea, specifically how painful it is to take this additional step. It'll be helpful in assessing the prioritization of this issue.
Yeah that's reasonable for me. Some of our projects already have a label like ready-but-waiting for when something is approved but needs coordination/timing for deployment. It kind of fits this too.
For us not ideal tbh, I can see that flow working but it's another step that people will forget that increases the false positive rate of the alerts pull reminders will generate for us.
It doesn't look like github / app has a way to add the label on approval and I'd prefer not to run a service that does this. If I could automate the label creation then yep this flow works.
I understand the need to tailor solutions to what your users need and that's ok, I really appreciate this service.
A possible solution would be to set a notification_rules boolean attribute to ignore approved PR's and when generating the notification, you reflect on this setting to skip any that that have approved review states, e.g.
- Get the pull request reviews - http://octokit.github.io/octokit.rb/Octokit/Client/Reviews.html#pull_request_reviews-instance_method
- See if any have approval state - https://developer.github.com/v3/pulls/reviews/#list-reviews-on-a-pull-request
Thanks for considering, I'll see if we can't use the label flow to help as it stands.
@camallen Yeah, the issue for us is diminishing returns on adding more configuration options. Mostly we want to avoid making setup complicated by offering too many options.
It doesn't look like github / app has a way to add the label on approval and I'd prefer not to run a service that does this. If I could automate the label creation then yep this flow works.
This is something you could easily automate with GitHub Actions. We've created Actions for other customers with unique situations, for example this one. If you're interested, shoot me an email at [email protected] and I can help you get set up with this. And in the meantime sign your org up for the beta and sign the beta agreement on behalf of your org.
I completely understand re the addition of options, etc.
I appreciate the reminder and offer for help with Github Actions I will look into your examples and try it out. Thanks again :)
@camallen Sounds good. And just to be clear, I'm offering to write the code for the Action for you. It's something we'd be happy to do since we could provide it to other customers as well, i.e. @goodspark.
(Let me know if you'd like us to – just wasnt sure if you'd be interested in the idea)
@camallen Yep, without charge. Please email me at [email protected] though – there's one other thing I need to assist you with.
To be honest, I'd rather have the service itself do this somehow. Either by an option or some other integration that's managed by the service itself as opposed to ad-hoc/bespoke work done on a repo-basis. I'm a bit iffy about having to add Actions to different repositories just for this. If it were part of an existing process a project already had, it'd make more sense to integrate.
As it stands, there's a reasonable work-around (albeit a slightly manual process that can easily be automated as you suggested). So my needs are met. If you do implement this, it'll be a nice convenience.
@goodspark Yeah, that makes sense. We'll continue considering this... it's not a lot of work but we're just concerned about bloating the tool.
@goodspark
In case you might be interested in giving it a try (no pressure), here's the GitHub Action I just created for @camallen to try out: https://github.com/pullreminders/label-when-approved-action. It only takes a minute to set up on a repo and the functionality is very straightforward. The example below is with the settings set to add a label after 2 approvals:
Would it be possible to make the # of approvals before hiding configurable?
Context: For several of our teams, our flow involves getting 2 approvals, so we don't want to show PRs that have 2 or more
Would it be possible to make the # of approvals before hiding configurable?
Context: For several of our teams, our flow involves getting 2 approvals, so we don't want to show PRs that have 2 or more
Seconding this request!
Use case: at $work my team is using our Pull Reminder integration to alert us to PRs which need to be reviewed, but another team is responsible for merging them, so once they have been reviewed, my team no longer considers them actionable. So these PRs require approvals but without anyone specifying who the reviewers must be (thus we're not using the review_requests option). We want to be able to consider a PR that has gotten (say) 2+ approvals not-stale, since after that point it becomes the territory of a different team. For now we are using a label approach, however I worry about how error-prone our system of manually adding and removing a tag will be when a PR is approved but then additional work is committed, thus requiring additional approvals...
Thanks!
