feat(gh-actions): exempt "needs review" PRs from labelled stale

[Swiftb0y]

IMO PRs labelled "stale" mean that the contributor has abandoned the PR.
For many PRs that are not abandoned but simply not being reviewed because
of our reviewer shortage, this is not true. In this case, the PR should be
labelled "needs review" and the "stale" label should not be applied.

[Swiftb0y]

does anybody want/care to comment on this proposal?

[daschuer]

Can we remove that entirely?
The GitHub GUI has been improved since we have introduced that feature.

Now we have the draft state and the review request.

Alternative we may use another "whining" Tag that is neutral adresseing both, reviews and contributors.

[Swiftb0y]

What do you refer to as "that"? The github bot marking PRs stale? IMO github does not really have a good replacement. You could filter by open PRs that the last time they were updated, but thats cumbersome. Still there is not really a way for reviewers to differentiate whether a PR is actually waiting for a review or already received one and is not being updated...

Alternative we may use another "whining" Tag that is neutral adresseing both, reviews and contributors.

I'd like to have exactly the opposite. I want to be able to tell at first glance whether a PR is currently waiting for a reviewer or the PR author to take action. PRs stall exactly because the interaction fizzles out and nobody knows what to do next.

[daschuer]

I'd like to have exactly the opposite. I want to be able to tell at first glance whether a PR is currently waiting for a reviewer or the PR author to take action.

That's a good idea. But that has nothing to do with a 90 days delay. In the VCPK upstream repro they have manual info tags for that:

GitHub has already the "Draft" "Changes requested" "Approved" states, can we utilize them better?

VCPKG has solved this by a number of additional state tags:
https://github.com/microsoft/vcpkg/pulls
"requires:review" "requires:authors-responds" "requires:vcpkg-team-review" "requires:discussion" "requires:more-information" "requires:testing"

So if you lock at a PR you like to review later or anyone else need to review, just add the label.

How about adopt that?

[Swiftb0y]

So if you lock at a PR you like to review later or anyone else need to review, just add the label.
How about adopt that?

Yes, that makes sense, but in that case the bot shouldn't touch the PR IMO, which is exactly what this PR does.

[daschuer]

I did not realized that you have already introduced a the "needs review" tag. That is nice, but we may consider to extend it towards the vcpkg labels.

For me the notification of of putting the staled label, was always a kind of ping that a PR is rotting and that we should consider to look at it and not treating a contributor unfair. If this is now bypassed.
This has been introduced, after a discussion to close PRs automatically after 90 days, to clean up our big PR backlog.

I am not sure if the stale label was ever very helpful. But after committing this PR it is even more useless. That's why I have proposed to remove that entirely. Alternately we may ping reviewers if the "needs review" is set and the author if another tag is set.

What do you think.

[Swiftb0y]

I did not realized that you have already introduced a the "needs review" tag.

I think it was a default tag from github. I didn't create it, but since it was already there, I went ahead and used it.

For me the notification of of putting the staled label, was always a kind of ping that a PR is rotting and that we should consider to look at it and not treating a contributor unfair. If this is now bypassed.

I understand that usecase, but its kind of a hack imo. Getting pinged for stale PRs does not seem like a good strategy for handling them. For the reviewers perspective, it would be much better to have a "needs review" tag so reviewers can filter explicitly for such PRs when they have the free time to take on reviewership of a PR (at least thats what I feel like). If you want to prioritize based on what PRs have not been addressed the longest, you can trivially filter by "needs review" and then sort by the last-interaction-date.

Alternately we may ping reviewers if the "needs review" is set and the author if another tag is set.

Yes, that sounds along the lines of what I intended. I figured the "stale" label was the counterpart for "needs review" (or what you meant with another tag).
The issue then is that if the reviewer interacts with the PR author, but the PR author never responds the fact that the reviewer has gone missing is likely flying under the radar. In that case, the bot is still useful to take care of going through the PRs and labeling it.

Since "stale" and "needs review" are supposed to be mutually exclusive, the change introduced by this PR is needed so the bot respects that mutual exclusion property.

[daschuer]

Getting pinged for stale PRs does not seem like a good strategy for handling them.

That is the essence!
We can build on that. Thank you for explaining.

[daschuer]

Than we can check id we get pinged for "stale" if it is correct or it should be changed to "needs review"