add(bot): ignore merge commits for coauthors (#581)

Authors of merge commits probably shouldn't be considered coauthors unless they have made other commits to the pull request.

related #576

CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - add new config option (`merge.priority_merge_label`) to place PR at front of merge queue. (#573, #555)
+- add merge queue per target branch (#572, #556)
+
+### Changed
+- merge commits are now excluded when calculating pull request coauthors for `merge.message.include_coauthors`. (#581, #576)
 
 ## 0.33.0 - 2020-11-22
 

bot/kodiak/evaluation.py

@@ -35,6 +35,7 @@
     BranchProtectionRule,
     CheckConclusionState,
     CheckRun,
+    Commit,
     MergeableState,
     MergeStateStatus,
     Permission,
@@ -153,7 +154,7 @@ def get_merge_body(
     config: V1,
     merge_method: MergeMethod,
     pull_request: PullRequest,
-    commit_authors: List[PullRequestCommitUser],
+    commits: List[Commit],
 ) -> MergeBody:
     """
     Get merge options for a pull request to call GitHub API.
@@ -191,7 +192,16 @@ def get_merge_body(
             )
         )
     if config.merge.message.include_coauthors:
-        coauthors += commit_authors
+        for commit in commits:
+            if (
+                # only use commits that have identified authors.
+                commit.author is None
+                or commit.author.user is None
+                # ignore merge commits. They will have more than one parent.
+                or commit.parents.totalCount > 1
+            ):
+                continue
+            coauthors.append(commit.author.user)
 
     coauthor_trailers = get_coauthor_trailers(
         coauthors=coauthors,
@@ -429,7 +439,7 @@ async def mergeable(
     reviews: List[PRReview],
     contexts: List[StatusContext],
     check_runs: List[CheckRun],
-    commit_authors: List[PullRequestCommitUser],
+    commits: List[Commit],
     valid_signature: bool,
     valid_merge_methods: List[MergeMethod],
     repository: RepoInfo,
@@ -1000,7 +1010,7 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None:
     # okay to merge if we reach this point.
 
     if (config.merge.prioritize_ready_to_merge and ready_to_merge) or merging:
-        merge_args = get_merge_body(config, merge_method, pull_request, commit_authors)
+        merge_args = get_merge_body(config, merge_method, pull_request, commits=commits)
         await set_status("⛴ attempting to merge PR (merging)")
         try:
             await api.merge(

bot/kodiak/pull_request.py

@@ -106,7 +106,7 @@ async def evaluate_pr(
                         reviews=pr.event.reviews,
                         contexts=pr.event.status_contexts,
                         check_runs=pr.event.check_runs,
-                        commit_authors=pr.event.commit_authors,
+                        commits=pr.event.commits,
                         valid_signature=pr.event.valid_signature,
                         valid_merge_methods=pr.event.valid_merge_methods,
                         merging=merging,

bot/kodiak/queries/__init__.py

@@ -19,8 +19,9 @@
 
 import kodiak.app_config as conf
 from kodiak.config import V1, MergeMethod
+from kodiak.queries.commits import Commit, CommitConnection, GitActor
 from kodiak.queries.commits import User as PullRequestCommitUser
-from kodiak.queries.commits import get_commit_authors
+from kodiak.queries.commits import get_commits
 from kodiak.throttle import Throttler, get_thottler_for_installation
 
 logger = structlog.get_logger()
@@ -160,6 +161,9 @@ class GraphQLResponse(TypedDict):
                 type: __typename
               }
             }
+            parents {
+              totalCount
+            }
           }
         }
       }
@@ -317,7 +321,7 @@ class EventInfoResponse:
     check_runs: List[CheckRun] = field(default_factory=list)
     valid_signature: bool = False
     valid_merge_methods: List[MergeMethod] = field(default_factory=list)
-    commit_authors: List[PullRequestCommitUser] = field(default_factory=list)
+    commits: List[Commit] = field(default_factory=list)
 
 
 MERGE_PR_MUTATION = """
@@ -946,7 +950,7 @@ async def get_event_info(
             review_requests=get_requested_reviews(pr=pull_request),
             reviews=reviews_with_permissions,
             status_contexts=get_status_contexts(pr=pull_request),
-            commit_authors=get_commit_authors(pr=pull_request),
+            commits=get_commits(pr=pull_request),
             check_runs=get_check_runs(pr=pull_request),
             head_exists=get_head_exists(pr=pull_request),
             valid_signature=get_valid_signature(pr=pull_request),
@@ -1182,3 +1186,6 @@ async def get_headers(*, installation_id: str) -> Mapping[str, str]:
         Authorization=f"token {token}",
         Accept="application/vnd.github.machine-man-preview+json,application/vnd.github.antiope-preview+json,application/vnd.github.lydian-preview+json",
     )
+
+
+__all__ = ["Commit", "GitActor", "CommitConnection", "PullRequestCommitUser"]

bot/kodiak/queries/commits.py

@@ -6,6 +6,10 @@
 logger = structlog.get_logger()
 
 
+class CommitConnection(pydantic.BaseModel):
+    totalCount: int
+
+
 class User(pydantic.BaseModel):
     databaseId: Optional[int]
     login: str
@@ -22,6 +26,7 @@ class GitActor(pydantic.BaseModel):
 
 
 class Commit(pydantic.BaseModel):
+    parents: CommitConnection
     author: Optional[GitActor]
 
 
@@ -37,7 +42,7 @@ class PullRequest(pydantic.BaseModel):
     commitHistory: PullRequestCommitConnection
 
 
-def get_commit_authors(*, pr: Dict[str, Any]) -> List[User]:
+def get_commits(*, pr: Dict[str, Any]) -> List[Commit]:
     """
     Extract the commit authors from the pull request commits.
     """
@@ -50,9 +55,4 @@ def get_commit_authors(*, pr: Dict[str, Any]) -> List[User]:
     nodes = pull_request.commitHistory.nodes
     if not nodes:
         return []
-    commit_authors = []
-    for node in nodes:
-        if node.commit.author is None or node.commit.author.user is None:
-            continue
-        commit_authors.append(node.commit.author.user)
-    return commit_authors
+    return [node.commit for node in nodes]

bot/kodiak/test_evaluation.py

@@ -26,6 +26,7 @@
     BranchProtectionRule,
     CheckConclusionState,
     CheckRun,
+    Commit,
     MergeableState,
     MergeStateStatus,
     NodeListPushAllowance,
@@ -36,7 +37,6 @@
     PRReviewState,
     PullRequest,
     PullRequestAuthor,
-    PullRequestCommitUser,
     PullRequestReviewDecision,
     PullRequestState,
     PushAllowance,
@@ -49,6 +49,7 @@
     SubscriptionExpired,
     TrialExpired,
 )
+from kodiak.tests.fixtures import create_commit
 
 log = logging.getLogger(__name__)
 
@@ -342,7 +343,7 @@ async def __call__(
         reviews: List[PRReview] = ...,
         contexts: List[StatusContext] = ...,
         check_runs: List[CheckRun] = ...,
-        commit_authors: List[PullRequestCommitUser] = ...,
+        commits: List[Commit] = ...,
         valid_signature: bool = ...,
         valid_merge_methods: List[MergeMethod] = ...,
         merging: bool = ...,
@@ -370,7 +371,7 @@ async def mergeable(
         reviews: List[PRReview] = [create_review()],
         contexts: List[StatusContext] = [create_context()],
         check_runs: List[CheckRun] = [create_check_run()],
-        commit_authors: List[PullRequestCommitUser] = [],
+        commits: List[Commit] = [],
         valid_signature: bool = False,
         valid_merge_methods: List[MergeMethod] = [
             MergeMethod.merge,
@@ -401,7 +402,7 @@ async def mergeable(
             reviews=reviews,
             contexts=contexts,
             check_runs=check_runs,
-            commit_authors=commit_authors,
+            commits=commits,
             valid_signature=valid_signature,
             valid_merge_methods=valid_merge_methods,
             repository=repository,
@@ -3005,7 +3006,7 @@ def test_pr_get_merge_body_full() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3021,7 +3022,7 @@ def test_pr_get_merge_body_empty() -> None:
         config=V1(version=1),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(merge_method="squash")
     assert actual == expected
@@ -3041,7 +3042,7 @@ def test_get_merge_body_strip_html_comments() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(merge_method="squash", commit_message="hello world")
     assert actual == expected
@@ -3056,7 +3057,7 @@ def test_get_merge_body_empty() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(merge_method="squash", commit_message="")
     assert actual == expected
@@ -3079,7 +3080,7 @@ def test_get_merge_body_includes_pull_request_url() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3109,7 +3110,7 @@ def test_get_merge_body_includes_pull_request_url_with_coauthor() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3139,7 +3140,7 @@ def test_get_merge_body_include_pull_request_author_user() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3166,7 +3167,7 @@ def test_get_merge_body_include_pull_request_author_bot() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3196,7 +3197,7 @@ def test_get_merge_body_include_pull_request_author_mannequin() -> None:
         ),
         pull_request=pull_request,
         merge_method=MergeMethod.squash,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(
         merge_method="squash",
@@ -3221,7 +3222,7 @@ def test_get_merge_body_include_pull_request_author_invalid_body_style() -> None
         config=config,
         pull_request=pull_request,
         merge_method=MergeMethod.merge,
-        commit_authors=[],
+        commits=[],
     )
     expected = MergeBody(merge_method="merge", commit_message=None)
     assert actual == expected
@@ -3242,29 +3243,36 @@ def test_get_merge_body_include_coauthors() -> None:
         config=config,
         merge_method=MergeMethod.merge,
         pull_request=pull_request,
-        commit_authors=[
-            PullRequestCommitUser(
-                databaseId=9023904, name="Bernard Lowe", login="b-lowe", type="User"
+        commits=[
+            create_commit(
+                database_id=9023904, name="Bernard Lowe", login="b-lowe", type="User"
             ),
-            PullRequestCommitUser(
-                databaseId=590434, name="Maeve Millay", login="maeve-m", type="Bot"
+            create_commit(
+                database_id=590434, name="Maeve Millay", login="maeve-m", type="Bot"
             ),
             # we default to the login when name is None.
-            PullRequestCommitUser(
-                databaseId=771233, name=None, login="d-abernathy", type="Bot"
+            create_commit(
+                database_id=771233, name=None, login="d-abernathy", type="Bot"
             ),
             # without a databaseID the commit author will be ignored.
-            PullRequestCommitUser(
-                databaseId=None, name=None, login="william", type="User"
-            ),
+            create_commit(database_id=None, name=None, login="william", type="User"),
             # duplicate should be ignored.
-            PullRequestCommitUser(
-                databaseId=9023904, name="Bernard Lowe", login="b-lowe", type="User"
+            create_commit(
+                database_id=9023904, name="Bernard Lowe", login="b-lowe", type="User"
+            ),
+            # merge commits should be ignored. merge commits will have more than
+            # one parent.
+            create_commit(
+                database_id=1,
+                name="Arnold Weber",
+                login="arnold",
+                type="User",
+                parents=2,
             ),
             # pull request author should be ignored when
             # include_pull_request_author is not enabled
-            PullRequestCommitUser(
-                databaseId=pull_request.author.databaseId,
+            create_commit(
+                database_id=pull_request.author.databaseId,
                 name="Joe PR Author",
                 login="j-author",
                 type="User",
@@ -3293,14 +3301,14 @@ def test_get_merge_body_include_coauthors_include_pr_author() -> None:
         config=config,
         merge_method=MergeMethod.merge,
         pull_request=pull_request,
-        commit_authors=[
-            PullRequestCommitUser(
-                databaseId=9023904, name="Bernard Lowe", login="b-lowe", type="User"
+        commits=[
+            create_commit(
+                database_id=9023904, name="Bernard Lowe", login="b-lowe", type="User"
             ),
             # we should ignore a duplicate entry for the PR author when
             # include_pull_request_author is enabled.
-            PullRequestCommitUser(
-                databaseId=pull_request.author.databaseId,
+            create_commit(
+                database_id=pull_request.author.databaseId,
                 name=pull_request.author.name,
                 login=pull_request.author.login,
                 type=pull_request.author.type,
@@ -3327,12 +3335,10 @@ def test_get_merge_body_include_coauthors_invalid_body_style() -> None:
         config=config,
         pull_request=pull_request,
         merge_method=MergeMethod.merge,
-        commit_authors=[
-            PullRequestCommitUser(
-                databaseId=9023904, name="", login="b-lowe", type="User"
-            ),
-            PullRequestCommitUser(
-                databaseId=590434, name="Maeve Millay", login="maeve-m", type="Bot"
+        commits=[
+            create_commit(database_id=9023904, name="", login="b-lowe", type="User"),
+            create_commit(
+                database_id=590434, name="Maeve Millay", login="maeve-m", type="Bot"
             ),
         ],
     )
@@ -3364,9 +3370,9 @@ async def test_mergeable_include_coauthors() -> None:
         await mergeable(
             api=api,
             config=config,
-            commit_authors=[
-                PullRequestCommitUser(
-                    databaseId=73213123,
+            commits=[
+                create_commit(
+                    database_id=73213123,
                     name="Barry Block",
                     login="b-block",
                     type="User",