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

Renaming issues/PRs incorrectly changes awaiting change review and awaiting merge to awaiting review #556

Closed
arhadthedev opened this issue May 6, 2023 · 3 comments · Fixed by #557
Labels

Comments

@arhadthedev
Copy link
Member

While changing bpo- to gh- I noticed that Bedevere makes forbidden transitions in response:

According to README, awaiting review has no incoming conditions:

flowchart TD
    A([Published PR]):::creator
    A_draft([New Draft PR]):::creator
    A_draft -- publish draft by contributor --> A:::creator
    A -- by contributor --> B[Awaiting review]:::anyone
    A -- by core dev --> C[Awaiting core review]:::coredev
    B & C -- new review by\nanother contributor --> C
    C & B & E  -- new core review\nrequests changes --> D[Awaiting changes]:::creator
    D -- changes by contributor --> E[Awaiting change review]:::coredev
    C & E & B -- new core review\napproves ---> F[Awaiting merge]:::coredev
    G[When a review is dismissed\nthe highest remaining state is restored\nbased on the remaining reviews]
classDef creator stroke:#CC0;
classDef anyone stroke:#00C;
classDef coredev stroke:#0C0;
classDef triager stroke:#C0C;
linkStyle 0,1,7 stroke:#CC0,color:auto;
linkStyle 2,3 stroke:#00C,color:auto;
linkStyle 4,5,6,8,9,10 stroke:#0C0,color:auto;
Loading

I suspect the bug is here, in a handler of PR/issue modifications introduced in gh-555:

bedevere/bedevere/stage.py

Lines 127 to 131 in 0f350ce

if pull_request.get("draft"):
await _remove_stage_labels(gh, issue)
else:
blocked_on = Blocker.core_review if await util.is_core_dev(gh, username) else Blocker.review
await stage(gh, issue, blocked_on)

We need to check whether assignment of Blocker.core_review or Blocker.review is allowed before executing the else branch.

Ideally (for some distant future) we need to move tag management into a separate .py file that reads existing tags, checks if a new one is applicable, and removes the old ones. However, I have no free time to do it currently (because it requires massive modification of both reactors and their tests).

cc @itamaro

@arhadthedev arhadthedev added the bug label May 6, 2023
@itamaro
Copy link
Contributor

itamaro commented May 6, 2023

oops, you're right, this handler is doing way more than it was intended!
that handler should respond only to PR edits that change the PR from draft to published or from published to draft.

@arhadthedev
Copy link
Member Author

@itamaro Could you submit a PR, please?

itamaro added a commit to itamaro/bedevere that referenced this issue May 6, 2023
pythongh-555 introduced the PR edited handler to handle draft PRs.
The implementation was over-reaching, and triggered on any edited events (including renaming PR).
This PR fixes that by removing the PR-edited handler and replacing it with
two more specific handlers that more narrowly match the original intent,
namely, respond to changes in the draft state of the PR (publish / unpublish).

Fixes pythongh-556
@itamaro
Copy link
Contributor

itamaro commented May 6, 2023

@itamaro Could you submit a PR, please?

of course

itamaro added a commit to itamaro/bedevere that referenced this issue May 6, 2023
JelleZijlstra pushed a commit that referenced this issue May 10, 2023
gh-555 introduced the PR edited handler to handle draft PRs.
The implementation was over-reaching, and triggered on any edited events (including renaming PR).
This PR fixes that by removing the PR-edited handler and replacing it with
two more specific handlers that more narrowly match the original intent,
namely, respond to changes in the draft state of the PR (publish / unpublish).

Fixes gh-556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants