@flamisz opened this Pull Request on March 29th 2021 Contributor

Description:

refs #17318 #17344

I checked the logs of the dry runs and actually did not find any when actually it would comment on any or. The current setup checks only PRs (not issues) and only PRs were created after 2021-03-01.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on March 29th 2021 Contributor

let me know if you happy with this @tsteur

@tsteur commented on March 30th 2021 Member

thanks @flamisz that's great. Do you have maybe a link to a summary what actions it would do?

Can we maybe tweak This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. and add something like "If you don't want this PR to be closed automatically in ... days then you need to assign the label 'Do not close'."

@mattab @sgiehl @diosmosis @Findus23 be great to read over the config as well. There are two different actions one for Not Review in 7 days and one for stale PRs where there was no activity for a certain amount of time.

@sgiehl commented on March 30th 2021 Member

Does that really work as expected. The action actually produces this warning: Warning: Reached max number of operations to process. Exiting.
Sounds like it would go through all issues and exit as there are too many?

@flamisz commented on March 30th 2021 Contributor

Sounds like it would go through all issues and exit as there are too many?

There is a limit on operations, you can read about it here: https://github.com/actions/stale#operations-per-run

@flamisz commented on April 6th 2021 Contributor

@tsteur I modified the stale message. Please check if it is ok now.

Do you have maybe a link to a summary what actions it would do?

What do you mean? A summary about dry runs, so we know what would have happened?

@tsteur commented on April 6th 2021 Member

What do you mean? A summary about dry runs, so we know what would have happened?

@flamisz yes that be great. Just so we know it would do the right thing and that it'll work and won't the wrong PRs etc

@flamisz commented on April 7th 2021 Contributor

Short summary for the follow up reviews action:

Yesterday it found this PR #17394 and wanted to stale it, which means add a Stale label and a comment message:
"This issue is in "needs review" but there has been no activity for 7 days. ping @tsteur @sgiehl @diosmosis @flamisz"

I checked the result for a run couple of days ago, and it didn't stale it, because it was updated less than 7 days ago (that time).

The same happened with this one #17387.

Skipped all the PR without the required label (Needs review).

It goes through the issues/PRs by the last updated date, descending. It runs 30 operations per run (by default). I'm not sure what is one operation when it's in production mode, so we will see how it works, how many PRs will be checked per run. It runs once daily.

@flamisz commented on April 7th 2021 Contributor

Handle inactive PRs

A couple of pr (like #17333 ) got stale during yesterday's run.

No PR has been closed, because we check PRs only from 1st March and the days before close is set to 42.

@tsteur commented on April 7th 2021 Member

Thanks @flamisz

Any more thoughts @mattab @sgiehl @diosmosis @Findus23 ? We'd otherwise activate it and would just need to keep an eye on it that we don't accidentally close a false PR.

Regarding closing stale PRs. I wonder if we'd need to change to something like this if that's possible:

  • If the contributor was last active (like pushed a commit) then we wouldn't want to close the PR automatically maybe because in this case we are late (we are the problem) and it's not the contributor we're waiting on. In that case we'd want to only comment again and ping us to review and follow up
  • Vs if we commented last and the contributor hasn't reacted in a while then we'd want to close the PR.

Not sure if that makes sense or is clear what I'm thinking of? Fingers crossed it's possible to configure this.

@flamisz commented on April 7th 2021 Contributor

If the contributor was last active (like pushed a commit) then we wouldn't want to close the PR automatically maybe because in this case we are late (we are the problem) and it's not the contributor we're waiting on. In that case we'd want to only comment again and ping us to review and follow up
Vs if we commented last and the contributor hasn't reacted in a while then we'd want to close the PR.

Unfortunately, I don't think it's that flexible. I take a look, but I doubt it. The best we can do is checking the labels probably, but that has to be handled by us, and we forget it, etc.
I double-check what are the options we have.

@flamisz commented on April 7th 2021 Contributor

@tsteur I checked the action, what is the best we can do, regarding to checking who was the last who did something with the PR.
Like I thought, there is no setting for that. The best option we can have if you use the Waiting for user feedback label whenever we comment on a PR and we really wait for the user.
This way the action can test if the label is there, and only stale/close it if that's the case.

Do you want me to add this to the yaml file?

@flamisz commented on April 9th 2021 Contributor

@tsteur we were talking about to do not close the PR automatically only add a comment, but this is not possible with this action. It closes the PR when adds the close comment and optional label.

@tsteur commented on April 11th 2021 Member

@flamisz don't we also just add a comment with the other action after 7 days? Was hoping it could look similar to https://github.com/matomo-org/matomo/pull/17404/files#diff-487d40658fdc47419d39d147a502671a427a1df852c41b5555b382ae515111d7R12-R20 or maybe I forgot about something?

@flamisz commented on April 13th 2021 Contributor

@flamisz don't we also just add a comment with the other action after 7 days?

@tsteur sure we can do that, what we can't do is add a comment after 7 days, and then a new one after 42 days, because the 2nd comment would close the PR. If we want to make 2 warnings (one at day 7, and one at day 42) we need a new action.

What do you think we should do here? We can add a comment with mentions to us at day 7, and than close it or not close it at day 42? We can add a new comment with mentions on that day too, and we can reopen if it was a mistake.

@tsteur commented on April 13th 2021 Member

We could use a new action and we wouldn't want to close it automatically. Only a comment be great 👍

@tsteur commented on April 13th 2021 Member

we can then always change later again

Powered by GitHub Issue Mirror