Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Making sure we review all pull requests #17318

Closed
tsteur opened this issue Mar 8, 2021 · 6 comments · Fixed by #17354
Closed

Making sure we review all pull requests #17318

tsteur opened this issue Mar 8, 2021 · 6 comments · Fixed by #17354
Assignees
@flamisz
Labels
Better processes Indicates an issue is about improving how we work. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Milestone
4.4.0

Comments

@tsteur
Copy link
Member

tsteur commented Mar 8, 2021 •
edited

Hi there,

currently we have a few PRs that are waiting for reviews for > 10 days like #17264 and #17266

I wonder if we could have a github action or some other reminder that pings all of us if there was an issue with "needs review" and there was no comment or review for 7 days? Or maybe even just 5 days? Of course it's not always that trivial since we might be waiting for a change from the author etc but maybe also #17317 would simplify the logic a bit.

We could maybe start with a simple reminder once there has been no comment from us for say 3 or 5 business days (or 5-7 regular days) and then see how this works over time and could then make adjustments to it if needed. The added comment could say something like This issue is in "needs review" but there has been no comment for X days. @tsteur ... we would then ping all of us maybe.

Or maybe the issue was simply that there was no milestone assigned in which case we'd want to make sure a Needs Review PR always also has a milestone?

Or are there better/other ways maybe? Any thoughts appreciated @diosmosis @Findus23 @flamisz @sgiehl @mattab

Maybe also #17317 would make things better and a new github action wouldn't be needed. Not sure it's even possible to solve with a github action.

In #17344 there's also an issue for automatically following up when we're waiting for a PR author for too long.
@tsteur tsteur added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Mar 8, 2021
@tsteur tsteur added this to the 4.3.0 milestone Mar 8, 2021
@diosmosis
Copy link
Member

Lately the problem I've had is just not having time, w/ deploys and L3 issues. That's mostly died down though, so I should be able to get back to reviewing regularly. It could be prioritized higher I suppose (ie, pr reviews before investigating L3 issues).

@sgiehl
Copy link
Member

sgiehl commented Mar 8, 2021

@tsteur such stuff should be possible with github actions for sure. Maybe there are already some matching actions like: https://github.com/marketplace/actions/pull-request-reviewer-reminder-action

@flamisz
Copy link
Contributor

flamisz commented Mar 9, 2021 •
edited

This action is promising:

https://github.com/actions/stale
https://github.com/marketplace/actions/close-stale-issues

@tsteur tsteur mentioned this issue Mar 14, 2021
Open
@tsteur
Copy link
Member Author

tsteur commented Mar 14, 2021

@sgiehl regarding https://github.com/marketplace/actions/pull-request-reviewer-reminder-action

do you know from when they start the timer to measure the turnaround time? Maybe it won't work for our case regarding the labels etc?

@flamisz for closing issues automatically created #17344

@sgiehl
Copy link
Member

sgiehl commented Mar 15, 2021

@tsteur yes, that won't work depending on labels. guess the time is calculated based on the creation time.
Maybe there are some other actions that would be able to handle what we need. Guess someone would need to look a bit deeper into it. I actually only did a quick search yet. If there isn't anything we could also make a custom action to handle that.

@tsteur tsteur added the Better processes Indicates an issue is about improving how we work. label Mar 15, 2021
@tsteur
Copy link
Member Author

tsteur commented Mar 15, 2021

I think just like in #17344 we could potentially use https://github.com/marketplace/actions/close-stale-issues as well and only apply the rule when only-labels = Needs Review and there has been no activity for say 7 days.

@flamisz flamisz self-assigned this Mar 17, 2021
@flamisz flamisz mentioned this issue Mar 17, 2021
Merged
10 tasks
@tsteur tsteur reopened this Mar 18, 2021
@flamisz flamisz mentioned this issue Mar 29, 2021
Merged
10 tasks
@mattab mattab modified the milestones: 4.3.0, 4.4.0 May 26, 2021
@tsteur tsteur closed this as completed Jun 16, 2021
@mattab mattab added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Jul 26, 2021
@mattab mattab changed the title Making sure we follow up with reviews Making sure we review all pull requests Jul 26, 2021
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@flamisz flamisz

Labels
Better processes Indicates an issue is about improving how we work. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Milestone
4.4.0
Development

Successfully merging a pull request may close this issue.

workflows for handling inactive prs matomo-org/matomo
5 participants
@diosmosis @tsteur @mattab @sgiehl @flamisz