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

add a new stale action #17404

Merged
merged 4 commits into from Apr 16, 2021
Merged

add a new stale action #17404

merged 4 commits into from Apr 16, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 29, 2021

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
Copy link
Contributor Author

flamisz commented Mar 29, 2021

let me know if you happy with this @tsteur

@tsteur
Copy link
Member

tsteur commented Mar 30, 2021

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
Copy link
Member

sgiehl commented Mar 30, 2021

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
Copy link
Contributor Author

flamisz commented Mar 30, 2021

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 flamisz self-assigned this Apr 6, 2021
@flamisz flamisz added the Needs Review PRs that need a code review label Apr 6, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Apr 6, 2021

@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
Copy link
Member

tsteur commented Apr 6, 2021

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
Copy link
Contributor Author

flamisz commented Apr 7, 2021

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
Copy link
Contributor Author

flamisz commented Apr 7, 2021

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
Copy link
Member

tsteur commented Apr 7, 2021

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
Copy link
Contributor Author

flamisz commented Apr 7, 2021

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
Copy link
Contributor Author

flamisz commented Apr 7, 2021

@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
Copy link
Contributor Author

flamisz commented Apr 9, 2021

@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
Copy link
Member

tsteur commented Apr 11, 2021

@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
Copy link
Contributor Author

flamisz commented Apr 13, 2021

@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
Copy link
Member

tsteur commented Apr 13, 2021

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

@tsteur
Copy link
Member

tsteur commented Apr 13, 2021

we can then always change later again

@flamisz
Copy link
Contributor Author

flamisz commented Apr 16, 2021

@tsteur I added a new action. Do you think we should give it a try like this, but still just dry-run? I week maybe, and we can go live the week after.

@tsteur
Copy link
Member

tsteur commented Apr 16, 2021

sure sounds good @flamisz

@flamisz flamisz changed the title turn on stale actions add a new stale action Apr 16, 2021
@flamisz flamisz added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. and removed Needs Review PRs that need a code review labels Apr 16, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Apr 16, 2021

I test with the new settings for one more week

@flamisz flamisz merged commit be41345 into 4.x-dev Apr 16, 2021
@flamisz flamisz deleted the turn-on-stale-pr-actions branch May 12, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants