[MergeQueues] Delete branch if PR is merged #193
Conversation
src/github/client.py
Outdated
| git_ref = repo.get_git_ref(ref) | ||
| git_ref.delete() | ||
| logger.info(f"Branch '{branch_name}' deleted successfully.") | ||
| except Exception as e: |
There was a problem hiding this comment.
i would imagine this is a GithubException where from github.GithubException import GithubException
src/github/models/pull_request.py
Outdated
| @@ -219,3 +219,6 @@ def labels(self) -> List[Label]: | |||
|
|
|||
| def base_ref_name(self) -> str: | |||
| return self._raw["baseRefName"] | |||
|
|
|||
| def head_ref_name(self) -> str: | |||
| return self._raw["headRefName"] | |||
There was a problem hiding this comment.
hmm I'm not seeing a corresponding change in FullPullRequest for this field. You need to specify which fields to query for in the graphql query to access it.
There was a problem hiding this comment.
yeah i just added it into src/github/graphql/fragments/FullPullRequest.py
src/github/webhook.py
Outdated
| @@ -19,6 +19,7 @@ def _handle_pull_request_webhook(payload: dict) -> HttpResponse: | |||
| # a label change will trigger this webhook, so it may trigger automerge | |||
| github_logic.maybe_automerge_pull_request(pull_request) | |||
| github_logic.maybe_add_automerge_warning_comment(pull_request) | |||
| github_logic.maybe_delete_branch_if_merged(pull_request) | |||
There was a problem hiding this comment.
nit: this is probably better placed right after maybe_automerge_pull_request just to highlight the causal effect. It's good that it's not in maybe_automerge because it the deletion fails for whatever reason, it can try again in another webhook.
| @@ -18,6 +18,8 @@ def _handle_pull_request_webhook(payload: dict) -> HttpResponse: | |||
| pull_request = graphql_client.get_pull_request(pull_request_id) | |||
| # a label change will trigger this webhook, so it may trigger automerge | |||
| github_logic.maybe_automerge_pull_request(pull_request) | |||
| if payload["action"] == "closed": | |||
There was a problem hiding this comment.
Put this check in so we don't delete branches on arbitrary PR updates. The only time we can delete branches is if a PR is closed and merged which can only happen once per Pull Request.
There was a problem hiding this comment.
I suppose someone could create a branch with a new name, then re-open the same pull request and merge it which would result in a deletion but this seems like an unlikely situation so 👍
There was a problem hiding this comment.
code looks good. Can you add new test cases in test/github/test_webhook.py and test/github/test_logic.py? You may need to create a new field in test/impl/builders/pull_request_builder.py which is the mock pull request builder object to supply pull request objects to tests
| @@ -18,6 +18,8 @@ def _handle_pull_request_webhook(payload: dict) -> HttpResponse: | |||
| pull_request = graphql_client.get_pull_request(pull_request_id) | |||
| # a label change will trigger this webhook, so it may trigger automerge | |||
| github_logic.maybe_automerge_pull_request(pull_request) | |||
| if payload["action"] == "closed": | |||
There was a problem hiding this comment.
I suppose someone could create a branch with a new name, then re-open the same pull request and merge it which would result in a deletion but this seems like an unlikely situation so 👍
prebeta
force-pushed
the
tonyliang-delete-branch-if-merged
branch
from
0f88084 to
59ee004
Compare
prebeta
force-pushed
the
tonyliang-delete-branch-if-merged
branch
from
f88c066 to
f71ee79
Compare
prebeta
added
the
merge after tests and approval
|
|
This reverts commit 557ad85.
This reverts commit 557ad85. Pull Request synchronized with [Asana task](https://app.asana.com/0/0/1207719852093155)
Summary
Github's delete branch logic doesn't work if SGTM added the pull request to the merge queue. This will make SGTM delete branches if they haven't been cleaned up yet.
Asana tasks:
https://app.asana.com/0/1125126232217429/1207661000934486/f
Relevant deployment:
CC: @suzyng83209 @prebeta @vn6 @michael-huang87
Test Plan
Run on test sgtm deployment
Risks
Pull Request: #193
Pull Request synchronized with Asana task