Skip to content
Merged
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
2 changes: 2 additions & 0 deletions .github/api-review-approvers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ data-plane:
- maorleger
go:
- jhendrixMSFT
rust:
- heaths

management-plane:
all:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ Service team fills out issue form
│ • Sync language labels │
└─────────────────────────────┘
↓
┌──────────────┐ ┌──────────────┐
│ All valid? │───→│ ready-for- │
│ │ Y │ review label │
└──────────────┘ └──────────────┘
┌──────────────┐ ┌───────────────────────────┐
│ All valid? │───→│ ready-for-review label │
│ │ Y │ + assign one architect │
│ │ │ per language (notifies │
│ │ │ them via GitHub) │
└──────────────┘ └───────────────────────────┘
│ N
↓
┌──────────────┐
Expand All @@ -49,13 +51,13 @@ Architect applies <language>-api-approved label

### Applied by automation

| Label | When applied | Meaning |
| -------------------------------------------- | --------------------------------- | ------------------------------- |
| `architecture` | On issue creation (from template) | Issue is a board review request |
| `board-review` | On issue creation (from template) | Issue is a board review request |
| `.NET`, `Java`, `Python`, `TypeScript`, `Go` | Triage detects selected language | Language needs review |
| `ready-for-review` | All required artifacts validated | Ready for architects |
| `needs-info` | Missing or invalid artifacts | Service team needs to fix |
| Label | When applied | Meaning |
| ----------------------------------------------------------- | --------------------------------- | ------------------------------- |
| `architecture` | On issue creation (from template) | Issue is a board review request |
| `board-review` | On issue creation (from template) | Issue is a board review request |
| `.NET`, `Java`, `Python`, `TypeScript`, `Go`, `C++`, `Rust` | Triage detects selected language | Language needs review |
| `ready-for-review` | All required artifacts validated | Ready for architects |
| `needs-info` | Missing or invalid artifacts | Service team needs to fix |

### Applied by architects (manually)

Expand All @@ -78,6 +80,26 @@ When all selected languages have `<lang>-api-approved`, the issue is auto-closed

Unauthorized label additions are reverted with a comment.

## Reviewer Assignment

When an issue reaches `ready-for-review`, `assign-reviewers.js` assigns every
architect configured for each selected language, drawn from the same
`api-review-approvers.yml` approvers list. Assigning the issue triggers GitHub's
native notifications, so architects are reached through their own notification
preferences. The assigned handles are listed in the triage "materials verified"
comment for visibility.

For **management plane** issues, the management-plane approvers are assigned
alongside the language-specific architects.

Assignment is idempotent: an architect already assigned is not re-assigned, so
subsequent edits to a ready issue do not re-notify. If an edit changes the
selected languages, architects the automation previously assigned but that no
longer apply are removed; assignees added manually (anyone outside the approvers
list) are always preserved. If the assignment API call fails, the "materials verified"
comment still posts with a note asking for manual assignment, so the issue is
never left silently unassigned.

## Approver Configuration

Approvers are defined in `.github/api-review-approvers.yml`:
Expand Down
20 changes: 20 additions & 0 deletions .github/workflows/src/arch-board-review/approval-check.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,25 @@ function getManagementApprovers(approversConfig) {
return approversConfig["management-plane"]?.all ?? [];
}

/**
* Every login that appears anywhere in the approvers list (all data-plane languages plus
* management-plane). Used to distinguish automation-managed assignees from manual
* ones during reviewer reconciliation.
*/
function getAllApprovers(approversConfig) {
const all = new Set();
const dataPlane = approversConfig["data-plane"] ?? {};
for (const approvers of Object.values(dataPlane)) {
for (const login of approvers ?? []) {
all.add(login);
}
}
for (const login of getManagementApprovers(approversConfig)) {
all.add(login);
}
return [...all];
}

