Right, good point, the script actually already has the logic to handle PR reviews. But the workflow run skips all steps when a PR review is submitted (see https://github.com/sagemath/sage/actions/runs/17460557035/job/49584072319), which suggests that the PR review event is in ' SYNC_LABELS_IGNORE_EVENTS'. Someone of the github admins needs to confirm/change that.
> if: vars.SYNC_LABELS_ACTIVE == 'yes' && (! vars.SYNC_LABELS_IGNORE_EVENTS || ! contains(fromJSON(vars.SYNC_LABELS_IGNORE_EVENTS), github.event.action)) On Thursday, September 4, 2025 at 1:35:36 AM UTC+8 Vincent Macri wrote: > On 2025-09-03 6:01 a.m., 'tobia...@gmx.de' via sage-devel wrote: > > I second that "needs review" and "needs work" are mostly obsolete and > > should be better handled by native github tools. Those labels were > > introduced when migrating to github to not change the workflow too > > much from the good old trac days. Besides the automatic transition > > "needs work" -> "needs review" after just merging develop, I also find > > it somewhat annoying to manually set the "needs work" label after just > > submitting a "request changes" review. > > > > Perhaps as a transition period, those two labels could just reflect > > the github status. So: > > - There is a review with "Request changes" -> "needs work" > > - The PR is not a draft and not in "needs work" or "positive review" > > -> "needs review" > > > > If someone gives a "request changes" review and the PR author > > addresses the changes, one can re-request a new review (last point > > in > https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review#requesting-reviews-from-collaborators-and-organization-members), > > > > which then removes the "request changes" status and thereby would > > automatically transition the PR into "needs review". The button is not > > there for new contributors, but those can either ping the original > > reviewer for a new review (or in the rare case that nobody is reacting > > to such a call, ping the core team to have the review invalidated.) > > > > The positive review label still needs to be there to signal Volker > > that the PR is ready to be merged. (The github way would be to replace > > the buildbots by the merge queue feature) > > In theory this behaviour is already implemented in > https://github.com/sagemath/sage/blob/develop/.github/sync_labels.py. > However it doesn't seem to work (I just tried on > https://github.com/sagemath/sage/pull/40634 and nothing happened, I'm > going to avoid setting the label manually in case it actually does work > just with a delay). So I guess that needs to be fixed. I'm not sure if > the sync_labels.py script is broken or if the GitHub Action that runs > the script just isn't triggering > ( > https://github.com/sagemath/sage/blob/develop/.github/workflows/sync_labels.yml > ). > > Something to note from scanning through the code, in order for it to > determine if someone has sufficient permissions to set the label to > positive review, one must set their membership in the SageMath > organization to public (mine is set to public and it still didn't work). > So in order for this to work consistently we may want to encourage > people to set their membership in the SageMath GitHub organization to > public. > > -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscr...@googlegroups.com. To view this discussion visit https://groups.google.com/d/msgid/sage-devel/b3db8b2b-ba3d-4fca-9934-defb252866c4n%40googlegroups.com.
