Skip to content

Commit 052a630

Browse files
joanagmaiaclaude
andcommitted
docs: explain rationale for maintainer identity comparison changes (DE-980)
Adds inline comments to the fix from the previous commit so future readers understand: - why get_maintainers_for_repo intentionally has no platform/type/verified filters (the read must mirror what the write path persists), - why _resolve_identity falls back from github username to email, - why _resolve_maintainers is a shared helper, and - why compare_and_update_maintainers keys by (identityId, role) instead of github_username. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
1 parent 40b9255 commit 052a630

2 files changed

Lines changed: 20 additions & 5 deletions

File tree

services/apps/git_integration/src/crowdgit/database/crud.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,11 @@ async def update_maintainer_run(repo_id: str, maintainer_file: str):
405405

406406

407407
async def get_maintainers_for_repo(repo_id: str):
408+
# No extra platform/type/verified filter here on purpose: the read set must
409+
# mirror what the write path (find_github_identity / find_maintainer_identity_by_email)
410+
# actually persists. A stricter read filter hides rows from the incremental diff,
411+
# which leads to spurious re-inserts and missed end-dates for email-linked
412+
# maintainers (e.g. platform='git' rows on the linux kernel repo).
408413
maintainers_sql_query = """
409414
SELECT mi.role, mi."originalRole", mi."repoUrl", mi."repoId", mi."identityId", mem.value as github_username
410415
FROM "maintainersInternal" mi

services/apps/git_integration/src/crowdgit/services/maintainer/maintainer_service.py

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,10 @@ def make_role(self, title: str):
156156
async def _resolve_identity(
157157
self, github_username: str | None, email: str | None
158158
) -> str | None:
159-
# Fall back from github username to email so a wrong/missing username
160-
# does not prevent an email match.
159+
# Try github username first, fall back to email. The fallback exists because
160+
# extraction frequently emits github_username="unknown" (e.g. the linux
161+
# MAINTAINERS file has ~4k entries with only an email and no github handle),
162+
# and because a wrong/stale username should not block a valid email match.
161163
if github_username and github_username != "unknown":
162164
identity_id = await find_github_identity(github_username)
163165
if identity_id:
@@ -169,6 +171,9 @@ async def _resolve_identity(
169171
async def _resolve_maintainers(
170172
self, maintainers: list[MaintainerInfoItem]
171173
) -> list[tuple[MaintainerInfoItem, str]]:
174+
# Centralised resolver used by both the first-run and incremental paths so
175+
# they share identical lookup + fallback semantics. The semaphore caps DB
176+
# concurrency at 3 (large MAINTAINERS files can carry thousands of entries).
172177
semaphore = asyncio.Semaphore(3)
173178

174179
async def resolve(m: MaintainerInfoItem) -> tuple[MaintainerInfoItem, str | None]:
@@ -209,13 +214,18 @@ async def compare_and_update_maintainers(
209214
self.logger.info(f"Comparing and updating maintainers for repo: {repo_id}")
210215
current_maintainers = await get_maintainers_for_repo(repo_id)
211216

212-
# Key by (identityId, role) — the natural unique tuple for a linked maintainer.
213-
# Keying by github_username collapses every "unknown" extraction into one slot,
214-
# which silently drops most email-only maintainers (e.g. linux MAINTAINERS).
217+
# Key by (identityId, role) — matches the unique index on maintainersInternal
218+
# (repoId, identityId, role) so the diff aligns with what the DB can actually
219+
# hold. The previous github_username key collapsed every "unknown" extraction
220+
# into a single slot, which on the linux MAINTAINERS file (~4k unknowns)
221+
# silently dropped most email-only maintainers and left them unlinked.
215222
current_by_key: dict[tuple[str, str], dict] = {
216223
(m["identityId"], m["role"]): m for m in current_maintainers
217224
}
218225

226+
# Resolve first so the comparison is identity-based, not username-based:
227+
# the same person may extract with different github_username strings across
228+
# runs (or "unknown") yet still resolve to the same memberIdentity.
219229
resolved = await self._resolve_maintainers(maintainers)
220230
new_by_key: dict[tuple[str, str], MaintainerInfoItem] = {
221231
(identity_id, m.normalized_title): m for m, identity_id in resolved

0 commit comments

Comments
 (0)