diff --git a/CHANGELOG.md b/CHANGELOG.md index 4205a92d0..e503de8b6 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bot/kodiak/evaluation.py b/bot/kodiak/evaluation.py index 62b48f4e1..bf364fb98 100644 --- a/bot/kodiak/evaluation.py +++ b/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( diff --git a/bot/kodiak/pull_request.py b/bot/kodiak/pull_request.py index f378f8edb..5dee6c754 100644 --- a/bot/kodiak/pull_request.py +++ b/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, diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index 1e310cb5d..0c5fd1394 100644 --- a/bot/kodiak/queries/__init__.py +++ b/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"] diff --git a/bot/kodiak/queries/commits.py b/bot/kodiak/queries/commits.py index a6c5e31dd..6b203f82f 100644 --- a/bot/kodiak/queries/commits.py +++ b/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] diff --git a/bot/kodiak/test_evaluation.py b/bot/kodiak/test_evaluation.py index 26fdca1c2..3dd199cb3 100644 --- a/bot/kodiak/test_evaluation.py +++ b/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", diff --git a/bot/kodiak/test_queries.py b/bot/kodiak/test_queries.py index f8a302551..3b42f5a60 100644 --- a/bot/kodiak/test_queries.py +++ b/bot/kodiak/test_queries.py @@ -16,6 +16,7 @@ CheckConclusionState, CheckRun, Client, + Commit, EventInfoResponse, GraphQLResponse, MergeableState, @@ -30,7 +31,6 @@ PRReviewState, PullRequest, PullRequestAuthor, - PullRequestCommitUser, PullRequestState, PushAllowance, PushAllowanceActor, @@ -39,9 +39,11 @@ StatusContext, StatusState, Subscription, - get_commit_authors, + get_commits, ) +from kodiak.queries.commits import CommitConnection, GitActor from kodiak.test_utils import wrap_future +from kodiak.tests.fixtures import create_commit @pytest.fixture @@ -578,7 +580,7 @@ async def test_get_subscription_unknown_blocker( ) -def test_get_commit_authors() -> None: +def test_get_commits() -> None: """ Verify we parse commit authors correctly. We should handle the nullability of name and databaseId. @@ -588,6 +590,7 @@ def test_get_commit_authors() -> None: "nodes": [ { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": "Christopher Dignam", @@ -595,11 +598,12 @@ def test_get_commit_authors() -> None: "login": "chdsbd", "type": "User", } - } + }, } }, { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": "b-lowe", @@ -607,11 +611,12 @@ def test_get_commit_authors() -> None: "login": "b-lowe", "type": "User", } - } + }, } }, { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": None, @@ -619,12 +624,13 @@ def test_get_commit_authors() -> None: "login": "kodiakhq", "type": "Bot", } - } + }, } }, - {"commit": {"author": {"user": None}}}, + {"commit": {"parents": {"totalCount": 1}, "author": {"user": None}}}, { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": "Christopher Dignam", @@ -632,11 +638,12 @@ def test_get_commit_authors() -> None: "login": "chdsbd", "type": "User", } - } + }, } }, { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": None, @@ -644,43 +651,39 @@ def test_get_commit_authors() -> None: "login": "j-doe", "type": "SomeGitActor", } - } + }, } }, ] } } - res = get_commit_authors(pr=pull_request_data) + res = get_commits(pr=pull_request_data) assert res == [ - PullRequestCommitUser( - name="Christopher Dignam", databaseId=1929960, login="chdsbd", type="User" + create_commit( + name="Christopher Dignam", database_id=1929960, login="chdsbd", type="User" ), - PullRequestCommitUser( - name="b-lowe", databaseId=5345234, login="b-lowe", type="User" - ), - PullRequestCommitUser( - name=None, databaseId=435453, login="kodiakhq", type="Bot" - ), - PullRequestCommitUser( - name="Christopher Dignam", databaseId=1929960, login="chdsbd", type="User" - ), - PullRequestCommitUser( - name=None, databaseId=None, login="j-doe", type="SomeGitActor" + create_commit(name="b-lowe", database_id=5345234, login="b-lowe", type="User"), + create_commit(name=None, database_id=435453, login="kodiakhq", type="Bot"), + Commit(parents=CommitConnection(totalCount=1), author=GitActor(user=None)), + create_commit( + name="Christopher Dignam", database_id=1929960, login="chdsbd", type="User" ), + create_commit(name=None, database_id=None, login="j-doe", type="SomeGitActor"), ] -def test_get_commit_authors_error_handling() -> None: +def test_get_commits_error_handling() -> None: """ We should handle parsing errors without raising an exception. """ pull_request_data = { "commitHistory": { "nodes": [ - {"commit": {"author": {"user": None}}}, - {"commit": {"author": None}}, + {"commit": {"parents": {"totalCount": 1}, "author": {"user": None}}}, + {"commit": {"parents": {"totalCount": 1}, "author": None}}, { "commit": { + "parents": {"totalCount": 3}, "author": { "user": { "name": None, @@ -688,11 +691,12 @@ def test_get_commit_authors_error_handling() -> None: "login": "kodiakhq", "type": "Bot", } - } + }, } }, { "commit": { + "parents": {"totalCount": 1}, "author": { "user": { "name": "Christopher Dignam", @@ -700,11 +704,12 @@ def test_get_commit_authors_error_handling() -> None: "login": "chdsbd", "type": "User", } - } + }, } }, { "commit": { + "parents": {"totalCount": 2}, "author": { "user": { "name": None, @@ -712,32 +717,38 @@ def test_get_commit_authors_error_handling() -> None: "login": "j-doe", "type": "SomeGitActor", } - } + }, } }, ] } } - res = get_commit_authors(pr=pull_request_data) + res = get_commits(pr=pull_request_data) assert res == [ - PullRequestCommitUser( - name=None, databaseId=435453, login="kodiakhq", type="Bot" + Commit(parents=CommitConnection(totalCount=1), author=GitActor(user=None)), + Commit(parents=CommitConnection(totalCount=1), author=None), + create_commit( + name=None, database_id=435453, login="kodiakhq", type="Bot", parents=3 ), - PullRequestCommitUser( - name="Christopher Dignam", databaseId=1929960, login="chdsbd", type="User" + create_commit( + name="Christopher Dignam", + database_id=1929960, + login="chdsbd", + type="User", + parents=1, ), - PullRequestCommitUser( - name=None, databaseId=None, login="j-doe", type="SomeGitActor" + create_commit( + name=None, database_id=None, login="j-doe", type="SomeGitActor", parents=2 ), ] -def test_get_commit_authors_error_handling_missing_response() -> None: +def test_get_commits_error_handling_missing_response() -> None: """ We should handle parsing errors without raising an exception. """ pull_request_data = {"commitHistory": None} - res = get_commit_authors(pr=pull_request_data) + res = get_commits(pr=pull_request_data) assert res == [] diff --git a/bot/kodiak/tests/__init__.py b/bot/kodiak/tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/bot/kodiak/tests/fixtures.py b/bot/kodiak/tests/fixtures.py new file mode 100644 index 000000000..d2f5683f2 --- /dev/null +++ b/bot/kodiak/tests/fixtures.py @@ -0,0 +1,21 @@ +from typing import Optional + +from kodiak.queries import Commit, CommitConnection, GitActor, PullRequestCommitUser + + +def create_commit( + *, + database_id: Optional[int], + name: Optional[str], + login: str, + type: str, + parents: int = 1, +) -> Commit: + return Commit( + parents=CommitConnection(totalCount=parents), + author=GitActor( + user=PullRequestCommitUser( + databaseId=database_id, name=name, login=login, type=type + ) + ), + )