Skip to content

Commit c6db102

Browse files
cvxluoarmenzg
authored andcommitted
fix(codeowners): parse CODEOWNERs associations case-insensitively (#98668)
Parse CODEOWNERs case-insensitively for SCM providers, behind a feature flag. We do this by constructing a mapping of all user/team ids -> possible external names (ex. `team_id_123: [@cvxluo, @cvxluo, @cvxluo]`) and then returning the reverse mapping (`{"@cvxluo" : team_id_123, "@cvxluo": team_id_123}`) as `associations`. This is notably a breaking change for a subset of customers that have bad mappings. For example, right now I have the following ExternalActors: `@charlieluo/test` maps to #charlie-test-team and `@charlieluo/TEST` maps to #another-charlie-test-team. If we run this new logic on my org, then we'll map both teams to the external team. Thus, to roll this out, we'll set the feature flag on all orgs but the ones with broken mappings (see [query](https://redash.getsentry.net/queries/9433/source)), fix those manually, then remove the feature flag. Follow up of #98667. Refs https://linear.app/getsentry/issue/ID-97/github-code-owner-teams-should-be-case-insensitive and #79296
1 parent d69f140 commit c6db102

File tree

2 files changed

+96
-26
lines changed

2 files changed

+96
-26
lines changed

src/sentry/api/validators/project_codeowners.py

Lines changed: 64 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
from __future__ import annotations
22

3-
from collections.abc import Collection, Iterable, Mapping
3+
from collections.abc import Collection, Mapping, Sequence
4+
from functools import reduce
5+
from operator import or_
46
from typing import Any
57

68
from django.db.models import Subquery
9+
from django.db.models.query_utils import Q
710

11+
from sentry import features
812
from sentry.integrations.models.external_actor import ExternalActor
913
from sentry.integrations.types import ExternalProviders
1014
from sentry.issues.ownership.grammar import parse_code_owners
@@ -23,13 +27,10 @@ def validate_association_emails(
2327

2428

2529
def validate_association_actors(
26-
raw_items: Collection[str],
27-
associations: Iterable[ExternalActor],
30+
raw_items: Sequence[str],
31+
associations: Sequence[str],
2832
) -> list[str]:
29-
raw_items_set = {str(item) for item in raw_items}
30-
# associations are ExternalActor objects
31-
sentry_items = {item.external_name for item in associations}
32-
return list(raw_items_set.difference(sentry_items))
33+
return list(set(raw_items).difference(associations))
3334

3435

3536
def validate_codeowners_associations(
@@ -43,28 +44,57 @@ def validate_codeowners_associations(
4344
filter=dict(emails=emails, organization_id=project.organization_id)
4445
)
4546

46-
# Check if the usernames/teamnames have an association
47-
external_actors = ExternalActor.objects.filter(
48-
external_name__in=usernames + team_names,
49-
organization_id=project.organization_id,
50-
provider__in=[
51-
ExternalProviders.GITHUB.value,
52-
ExternalProviders.GITHUB_ENTERPRISE.value,
53-
ExternalProviders.GITLAB.value,
54-
],
55-
)
47+
if features.has("organizations:use-case-insensitive-codeowners", project.organization):
48+
# GitHub team and user names are case-insensitive
49+
# We build a query that filters on each name we parsed case-insensitively
50+
queries = [Q(external_name__iexact=xname) for xname in usernames + team_names]
51+
if queries:
52+
query = reduce(or_, queries)
53+
external_actors = ExternalActor.objects.filter(
54+
query,
55+
organization_id=project.organization_id,
56+
provider__in=[
57+
ExternalProviders.GITHUB.value,
58+
ExternalProviders.GITHUB_ENTERPRISE.value,
59+
ExternalProviders.GITLAB.value,
60+
],
61+
)
62+
else:
63+
external_actors = ExternalActor.objects.none()
64+
else:
65+
# Check if the usernames/teamnames have an association
66+
external_actors = ExternalActor.objects.filter(
67+
external_name__in=usernames + team_names,
68+
organization_id=project.organization_id,
69+
provider__in=[
70+
ExternalProviders.GITHUB.value,
71+
ExternalProviders.GITHUB_ENTERPRISE.value,
72+
ExternalProviders.GITLAB.value,
73+
],
74+
)
5675

5776
# Convert CODEOWNERS into IssueOwner syntax
5877
users_dict = {}
5978
teams_dict = {}
79+
6080
teams_without_access = []
81+
teams_without_access_external_names = []
6182
users_without_access = []
62-
63-
team_ids_to_external_names: Mapping[int, str] = {
64-
xa.team_id: xa.external_name for xa in external_actors if xa.team_id is not None
83+
users_without_access_external_names = []
84+
85+
team_ids_to_external_names: Mapping[int, list[str]] = {
86+
xa.team_id: list(
87+
filter(lambda team_name: team_name.lower() == xa.external_name.lower(), team_names)
88+
)
89+
for xa in external_actors
90+
if xa.team_id is not None
6591
}
66-
user_ids_to_external_names: Mapping[int, str] = {
67-
xa.user_id: xa.external_name for xa in external_actors if xa.user_id is not None
92+
user_ids_to_external_names: Mapping[int, list[str]] = {
93+
xa.user_id: list(
94+
filter(lambda username: username.lower() == xa.external_name.lower(), usernames)
95+
)
96+
for xa in external_actors
97+
if xa.user_id is not None
6898
}
6999

70100
for user in user_service.get_many(
@@ -79,17 +109,21 @@ def validate_codeowners_associations(
79109
projects = Project.objects.get_for_team_ids(Subquery(team_ids))
80110

81111
if project in projects:
82-
users_dict[user_ids_to_external_names[user.id]] = user.email
112+
for external_name in user_ids_to_external_names[user.id]:
113+
users_dict[external_name] = user.email
83114
else:
84115
users_without_access.append(f"{user.get_display_name()}")
116+
users_without_access_external_names.extend(user_ids_to_external_names[user.id])
85117

86118
for team in Team.objects.filter(id__in=list(team_ids_to_external_names.keys())):
87119
# make sure the sentry team has access to the project
88120
# tied to the codeowner
89121
if project in team.get_projects():
90-
teams_dict[team_ids_to_external_names[team.id]] = f"#{team.slug}"
122+
for external_name in team_ids_to_external_names[team.id]:
123+
teams_dict[external_name] = f"#{team.slug}"
91124
else:
92125
teams_without_access.append(f"#{team.slug}")
126+
teams_without_access_external_names.extend(team_ids_to_external_names[team.id])
93127

94128
emails_dict = {}
95129
user_emails = set()
@@ -102,8 +136,12 @@ def validate_codeowners_associations(
102136

103137
errors = {
104138
"missing_user_emails": validate_association_emails(emails, user_emails),
105-
"missing_external_users": validate_association_actors(usernames, external_actors),
106-
"missing_external_teams": validate_association_actors(team_names, external_actors),
139+
"missing_external_users": validate_association_actors(
140+
usernames, list(associations.keys()) + users_without_access_external_names
141+
),
142+
"missing_external_teams": validate_association_actors(
143+
team_names, list(associations.keys()) + teams_without_access_external_names
144+
),
107145
"teams_without_access": teams_without_access,
108146
"users_without_access": users_without_access,
109147
}

tests/sentry/issues/endpoints/test_project_codeowners_index.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
from sentry.models.projectcodeowners import ProjectCodeOwners
66
from sentry.testutils.cases import APITestCase
7+
from sentry.testutils.helpers import with_feature
78

89

910
class ProjectCodeOwnersEndpointTestCase(APITestCase):
@@ -273,6 +274,37 @@ def test_schema_is_correct(self, get_codeowner_mock_file: MagicMock) -> None:
273274
],
274275
}
275276

277+
@patch(
278+
"sentry.integrations.source_code_management.repository.RepositoryIntegration.get_codeowner_file",
279+
return_value={"html_url": "https://github.com/test/CODEOWNERS"},
280+
)
281+
@with_feature("organizations:use-case-insensitive-codeowners")
282+
def test_case_insensitive_team_matching(self, get_codeowner_mock_file: MagicMock) -> None:
283+
"""Test that team names are matched case-insensitively in CODEOWNERS files."""
284+
285+
external_team_name = self.external_team.external_name
286+
capitalized_external_team_name = external_team_name.swapcase()
287+
288+
self.data[
289+
"raw"
290+
] = f"""
291+
src/frontend/* {external_team_name}
292+
src/frontend2/* {capitalized_external_team_name}
293+
"""
294+
295+
with self.feature({"organizations:integrations-codeowners": True}):
296+
response = self.client.post(self.url, self.data)
297+
298+
assert response.status_code == 201, response.content
299+
300+
assert (
301+
response.data["ownershipSyntax"]
302+
== f"codeowners:src/frontend/* #{self.team.slug}\ncodeowners:src/frontend2/* #{self.team.slug}\n"
303+
)
304+
305+
errors = response.data["errors"]
306+
assert errors["missing_external_teams"] == []
307+
276308
@patch(
277309
"sentry.integrations.source_code_management.repository.RepositoryIntegration.get_codeowner_file",
278310
return_value={"html_url": "https://github.com/test/CODEOWNERS"},

0 commit comments

Comments
 (0)