Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion bot/kodiak/evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,22 @@ def review_status(reviews: List[PRReview]) -> PRReviewState:


def deduplicate_check_runs(check_runs: Iterable[CheckRun]) -> Iterable[CheckRun]:
check_run_map = {check_run.name: check_run for check_run in check_runs}
"""
unique by name, workflow databaseId, app
Copy link
Copy Markdown
Owner Author

@chdsbd chdsbd Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is sooooooo complicated. GitHub Actions can create multiple check suites for the same workflow instead of just creating a new check run in the same check suite :(

"""
check_run_map = {}
for check_run in check_runs:
app = (
check_run.checkSuite.app.databaseId
if check_run.checkSuite.app is not None
else None
)
workflow = (
check_run.checkSuite.workflowRun.workflow.databaseId
if check_run.checkSuite.workflowRun is not None
else None
)
check_run_map[(check_run.name, app, workflow)] = check_run
return check_run_map.values()


Expand Down
28 changes: 28 additions & 0 deletions bot/kodiak/queries/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,16 @@ def get_event_info_query(
checkRuns(first: 100) {
nodes {
name
checkSuite {
app {
databaseId
}
workflowRun {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be null if we don't have permission. We currently don't have access. We need to request new permissions

workflow {
databaseId
}
}
}
conclusion
}
}
Expand Down Expand Up @@ -552,8 +562,26 @@ class CheckConclusionState(Enum):
TIMED_OUT = "TIMED_OUT"


class App(BaseModel):
databaseId: Optional[int]


class Workflow(BaseModel):
databaseId: Optional[int]


class WorkflowRun(BaseModel):
workflow: Workflow


class CheckSuite(BaseModel):
app: Optional[App]
workflowRun: Optional[WorkflowRun]


class CheckRun(BaseModel):
name: str
checkSuite: CheckSuite
conclusion: Optional[CheckConclusionState]


Expand Down
15 changes: 14 additions & 1 deletion bot/kodiak/test_evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
from kodiak.messages import APICallRetry
from kodiak.pull_request import APICallError
from kodiak.queries import (
App,
BranchProtectionRule,
CheckConclusionState,
CheckRun,
CheckSuite,
Commit,
MergeableState,
MergeStateStatus,
Expand All @@ -36,6 +38,8 @@
Subscription,
SubscriptionExpired,
TrialExpired,
Workflow,
WorkflowRun,
)

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -282,8 +286,17 @@ def create_check_run(
*,
name: str = "WIP (beta)",
conclusion: CheckConclusionState = CheckConclusionState.SUCCESS,
app_id: Optional[int] = None,
workflow_id: Optional[int] = None,
) -> CheckRun:
return CheckRun(name=name, conclusion=conclusion)
return CheckRun(
name=name,
conclusion=conclusion,
checkSuite=CheckSuite(
app=App(databaseId=app_id),
workflowRun=WorkflowRun(workflow=Workflow(databaseId=workflow_id)),
),
)


def create_review_request() -> PRReviewRequest:
Expand Down
35 changes: 35 additions & 0 deletions bot/kodiak/tests/evaluation/test_branch_protection.py
Original file line number Diff line number Diff line change
Expand Up @@ -717,3 +717,38 @@ async def test_mergeable_uncollapsed_reviews() -> None:
)
assert api.set_status.call_count == 1
assert "Merging blocked by GitHub requirements" in api.set_status.calls[0]["msg"]


@pytest.mark.asyncio
async def test_merge_blocked_multiple_contexts() -> None:
"""
When we have a required status check of "build", if we have any context with the same
name that's failing, we should block merge. This matches GitHub's behavior.
"""
api = create_api()
mergeable = create_mergeable()
pull_request = create_pull_request()
pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
branch_protection = create_branch_protection()
branch_protection.requiredStatusCheckContexts = ["build"]

context_go = create_context()
context_go.context = "build"
context_ts = create_context()
context_ts.context = "build"

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
contexts=[context_go, context_ts],
)

assert api.set_status.called is True
assert api.queue_for_merge.called is False

assert "failing required status checks" in api.set_status.calls[0]["msg"]

assert api.dequeue.call_count == 1
assert api.update_branch.called is False
assert api.merge.called is False
52 changes: 46 additions & 6 deletions bot/kodiak/tests/evaluation/test_check_runs.py
Original file line number Diff line number Diff line change
Expand Up @@ -582,26 +582,66 @@ async def test_duplicate_check_suites() -> None:
pull_request.mergeStateStatus = MergeStateStatus.BEHIND
branch_protection = create_branch_protection()
branch_protection.requiresStatusChecks = True
branch_protection.requiredStatusCheckContexts = ["Pre-merge checks"]
branch_protection.requiredStatusCheckContexts = [
"Pre-merge checks",
"build",
"circleci-test",
]
mergeable = create_mergeable()
await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
check_runs=[
create_check_run(
name="Pre-merge checks", conclusion=CheckConclusionState.NEUTRAL
name="Pre-merge checks",
conclusion=CheckConclusionState.NEUTRAL,
app_id=1000,
workflow_id=8888,
),
create_check_run(
name="Pre-merge checks",
conclusion=CheckConclusionState.FAILURE,
app_id=1000,
workflow_id=8888,
),
create_check_run(
name="Pre-merge checks",
conclusion=CheckConclusionState.SUCCESS,
app_id=1000,
workflow_id=8888,
),
create_check_run(
name="build",
conclusion=CheckConclusionState.FAILURE,
app_id=1000,
workflow_id=2222,
),
create_check_run(
name="Pre-merge checks", conclusion=CheckConclusionState.FAILURE
name="build",
conclusion=CheckConclusionState.SUCCESS,
app_id=1000,
workflow_id=33333,
),
create_check_run(
name="Pre-merge checks", conclusion=CheckConclusionState.SUCCESS
name="build",
conclusion=CheckConclusionState.SUCCESS,
app_id=1000,
workflow_id=444444,
),
create_check_run(
name="circleci-test",
conclusion=CheckConclusionState.FAILURE,
app_id=1000,
workflow_id=None,
),
],
)
assert "enqueued for merge" in api.set_status.calls[0]["msg"]
assert api.queue_for_merge.call_count == 1
assert "required status checks" in api.set_status.calls[0]["msg"]
assert "build" in api.set_status.calls[0]["msg"]
assert "circleci-test" in api.set_status.calls[0]["msg"]
assert "Pre-merge" not in api.set_status.calls[0]["msg"]
assert api.queue_for_merge.call_count == 0


@pytest.mark.asyncio
Expand Down