Only comment about new commits if the PR is still open.

[Mariatta]

This should fix the issue where the "awaiting core review" label gets added after PR has ben merged. Example: python/cpython#109082

[codecov]

Codecov Report

Merging #583 (1517955) into main (249bab5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #583   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         2088      2098   +10     
  Branches       237       239    +2     
=========================================
+ Hits          2088      2098   +10     

Flag	Coverage Δ
Python_3.10.12	?
Python_3.10.13	100.00% <100.00%> (?)
Python_3.11.4	?
Python_3.11.5	100.00% <100.00%> (?)
Python_3.8.17	?
Python_3.8.18	100.00% <100.00%> (?)
Python_3.9.17	?
Python_3.9.18	100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
bedevere/stage.py	100.00% <100.00%> (ø)
tests/test_stage.py	100.00% <100.00%> (ø)

[AA-Turner]

I wonder if the entire new_commit_pushed should be removed? It doesn't seem to work even in the cases that it should -- Paul approved GH-109130, putting it into awaiting merge, and I've pushed several commits since, which haven't triggered Bedevere to change the state.

A

[AlexWaygood]

I agree with Adam. I don't think I've ever seen Bedevere correctly post this comment -- the first time I saw this comment being posted was when Bedevere started posting it incorrectly in the last few weeks.

And even if this logic could be fully fixed, I'm not sure it's worth fixing it. I can't think of a situation in which I'd find this comment from Bedevere useful, since I already get a notification on GitHub if a new commit has been pushed on a PR I've already reviewed. I feel like the extra comment from Bedevere would just be annoying and lead to me having to delete two emails from my inbox, rather than just one.

I'd be in favour of just removing this logic rather than trying to fix it.

[Mariatta]

Hmm that's interesting, looking at the PR mentioned by @AA-Turner , it looks like the new commits triggered the synchronize event instead.

I'll investigate this some more.

I'm ok with removing bedevere's comment, but I think we still need to logic to reapply the [awaiting core review] label.

[AA-Turner]

I think we still need to logic to reapply the [awaiting core review] label

I'd push back slightly in line with Alex's thoughts and ask why here: Bedevere seems to have only started applying the label three weeks ago, so from the time CPython moved to GitHub to ~mid August 2023 the logic hasn't been running and (to my knowledge) no one has complained about it.

A

[AlexWaygood]

I think we still need to logic to reapply the [awaiting core review] label.

I don't have a strong opinion on this question, but it feels strange to me that merging in main (for example) would cause Bedevere to remove the "awaiting merge" label. Personally, I think it would be fine for the "awaiting merge" to stay there unless a core dev requests changes, adds "DO-NOT-MERGE", or dismisses their previous approving review

[pradyunsg]

it looks like the new commits triggered the synchronize event instead.

Indeed.

Currently, we have @router.register("push") which is triggered for (docs):

when there is a push to a repository branch

This means it'll trigger for pushes to main on python/cpython (or any other branch in python/cpython) but it won't trigger when PRs are filed from a branch that doesn't live in python/cpython.

We should change this to use the pull_request event (docs):

@router.register("pull_request", action="opened")
@router.register("pull_request", action="reopened")
@router.register("pull_request", action="synchronize")

IIUC, those are all the events that we'd want to listen to for determining whether the label needs to be updated.

[ezio-melotti]

Can't this be replaced by an if 'awaiting merge' in utils.labels(pr):, and possibly combined with the previous if?