Skip to content

Commit 3d8663c

Browse files
authored
BCR PR reviewer: properly deal with user renames (bazelbuild#2629)
We're currently querying GitHub with the maintainer's username to see whether it matches the user ID. This doesn't work when the user has been renamed. Instead, we can use the user ID to query for the up-to-date username, and post a comment to the PR in case of a mismatch. Effectively, this treats the user ID as the source of truth, which is more stable and secure anyway. Tested at Wyverald/bazel-central-registry#1
1 parent b0f59b9 commit 3d8663c

1 file changed

Lines changed: 34 additions & 24 deletions

File tree

actions/bcr-pr-reviewer/index.js

Lines changed: 34 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,10 @@ async function fetchAllModulesWithMetadataChange(octokit, owner, repo, prNumber)
8282
return await _processAllPrFiles(octokit, owner, repo, prNumber, fileProcessor);
8383
}
8484

85-
async function generateMaintainersMap(octokit, owner, repo, modifiedModules, toNotifyOnly) {
85+
async function generateMaintainersMap(octokit, owner, repo, modifiedModules, toNotifyOnly, prNumber) {
8686
const maintainersMap = new Map(); // Map: maintainer GitHub username (lowercase) -> Set of module they maintain
8787
const modulesWithoutGithubMaintainers = new Set(); // Set of module names without module maintainers
88+
const userIdToName = new Map(); // Map: github_user_id -> username; serves as a cache
8889
for (const moduleName of modifiedModules) {
8990
console.log(`Fetching metadata for module: ${moduleName}`);
9091
try {
@@ -99,28 +100,37 @@ async function generateMaintainersMap(octokit, owner, repo, modifiedModules, toN
99100
let hasGithubMaintainer = false;
100101
for (const maintainer of metadata.maintainers) {
101102
// Only add maintainers with a github handle set. When `toNotifyOnly`, also exclude those who have set "do_not_notify"
102-
if (maintainer.github && !(toNotifyOnly && maintainer["do_not_notify"])) {
103-
hasGithubMaintainer = true;
104-
if (!maintainersMap.has(maintainer.github.toLowerCase())) {
105-
try {
106-
// Verify maintainer.github matches maintainer.github_user_id via GitHub API
107-
const { data: user } = await octokit.rest.users.getByUsername({
108-
username: maintainer.github,
109-
});
110-
111-
if (!user || user.id !== maintainer.github_user_id) {
112-
console.error(`Maintainer ${maintainer.github} does not match the user ID ${maintainer.github_user_id} or user not found`);
113-
setFailed(`Maintainer ${maintainer.github} does not match the user ID ${maintainer.github_user_id} or user not found`);
114-
return;
115-
}
116-
maintainersMap.set(maintainer.github.toLowerCase(), new Set());
117-
} catch (error) {
118-
console.error(`Failed to fetch user ID for GitHub username ${maintainer.github}: ${error.message}`);
119-
setFailed(`Failed to fetch user ID for GitHub username ${maintainer.github}: ${error.message}`);
120-
return;
121-
}
103+
if (!maintainer.github || (toNotifyOnly && maintainer["do_not_notify"])) {
104+
continue;
105+
}
106+
107+
hasGithubMaintainer = true;
108+
109+
if (!userIdToName.has(maintainer.github_user_id)) {
110+
try {
111+
const { data: user } = await octokit.request('GET /user/{account_id}', {
112+
account_id: maintainer.github_user_id,
113+
});
114+
115+
userIdToName.set(maintainer.github_user_id, user.login);
116+
maintainersMap.set(user.login.toLowerCase(), new Set());
117+
} catch (error) {
118+
console.error(`Error fetching maintainer with GitHub ID ${maintainer.github_user_id}: ${error}`);
119+
setFailed(`Error fetching maintainer with GitHub ID ${maintainer.github_user_id}: ${error}`);
120+
return;
122121
}
123-
maintainersMap.get(maintainer.github.toLowerCase()).add(moduleName);
122+
}
123+
124+
const actualName = userIdToName.get(maintainer.github_user_id);
125+
maintainersMap.get(actualName.toLowerCase()).add(moduleName);
126+
127+
// Detect if there's a mismatch between the github username in metadata.json and what GitHub reports
128+
if (actualName.toLowerCase() !== maintainer.github.toLowerCase() && prNumber) {
129+
console.log(`Detected GitHub username mismatch for user ID ${maintainer.github_user_id}: metadata.json has '${maintainer.github}', but GitHub has '${actualName}'.`);
130+
const commentBody = `⚠️ **GitHub Username Mismatch Detected**\n\n` +
131+
`The BCR registry entry for module **${moduleName}** contains an outdated GitHub username (\`@${maintainer.github}\`) that does not match their current active GitHub username (\`@${actualName}\`).\n\n` +
132+
`Please update the \`metadata.json\` file of **${moduleName}** to use the up-to-date username \`"${actualName}"\` to ensure proper review routing and notifications.`;
133+
await postComment(octokit, owner, repo, prNumber, commentBody);
124134
}
125135
}
126136

@@ -447,7 +457,7 @@ async function reviewPR(octokit, owner, repo, prNumber) {
447457
}
448458

449459
// Figure out maintainers for each modified module
450-
const [maintainersMap, _] = await generateMaintainersMap(octokit, owner, repo, modifiedModules, /* toNotifyOnly= */ false);
460+
const [maintainersMap, _] = await generateMaintainersMap(octokit, owner, repo, modifiedModules, /* toNotifyOnly= */ false, prNumber);
451461
console.log('Maintainers Map:');
452462
for (const [maintainer, maintainedModules] of maintainersMap.entries()) {
453463
console.log(`- Maintainer: ${maintainer}, Modules: ${Array.from(maintainedModules).join(', ')}`);
@@ -611,7 +621,7 @@ async function runNotifier(octokit) {
611621
console.log(`Modified modules: ${Array.from(modifiedModules).join(', ')}`);
612622

613623
// Figure out maintainers for each modified module
614-
const [ maintainersMap, modulesWithoutGithubMaintainers ] = await generateMaintainersMap(octokit, owner, repo, modifiedModules, /* toNotifyOnly= */ true);
624+
const [maintainersMap, modulesWithoutGithubMaintainers] = await generateMaintainersMap(octokit, owner, repo, modifiedModules, /* toNotifyOnly= */ true, prNumber);
615625

616626
// Notify maintainers for modules with module maintainers
617627
await notifyMaintainers(octokit, owner, repo, prNumber, maintainersMap);

0 commit comments

Comments
 (0)