Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ lint.install:
pip install -Iv ruff

lint.run:
ruff check
ruff check --fix
ruff format

lint.check:
Expand Down
8 changes: 6 additions & 2 deletions webhook_handlers/tests/test_github.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,12 @@ def test_push_updates_only_unmerged_commits_with_branch_name(self):

@patch("redis.Redis.sismember", lambda x, y, z: False)
def test_push_updates_commit_on_default_branch(self):
commit1 = CommitFactory(merged=False, repository=self.repo)
commit2 = CommitFactory(merged=False, repository=self.repo)
commit1 = CommitFactory(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change branch name for these commits to non-default branch names for the test to make sense - I believe it originally wasn't working as intended

merged=False, repository=self.repo, branch="feature-branch"
)
commit2 = CommitFactory(
merged=False, repository=self.repo, branch="feature-branch"
)

merged_branch_name = "merged"
repo_branch = self.repo.branch
Expand Down
8 changes: 6 additions & 2 deletions webhook_handlers/tests/test_github_enterprise.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,12 @@ def test_push_updates_only_unmerged_commits_with_branch_name(self):

@patch("redis.Redis.sismember", lambda x, y, z: False)
def test_push_updates_commit_on_default_branch(self):
commit1 = CommitFactory(merged=False, repository=self.repo)
commit2 = CommitFactory(merged=False, repository=self.repo)
commit1 = CommitFactory(
merged=False, repository=self.repo, branch="feature-branch"
)
commit2 = CommitFactory(
merged=False, repository=self.repo, branch="feature-branch"
)

merged_branch_name = "merged"
repo_branch = self.repo.branch
Expand Down
18 changes: 11 additions & 7 deletions webhook_handlers/views/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from hashlib import sha1, sha256
from typing import Optional

from django.db.models import Q
from django.utils import timezone
from django.utils.crypto import constant_time_compare
from rest_framework import status
Expand Down Expand Up @@ -239,24 +240,25 @@ def push(self, request, *args, **kwargs):
)
return Response(data=WebhookHandlerErrorMessages.SKIP_WEBHOOK_IGNORED)

branch_name = self.request.data.get("ref")[11:]
pushed_to_branch_name = self.request.data.get("ref")[11:]
commits = self.request.data.get("commits", [])

if not commits:
log.debug(
f"No commits in webhook payload for branch {branch_name}",
f"No commits in webhook payload for branch {pushed_to_branch_name}",
extra=dict(repoid=repo.repoid, github_webhook_event=self.event),
)
return Response()

commits_queryset = Commit.objects.filter(
~Q(branch=pushed_to_branch_name),
repository=repo,
commitid__in=[commit.get("id") for commit in commits],
merged=False,
)
commits_queryset.update(branch=branch_name)
if branch_name == repo.branch:
commits_queryset.update(merged=True)

if pushed_to_branch_name == repo.branch:
Copy link
Contributor

Choose a reason for hiding this comment

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

For more optimization, we can probably also call this before querying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

donee

commits_queryset.update(branch=pushed_to_branch_name, merged=True)
log.info(
"Pushed commits to default branch; setting merged to True",
extra=dict(
Expand All @@ -265,9 +267,11 @@ def push(self, request, *args, **kwargs):
commits=[commit.get("id") for commit in commits],
),
)
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to do it this way because after a django update, if your original query is no longer satisfied, which was the case when we updated the branch to pushed_to_branch_name, then your queryset no longer uses records. This is the same logic because both if's do an update, only difference is that the first if also does the merged=True

commits_queryset.update(branch=pushed_to_branch_name)

log.info(
f"Branch name updated for commits to {branch_name}",
f"Branch name updated for commits to {pushed_to_branch_name}",
extra=dict(
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably also need to be in the if statement now. We're not updating the branch if it's not the default branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed and consolidated into 1 log

repoid=repo.repoid,
github_webhook_event=self.event,
Expand Down Expand Up @@ -300,7 +304,7 @@ def push(self, request, *args, **kwargs):
TaskService().status_set_pending(
repoid=repo.repoid,
commitid=most_recent_commit.get("id"),
branch=branch_name,
branch=pushed_to_branch_name,
on_a_pull_request=False,
)

Expand Down
Loading