function isManagementPlaneIssue(issueBody) {
return /management\s*plane/i.test(issueBody);
}
Expand Down Expand Up @@ -64,6 +83,7 @@ export {
APPROVAL_LABEL_PATTERN,
buildApprovalCompleteComment,
buildUnauthorizedComment,
getAllApprovers,
getAllowedApprovers,
getApproversForLanguage,
getManagementApprovers,
Expand Down
174 changes: 174 additions & 0 deletions .github/workflows/src/arch-board-review/assign-reviewers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/**
* Assign Reviewers — routes a ready review request to per-language architects.
*
* Called by triage once an issue reaches `ready-for-review`. For each selected
* language it assigns every configured architect from the shared
* `api-review-approvers.yml` approvers list to the issue. Assigning triggers
* GitHub's native notifications, so architects are reached through their own
* notification preferences.
*
* The approvers list is the same source of truth used by approval validation, so
* there is no separate reviewer list to keep in sync. Management-plane issues
* additionally consider the management-plane approvers.
*
* The assignment itself is the notification mechanism; the resolved handles are
* surfaced in triage's "materials verified" comment for visibility. Idempotent:
* only newly resolved assignees are added, so edits to an already-assigned issue
* do not re-assign or re-notify.
*
* Edits are reconciled: if a language selection changes (e.g. Java swapped for
* Python), architects the automation previously assigned but that no longer apply
* are removed. Only configured approvers are removed, so assignees added manually
* (or by anyone outside the approvers list) are always preserved.
*/
import { readFile } from "node:fs/promises";

import { getSelectedLanguages } from "./issue-parsing.js";
import {
getAllApprovers,
getApproversForLanguage,
getManagementApprovers,
isManagementPlaneIssue,
loadApproversConfig,
} from "./approval-check.js";

const DEFAULT_APPROVERS_PATH = new URL("../../../api-review-approvers.yml", import.meta.url);

/**
* Resolve the architects for each selected language from the approvers list.
*
* Every configured architect for a language is assigned (management-plane issues
* additionally include the management-plane approvers). There is intentionally no
* load balancing - if a language has more than one architect they are all tagged.
* If that ever gets noisy we can add distribution later.
*
* @param {object} params
* @param {string} params.issueBody
* @param {object} params.approversConfig
* @returns {{ byLanguage: { language: string, reviewers: string[] }[], assignees: string[], unassigned: string[] }}
* `unassigned` lists selected languages that have no configured architect.
*/
function resolveAssignments({ issueBody, approversConfig }) {
const selectedLanguages = getSelectedLanguages(issueBody);
const managementReviewers = isManagementPlaneIssue(issueBody)
? getManagementApprovers(approversConfig)
: [];

const byLanguage = [];
const assigneeSet = new Set();
const unassigned = [];

for (const language of selectedLanguages) {
const reviewers = [
...new Set([
...getApproversForLanguage(approversConfig, language.id),
...managementReviewers,
]),
];
if (reviewers.length > 0) {
byLanguage.push({ language: language.label, reviewers });
for (const reviewer of reviewers) {
assigneeSet.add(reviewer);
}
} else {
unassigned.push(language.label);
}
}

return { byLanguage, assignees: [...assigneeSet], unassigned };
}

export { resolveAssignments };

/**
* Resolve reviewers for the issue, assign any that are not already assigned, and
* remove stale automation-assigned architects that no longer apply after an edit.
* Does not post a comment - the resolved handles are surfaced by the caller's
* validation comment. Returns the per-language mapping for that rendering.
*
* @returns {Promise<{ byLanguage: { language: string, reviewers: string[] }[], assigned: string[], removed: string[], unassigned: string[], skipped: boolean, reason: string | null }>}
* When `skipped` is true, `reason` is `"already-assigned"` (reviewers exist and
* nothing changed) or `"no-reviewers-resolved"` (no architect could be resolved
* for the selected languages). When `skipped` is false, `reason` is null.
*/
export default async function assignReviewers({
github,
context,
core,
approversConfig,
approversPath = DEFAULT_APPROVERS_PATH,
readFileImpl = readFile,
}) {
const issue = context.payload.issue;
const issueBody = issue.body ?? "";
const issueNumber = issue.number;
const currentAssignees = (issue.assignees ?? []).map((assignee) => assignee.login);
const { owner, repo } = context.repo;

const resolvedConfig =
approversConfig ?? (await loadApproversConfig(approversPath, readFileImpl));

const { byLanguage, assignees, unassigned } = resolveAssignments({
issueBody,
approversConfig: resolvedConfig,
});

if (unassigned.length > 0) {
core?.warning?.(`No architect configured for: ${unassigned.join(", ")}`);
}

const resolvedLower = new Set(assignees.map((login) => login.toLowerCase()));
const currentLower = new Set(currentAssignees.map((login) => login.toLowerCase()));
const configuredApprovers = new Set(
getAllApprovers(resolvedConfig).map((login) => login.toLowerCase()),
);

const newAssignees = assignees.filter((login) => !currentLower.has(login.toLowerCase()));

// Reconcile edits: remove architects the automation manages (configured
// approvers) that are no longer resolved for the current languages. Manual
// assignees that are not in the approvers list are left untouched.
const staleAssignees = currentAssignees.filter(
(login) =>
configuredApprovers.has(login.toLowerCase()) && !resolvedLower.has(login.toLowerCase()),
);

if (newAssignees.length > 0) {
await github.rest.issues.addAssignees({
owner,
repo,
issue_number: issueNumber,
assignees: newAssignees,
});
core?.info?.(`Assigned reviewers: ${newAssignees.join(", ")}`);
}

if (staleAssignees.length > 0) {
await github.rest.issues.removeAssignees({
owner,
repo,
issue_number: issueNumber,
assignees: staleAssignees,
});
core?.info?.(`Removed stale reviewers: ${staleAssignees.join(", ")}`);
}

if (newAssignees.length === 0 && staleAssignees.length === 0) {
const reason = assignees.length === 0 ? "no-reviewers-resolved" : "already-assigned";
core?.info?.(
reason === "already-assigned"
? "Reviewers already up to date; no assignment changes."
: "No architect could be resolved for the selected languages.",
);
return { byLanguage, assigned: [], removed: [], unassigned, skipped: true, reason };
}

return {
byLanguage,
assigned: newAssignees,
removed: staleAssignees,
unassigned,
skipped: false,
reason: null,
};
}
56 changes: 52 additions & 4 deletions .github/workflows/src/arch-board-review/triage.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ import {
LANGUAGE_DEFINITIONS,
} from "./issue-parsing.js";
import { validateUrl as defaultValidateUrl } from "./url-validation.js";
import defaultAssignReviewers from "./assign-reviewers.js";
import { commentOrUpdate } from "../comment.js";
import { addLabels, ensureLabel, ensureLabelRemoved, removeLabel } from "../labels.js";

const COMMENT_IDENTIFIER = "arch-board-triage-bot";
const APPROVERS_FILE_URL =
"https://github.com/Azure/azure-sdk/blob/main/.github/api-review-approvers.yml";

function createValidationItem(kind, value) {
return `${kind}: ${value}`;
Expand Down Expand Up @@ -156,12 +159,35 @@ async function analyzeTriage(issueBody, currentLabels, { validateUrl = defaultVa
};
}

function buildSuccessComment({ selectedLanguages, validated, warnings }) {
function buildSuccessComment({
selectedLanguages,
validated,
warnings,
assignments,
unassigned,
assignmentFailed,
}) {
let body = `✅ **All materials verified for ${selectedLanguages.map((language) => language.label).join(", ")}.**\n\nThis review request is ready for architects. The \`ready-for-review\` label has been applied.\n\n`;
if (warnings.length > 0) {
body += `**Note:** ${warnings.join(", ")}\n\n`;
}

if (assignmentFailed) {
body += `⚠️ **Automatic reviewer assignment failed.** Please assign an architect manually - see the [architect approvers list](${APPROVERS_FILE_URL}).\n\n`;
} else if (assignments && assignments.length > 0) {
const assignedLine = assignments
.map(
(entry) =>
`${entry.language} → ${entry.reviewers.map((reviewer) => `\`${reviewer}\``).join(", ")}`,
)
.join(" · ");
body += `**Assigned for review:** ${assignedLine}\n\nAssigned architects are notified via GitHub. Apply your \`<language>-api-approved\` label when the review is complete.\n\n`;
}

if (unassigned && unassigned.length > 0) {
body += `⚠️ **No architect is configured for: ${unassigned.join(", ")}.** These languages need manual assignment - please assign a reviewer or add one to the [architect approvers list](${APPROVERS_FILE_URL}).\n\n`;
}

body += "<details><summary>Validation details</summary>\n\n";
for (const entry of validated) {
body += `**${entry.language}:**\n`;
Expand Down Expand Up @@ -202,7 +228,13 @@ function buildFailureComment({ missing, validated }) {

export { COMMENT_IDENTIFIER, analyzeTriage, buildFailureComment, buildSuccessComment };

export default async function triage({ github, context, core, validateUrl = defaultValidateUrl }) {
export default async function triage({
github,
context,
core,
validateUrl = defaultValidateUrl,
assignReviewers = defaultAssignReviewers,
}) {
const issue = context.payload.issue;
const issueBody = issue.body ?? "";
const issueNumber = issue.number;
Expand Down Expand Up @@ -234,10 +266,26 @@ export default async function triage({ github, context, core, validateUrl = defa
await ensureLabel(github, owner, repo, issueNumber, "ready-for-review", currentLabels);
await ensureLabelRemoved(github, owner, repo, issueNumber, "needs-info", currentLabels);

await postComment(buildSuccessComment(result));
let assignment = null;
let assignmentFailed = false;
try {
assignment = await assignReviewers({ github, context, core });
} catch (error) {
assignmentFailed = true;
core?.warning?.(`Reviewer assignment failed: ${error.message}`);
}

await postComment(
buildSuccessComment({
...result,
assignments: assignment?.byLanguage ?? [],
unassigned: assignment?.unassigned ?? [],
assignmentFailed,
}),
);

core?.info?.("Review request is ready for review.");
return { ...result, status: "ready-for-review" };
return { ...result, status: "ready-for-review", assignment };
}

await ensureLabel(github, owner, repo, issueNumber, "needs-info", currentLabels);
Expand Down
Loading
Loading