From e6f55f177609848ec95940be45fd28ec62451a58 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:10:13 -0400 Subject: [PATCH 1/6] ref(notifications): add message for merging CHANGES - replace BaseException with Exception. BaseException is improper. - reduce some duplicated logic in mergeability for set_status --- kodiak/evaluation.py | 17 +++++++++++------ kodiak/pull_request.py | 13 +++++-------- kodiak/queries.py | 2 +- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/kodiak/evaluation.py b/kodiak/evaluation.py index 4d8271aa1..0e484531e 100644 --- a/kodiak/evaluation.py +++ b/kodiak/evaluation.py @@ -35,7 +35,7 @@ async def valid_merge_methods(cfg: config.V1, repo: RepoInfo) -> bool: raise TypeError("Unknown value") -class Queueable(BaseException): +class Queueable(Exception): pass @@ -51,25 +51,30 @@ class WaitingForChecks(Queueable): pass -class NotQueueable(BaseException): +class NotQueueable(Exception): pass -class MissingAppID(BaseException): +class MissingAppID(Exception): """ Application app_id doesn't match configuration We do _not_ want to display this message to users as it could clobber another instance of kodiak. """ + def __str__(self) -> str: + return "missing Github app id" -class BranchMerged(BaseException): + +class BranchMerged(Exception): """branch has already been merged""" +class MergeConflict(Exception): + """Merge conflict in PR.""" + def __str__(self) -> str: + return "merge conflict" -class MergeConflict(BaseException): - """Merge conflict in the PR.""" def review_status(reviews: typing.List[PRReview]) -> PRReviewState: diff --git a/kodiak/pull_request.py b/kodiak/pull_request.py index d9011ba1b..51fea8f17 100644 --- a/kodiak/pull_request.py +++ b/kodiak/pull_request.py @@ -150,16 +150,13 @@ async def mergeability( valid_merge_methods=self.event.valid_merge_methods, ) self.log.info("okay") + await self.set_status(summary="⛴ ready to merge") return MergeabilityResponse.OK, self.event - except MissingAppID: - return MergeabilityResponse.NOT_MERGEABLE, self.event - except NotQueueable as e: + except (NotQueueable, MissingAppID, MergeConflict) as e: await self.set_status(summary="🛑 cannot merge", detail=str(e)) - return MergeabilityResponse.NOT_MERGEABLE, self.event - except MergeConflict: - await self.set_status(summary="🛑 cannot merge", detail="merge conflict") - if self.event.config.merge.notify_on_conflict: - await self.notify_pr_creator() + if isinstance(e, MergeConflict): + if self.event.config.merge.notify_on_conflict: + await self.notify_pr_creator() return MergeabilityResponse.NOT_MERGEABLE, self.event except MissingGithubMergeabilityState: self.log.info("missing mergeability state, need refresh") diff --git a/kodiak/queries.py b/kodiak/queries.py index c04974f3a..014bca1c8 100644 --- a/kodiak/queries.py +++ b/kodiak/queries.py @@ -44,7 +44,7 @@ class GraphQLResponse(TypedDict): @dataclass -class ServerError(BaseException): +class ServerError(Exception): response: Response From c6ca1b8f64da3a5137fd4900893ab2de587f67ab Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:12:11 -0400 Subject: [PATCH 2/6] lint --- kodiak/evaluation.py | 5 +++-- kodiak/pull_request.py | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/kodiak/evaluation.py b/kodiak/evaluation.py index 0e484531e..9bb29d9e9 100644 --- a/kodiak/evaluation.py +++ b/kodiak/evaluation.py @@ -62,21 +62,22 @@ class MissingAppID(Exception): We do _not_ want to display this message to users as it could clobber another instance of kodiak. """ + def __str__(self) -> str: return "missing Github app id" - class BranchMerged(Exception): """branch has already been merged""" + class MergeConflict(Exception): """Merge conflict in PR.""" + def __str__(self) -> str: return "merge conflict" - def review_status(reviews: typing.List[PRReview]) -> PRReviewState: """ Find the most recent actionable review state for a user diff --git a/kodiak/pull_request.py b/kodiak/pull_request.py index 51fea8f17..c7028f184 100644 --- a/kodiak/pull_request.py +++ b/kodiak/pull_request.py @@ -154,9 +154,11 @@ async def mergeability( return MergeabilityResponse.OK, self.event except (NotQueueable, MissingAppID, MergeConflict) as e: await self.set_status(summary="🛑 cannot merge", detail=str(e)) - if isinstance(e, MergeConflict): - if self.event.config.merge.notify_on_conflict: - await self.notify_pr_creator() + if ( + isinstance(e, MergeConflict) + and self.event.config.merge.notify_on_conflict + ): + await self.notify_pr_creator() return MergeabilityResponse.NOT_MERGEABLE, self.event except MissingGithubMergeabilityState: self.log.info("missing mergeability state, need refresh") From f9a354b9908362d9c558997d5d6c47ce9f8cdadd Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:27:24 -0400 Subject: [PATCH 3/6] undo exception changes --- kodiak/evaluation.py | 12 ++++++------ kodiak/queries.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/kodiak/evaluation.py b/kodiak/evaluation.py index 9bb29d9e9..fd6fc84e2 100644 --- a/kodiak/evaluation.py +++ b/kodiak/evaluation.py @@ -35,7 +35,7 @@ async def valid_merge_methods(cfg: config.V1, repo: RepoInfo) -> bool: raise TypeError("Unknown value") -class Queueable(Exception): +class Queueable(BaseException): pass @@ -51,11 +51,11 @@ class WaitingForChecks(Queueable): pass -class NotQueueable(Exception): +class NotQueueable(BaseException): pass -class MissingAppID(Exception): +class MissingAppID(BaseException): """ Application app_id doesn't match configuration @@ -67,12 +67,12 @@ def __str__(self) -> str: return "missing Github app id" -class BranchMerged(Exception): +class BranchMerged(BaseException): """branch has already been merged""" -class MergeConflict(Exception): - """Merge conflict in PR.""" +class MergeConflict(BaseException): + """Merge conflict in the PR.""" def __str__(self) -> str: return "merge conflict" diff --git a/kodiak/queries.py b/kodiak/queries.py index 014bca1c8..c04974f3a 100644 --- a/kodiak/queries.py +++ b/kodiak/queries.py @@ -44,7 +44,7 @@ class GraphQLResponse(TypedDict): @dataclass -class ServerError(Exception): +class ServerError(BaseException): response: Response From 3c818dce6a730ba60d8b0a65684e78dfb4ac693d Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:31:55 -0400 Subject: [PATCH 4/6] ref --- kodiak/evaluation.py | 3 +++ kodiak/pull_request.py | 19 +++++++++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/kodiak/evaluation.py b/kodiak/evaluation.py index fd6fc84e2..26bdf1b99 100644 --- a/kodiak/evaluation.py +++ b/kodiak/evaluation.py @@ -70,6 +70,9 @@ def __str__(self) -> str: class BranchMerged(BaseException): """branch has already been merged""" + def __str__(self) -> str: + return str(self.__doc__) + class MergeConflict(BaseException): """Merge conflict in the PR.""" diff --git a/kodiak/pull_request.py b/kodiak/pull_request.py index c7028f184..accec97c7 100644 --- a/kodiak/pull_request.py +++ b/kodiak/pull_request.py @@ -152,13 +152,21 @@ async def mergeability( self.log.info("okay") await self.set_status(summary="⛴ ready to merge") return MergeabilityResponse.OK, self.event - except (NotQueueable, MissingAppID, MergeConflict) as e: + except (NotQueueable, MissingAppID, MergeConflict, BranchMerged) as e: await self.set_status(summary="🛑 cannot merge", detail=str(e)) if ( isinstance(e, MergeConflict) and self.event.config.merge.notify_on_conflict ): await self.notify_pr_creator() + if ( + isinstance(e, BranchMerged) + and self.event.config.merge.delete_branch_on_merge + ): + await self.client.delete_branch( + branch=self.event.pull_request.headRefName + ) + return MergeabilityResponse.NOT_MERGEABLE, self.event except MissingGithubMergeabilityState: self.log.info("missing mergeability state, need refresh") @@ -169,15 +177,6 @@ async def mergeability( except NeedsBranchUpdate: await self.set_status(summary="⏭ need update") return MergeabilityResponse.NEEDS_UPDATE, self.event - except BranchMerged: - await self.set_status( - summary="🛑 cannot merge", detail="branch merged already" - ) - if self.event.config.merge.delete_branch_on_merge: - await self.client.delete_branch( - branch=self.event.pull_request.headRefName - ) - return MergeabilityResponse.NOT_MERGEABLE, self.event async def update(self) -> None: self.log.info("update") From 5f78d4bdf426703606cfb6cd62147015ba90e592 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:36:00 -0400 Subject: [PATCH 5/6] . --- kodiak/pull_request.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kodiak/pull_request.py b/kodiak/pull_request.py index accec97c7..75b69021f 100644 --- a/kodiak/pull_request.py +++ b/kodiak/pull_request.py @@ -153,12 +153,12 @@ async def mergeability( await self.set_status(summary="⛴ ready to merge") return MergeabilityResponse.OK, self.event except (NotQueueable, MissingAppID, MergeConflict, BranchMerged) as e: - await self.set_status(summary="🛑 cannot merge", detail=str(e)) if ( isinstance(e, MergeConflict) and self.event.config.merge.notify_on_conflict ): await self.notify_pr_creator() + if ( isinstance(e, BranchMerged) and self.event.config.merge.delete_branch_on_merge @@ -167,6 +167,7 @@ async def mergeability( branch=self.event.pull_request.headRefName ) + await self.set_status(summary="🛑 cannot merge", detail=str(e)) return MergeabilityResponse.NOT_MERGEABLE, self.event except MissingGithubMergeabilityState: self.log.info("missing mergeability state, need refresh") From 682c6063bf091050b9b8048e538634ebf4e605b8 Mon Sep 17 00:00:00 2001 From: Christopher Dignam Date: Wed, 17 Jul 2019 19:38:08 -0400 Subject: [PATCH 6/6] fmt --- kodiak/pull_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kodiak/pull_request.py b/kodiak/pull_request.py index 75b69021f..26ec37f92 100644 --- a/kodiak/pull_request.py +++ b/kodiak/pull_request.py @@ -158,7 +158,7 @@ async def mergeability( and self.event.config.merge.notify_on_conflict ): await self.notify_pr_creator() - + if ( isinstance(e, BranchMerged) and self.event.config.merge.delete_branch_on_merge