From 41996f7b4331f798a31b12d1cdf6dc9ffb1d3bf7 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Thu, 18 Jun 2026 10:57:27 -0700 Subject: [PATCH 1/4] Update ADO work item at PR creation --- scripts/api_md_workflow/README.md | 210 ++++++++- .../api_md_workflow/create_api_review_pr.py | 138 +++++- .../create_api_review_pr_test.py | 398 +++--------------- 3 files changed, 408 insertions(+), 338 deletions(-) diff --git a/scripts/api_md_workflow/README.md b/scripts/api_md_workflow/README.md index 4d96401c29a5..57842187f67a 100644 --- a/scripts/api_md_workflow/README.md +++ b/scripts/api_md_workflow/README.md @@ -1,21 +1,217 @@ # API Review PR Helper -This folder contains the standalone Python helper used to create API review PRs from generated `api.md` files. +This folder contains the standalone helper used to create API review pull requests from generated `api.md` files. ## Purpose -`create_api_review_pr.py` compares a baseline package release tag with a target API surface, creates or reuses dedicated API review branches, and opens a draft PR that shows the `api.md` diff. +`create_api_review_pr.py` automates API review PR creation for a package by: + +1. Generating baseline and target API markdown snapshots. +2. Creating or reusing dedicated baseline/review branches. +3. Creating or reusing a draft GitHub PR with the `api.md` diff. +4. Updating PR body sync metadata so future automation can identify branch relationships. +5. Updating the package ADO work item `Custom.PendingAPIReviews` field with the review PR URL. The API consistency workflow helpers live under `.github/workflows/src/api-md-consistency`. -## Usage +## Inputs and Output + +Required inputs: + +- `--package-name`: package folder name, such as `azure-ai-projects`. +- `--base`: baseline package tag, required to match `_` and exist locally/remotely. + +Optional inputs: + +- `--target`: one of: + - package tag (for static tag-to-tag reviews), + - branch name on `origin`, + - `owner:branch` fork branch reference. +- `--python` / `--runtime`: runtime executable used for `azpysdk apistub` generation. + +Primary output: + +- A draft PR URL representing baseline vs review branch `api.md` diff. + +Secondary output: + +- `Custom.PendingAPIReviews` on the package work item is updated to include the PR URL (idempotent append). + +## High-Level Flow + +`create_api_review_pr.py` executes this logical sequence: + +1. Validate environment and arguments. +2. Resolve package directory. +3. Capture baseline `api.md` + `api.metadata.yml` from `--base` tag. +4. Capture target `api.md` + `api.metadata.yml` from resolved target ref. +5. Exit early if baseline and target `api.md` are identical. +6. Resolve branch reuse or branch creation for baseline and review branches. +7. Create or reuse PR between baseline and review branches. +8. Ensure PR sync metadata block in PR body. +9. Update package work item `Custom.PendingAPIReviews` with PR URL. +10. Restore original local branch. + +## Detailed Decision Paths + +### 1) Target Resolution + +`--target` resolution order and behavior: + +- If target is omitted: use `origin/main`. +- If target looks like package tag and exists as tag: use tag. +- Else if target is plain branch and exists on `origin`: use `origin/`. +- Else if target is `owner:branch` and exists on fork: use `FETCH_HEAD`. +- Else: fail. + +Implication for PR body labeling: + +- Tag target: labeled as `Target tag` and called out as static tag-to-tag review. +- Branch target: labeled as `Working branch` or `Working PR` if a matching open PR exists. + +### 2) API Snapshot Capture + +For both baseline and target refs, the script overlays package files from that ref, then runs `azpysdk apistub --md --extract-metadata`. + +Captured artifacts: + +- `api.md` bytes (required). +- `api.metadata.yml` bytes (optional but expected for metadata hash checks). +- parsed package version from `_version.py` or `version.py`. + +After each capture, the script resets package files in the working tree to avoid local drift. + +### 3) API Difference Gate + +Diff condition is intentionally narrow: + +- If `base.api_md == target.api_md`: no branches or PR are created. +- Metadata-only differences do not trigger PR creation. + +### 4) Branch Reuse vs Branch Creation + +Branch naming convention: + +- baseline: `apireview/base__` +- review: `apireview/review__` + +Reuse logic: + +- Enumerate existing remote branches with same prefix. +- Read branch state (`api.md`, `api.metadata.yml`, `apiMdSha256`). +- Reuse branch only when branch state matches desired state. +- For review branch, required ancestor must include selected baseline branch. + +Creation logic: + +- Baseline branch starts from `origin/main` and commits baseline `api.md`/metadata. +- Review branch starts from selected baseline branch and commits target `api.md`/metadata. +- Both are pushed with `--force-with-lease`. + +### 5) PR Reuse vs PR Creation + +If both branches are reused: + +- Search for existing open PR matching baseline/review pair. +- If found, reuse PR and update body sync metadata block if stale. -The script includes Python package discovery, version parsing, `api.md` generation, git branch orchestration, and GitHub PR creation in one file. +Otherwise: -`create_api_review_pr.py` compares a baseline package release tag with a target API surface. The target can be a package release tag, an `origin` branch, or an `owner:branch` fork reference. When the target is a tag, the generated PR body identifies it as a target tag instead of a working branch. +- Attempt draft PR creation via GitHub REST API. +- If creation fails, search for already-open PR as fallback. +- If fallback PR exists, reuse it. +- If no PR is found, log manual compare URL and diagnostics. + +## PR Body Sync Metadata + +The PR body can include a hidden metadata block (`api-md-review-sync`) with: + +- repository slug, +- package name and dir, +- baseline/review branch names, +- working branch owner/name, +- optional working PR number. + +Behavior: + +- Existing stale sync block is replaced. +- Block is omitted for target-tag flows (no working branch to track). + +## ADO Package Work Item Update Flow + +After a PR URL is available (created or reused), the script updates the package work item: + +1. Fetch package work item via: + +```bash +azsdk package get-work-item --package-name -o json +``` + +2. Extract: + +- work item id: `id` +- pending review markdown field: `fields.Custom.PendingAPIReviews` + +3. Parse existing field value as newline-separated URL list. + +4. Append PR URL only when missing (idempotent behavior). + +5. Join list with newlines and update work item: + +```bash +azsdk package update-work-item \ + --work-item-id \ + --field "Custom.PendingAPIReviews=" \ + --multiline-fields-format "Custom.PendingAPIReviews=markdown" +``` + +Failure policy: + +- Work item update failures are logged as warnings. +- PR flow still succeeds (best-effort work item synchronization). + +## Error Handling Strategy + +Hard failures: + +- dirty working tree, +- detached HEAD, +- invalid/missing base tag, +- unresolved target reference, +- missing required API artifact generation. + +Soft failures (warning and continue): + +- inability to update PR body sync metadata, +- inability to update package work item pending review URLs, +- draft PR creation failure when existing PR fallback succeeds. + +## Example Usage + + +Baseline tag vs fork working branch: + +Release from main review (most common): + +```bash +python scripts/api_md_workflow/create_api_review_pr.py \ + --package-name azure-ai-projects \ + --base azure-ai-projects_2.1.0 \ + --target main +``` + +```bash +python scripts/api_md_workflow/create_api_review_pr.py \ + --package-name azure-ai-projects \ + --base azure-ai-projects_2.1.0 \ + --target someuser:feature/api-changes +``` -Example comparing two package release tags: +Tag-to-tag review (uncommon): ```bash -python scripts/api_md_workflow/create_api_review_pr.py --package-name azure-ai-projects --base azure-ai-projects_2.1.0 --target azure-ai-projects_2.2.0 +python scripts/api_md_workflow/create_api_review_pr.py \ + --package-name azure-ai-projects \ + --base azure-ai-projects_2.1.0 \ + --target azure-ai-projects_2.2.0 ``` diff --git a/scripts/api_md_workflow/create_api_review_pr.py b/scripts/api_md_workflow/create_api_review_pr.py index 057fd406a30d..16bc9dc18566 100644 --- a/scripts/api_md_workflow/create_api_review_pr.py +++ b/scripts/api_md_workflow/create_api_review_pr.py @@ -68,6 +68,12 @@ class ArchitectReviewers: teams: list[str] +@dataclass +class PackageWorkItem: + id: int + pending_api_review_urls: list[str] + + GitRunner = Callable[[list[str], bool], CommandResult] _git_runner: GitRunner | None = None _github_api: "GitHubApi | None" = None @@ -176,7 +182,7 @@ def normalize_pull_request(pr: dict[str, Any] | None) -> dict[str, Any] | None: pr["head"].get("repo") if isinstance(pr["head"].get("repo"), dict) else {} ) owner = repo.get("owner") if isinstance(repo.get("owner"), dict) else {} - owner_login = owner.get("login") + owner_login = owner.get("login") if isinstance(owner, dict) else None if isinstance(pr.get("author"), dict): author_login = pr["author"].get("login") elif isinstance(pr.get("user"), dict): @@ -849,6 +855,127 @@ def architects_for_package( return matching_architects +def parse_pending_api_review_urls(markdown_urls: str | None) -> list[str]: + if not markdown_urls: + return [] + return [line.strip() for line in markdown_urls.splitlines() if line.strip()] + + +def dedupe_preserve_order(values: list[str]) -> list[str]: + seen: set[str] = set() + deduped: list[str] = [] + for value in values: + cleaned = value.strip() + if not cleaned or cleaned in seen: + continue + seen.add(cleaned) + deduped.append(cleaned) + return deduped + + +def get_package_work_item(package_name: str) -> PackageWorkItem: + result = run( + [ + "azsdk", + "package", + "get-work-item", + "--package-name", + package_name, + "-o", + "json", + ], + check=False, + capture=True, + ) + if result.status != 0: + raise RuntimeError( + "Failed to get package work item via `azsdk package get-work-item -o json`." + + (f"\n stderr: {result.stderr.strip()}" if result.stderr.strip() else "") + ) + + try: + data = json.loads(result.stdout) + except json.JSONDecodeError as error: + raise RuntimeError( + f"Failed to parse package work item JSON: {error}" + ) from error + + raw_work_item_id = data.get("id") + if raw_work_item_id is None: + raise RuntimeError("Package work item response did not contain an 'id'.") + + try: + work_item_id = int(raw_work_item_id) + except (TypeError, ValueError) as error: + raise RuntimeError( + f"Invalid package work item id: {raw_work_item_id}" + ) from error + + fields = data.get("fields") if isinstance(data, dict) else None + pending_reviews_markdown = "" + if isinstance(fields, dict): + pending_reviews_markdown = str(fields.get("Custom.PendingAPIReviews") or "") + + return PackageWorkItem( + id=work_item_id, + pending_api_review_urls=parse_pending_api_review_urls(pending_reviews_markdown), + ) + + +def update_package_pending_api_reviews( + work_item_id: int, api_reviews_string: str +) -> None: + result = run( + [ + "azsdk", + "package", + "update-work-item", + "--work-item-id", + str(work_item_id), + "--field", + f"Custom.PendingAPIReviews={api_reviews_string}", + "--multiline-fields-format", + "Custom.PendingAPIReviews=markdown", + ], + check=False, + capture=True, + ) + if result.status != 0: + raise RuntimeError( + f"Failed to update package work item {work_item_id} Pending API Reviews field." + + (f"\n stderr: {result.stderr.strip()}" if result.stderr.strip() else "") + ) + + +def sync_package_pending_api_review_pr_url(package_name: str, pr_url: str) -> None: + if not pr_url.strip(): + log_warning("WARNING: empty PR URL; skipping package work item update.") + return + + try: + package_work_item = get_package_work_item(package_name) + updated_urls = dedupe_preserve_order( + [*package_work_item.pending_api_review_urls, pr_url] + ) + if updated_urls == package_work_item.pending_api_review_urls: + log_info( + f"Package work item {package_work_item.id} already contains API review PR URL; no update needed." + ) + return + + api_reviews_string = "\n".join(updated_urls) + update_package_pending_api_reviews(package_work_item.id, api_reviews_string) + log_info( + f"Updated package work item {package_work_item.id} with API review PR URL." + ) + except Exception as error: # pylint: disable=broad-except + details = str(error) + log_warning( + "WARNING: failed to update package work item Pending API Reviews." + + (f"\n {details}" if details else "") + ) + + def branch_remote_ref(branch: str) -> str: return f"{REMOTE}/{branch}" @@ -1521,6 +1648,9 @@ def main(argv: list[str] | None = None) -> int: existing_pr.get("authorLogin"), architect_reviewers, ) + sync_package_pending_api_review_pr_url( + args.package_name, str(existing_pr["url"]) + ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 @@ -1537,6 +1667,9 @@ def main(argv: list[str] | None = None) -> int: ) if pr_create.get("url"): log_info(pr_create["url"]) + sync_package_pending_api_review_pr_url( + args.package_name, str(pr_create["url"]) + ) else: existing_pr = find_open_pr_for_branches(base_branch, review_branch) if existing_pr: @@ -1547,6 +1680,9 @@ def main(argv: list[str] | None = None) -> int: existing_pr.get("authorLogin"), architect_reviewers, ) + sync_package_pending_api_review_pr_url( + args.package_name, str(existing_pr["url"]) + ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 diff --git a/scripts/api_md_workflow/create_api_review_pr_test.py b/scripts/api_md_workflow/create_api_review_pr_test.py index a0e2e45a201f..51eeb4015665 100644 --- a/scripts/api_md_workflow/create_api_review_pr_test.py +++ b/scripts/api_md_workflow/create_api_review_pr_test.py @@ -1,7 +1,5 @@ import json -import tempfile import unittest -from pathlib import Path from unittest.mock import MagicMock, patch from scripts.api_md_workflow import create_api_review_pr as workflow @@ -31,7 +29,6 @@ def __init__(self, head_results=None, search_results=None, on_lookup=None): self.head_results = head_results or [] self.search_results = search_results or [] self.on_lookup = on_lookup - self.reviewers = [] def _lookup(self, results): if self.on_lookup: @@ -51,13 +48,7 @@ def update_pull_request_body(self, _number, _body): return None def create_draft_pull_request(self, _base, _head, _title, _body): - return { - "number": 1, - "html_url": "https://github.com/Azure/azure-sdk-for-python/pull/1", - } - - def request_reviewers(self, pr_number, reviewers, team_reviewers=None): - self.reviewers.append((pr_number, reviewers, team_reviewers or [])) + return {"html_url": "https://github.com/Azure/azure-sdk-for-python/pull/1"} class ApiReviewPrTests(unittest.TestCase): @@ -157,8 +148,7 @@ def on_lookup(): workflow.target_reference_info("azure-example_1.2.3"), { "label": "Target tag", - "markdown": "[tag `azure-example_1.2.3`]" - "(https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", + "markdown": "[tag `azure-example_1.2.3`](https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", }, ) self.assertEqual(pr_lookup_count, 0) @@ -202,8 +192,7 @@ def on_lookup(): workflow.target_reference_info("azure-example_1.2.3", "azure-example"), { "label": "Target tag", - "markdown": "[tag `azure-example_1.2.3`]" - "(https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", + "markdown": "[tag `azure-example_1.2.3`](https://github.com/Azure/azure-sdk-for-python/commit/abc123def456)", }, ) self.assertEqual(pr_lookup_count, 0) @@ -233,13 +222,11 @@ def test_build_sync_metadata_object_records_fork_owner_and_branch(self): base_branch="apireview/base_azure-example_1.0.0", review_branch="apireview/review_azure-example_1.1.0", head_selector="example:users/example/feature", - package_work_item_id_value=31370, ) self.assertEqual(metadata["workingOwner"], "example") self.assertEqual(metadata["workingBranch"], "users/example/feature") self.assertEqual(metadata["workingPrNumber"], 47204) - self.assertEqual(metadata["packageWorkItemId"], 31370) def test_build_sync_metadata_object_omits_metadata_for_tag_targets(self): pr_lookup_count = 0 @@ -302,7 +289,6 @@ def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews( "workingOwner": "Azure", "workingBranch": "main", "workingPrNumber": None, - "packageWorkItemId": "31370", } ) @@ -322,7 +308,6 @@ def test_build_review_pr_body_includes_sync_metadata_for_working_branch_reviews( self.assertNotIn("Static tag-to-tag review", body) self.assertIn("`, + "m" + ); + const match = pattern.exec(body || ""); + if (!match) { + return null; + } + + const jsonText = match[1] + .split(/\r?\n/) + .filter((line) => line.trim() !== METADATA_WARNING) + .join("\n") + .trim(); + + const metadata = parseJson(jsonText, `${marker} metadata block`); + if (!metadata || typeof metadata !== "object" || Array.isArray(metadata)) { + throw new Error(`${marker} metadata block must contain a JSON object.`); + } + return metadata; +} + +function normalizeUrlList(value) { + if (!value) { + return []; + } + + const seen = new Set(); + const urls = []; + for (const line of String(value).split(/\r?\n/)) { + const url = line.trim(); + if (!url || seen.has(url)) { + continue; + } + seen.add(url); + urls.push(url); + } + return urls; +} + +function packageNameFromMetadata(metadata) { + if (typeof metadata.packageName === "string" && metadata.packageName.trim()) { + return metadata.packageName.trim(); + } + + if (typeof metadata.packageDir === "string" && metadata.packageDir.trim()) { + return path.basename(metadata.packageDir.replace(/\\/g, "/")); + } + + return ""; +} + +function getPackageWorkItem(metadata) { + const rawWorkItemId = metadata.packageWorkItemId; + if (rawWorkItemId !== undefined && rawWorkItemId !== null && rawWorkItemId !== "" && rawWorkItemId !== "ERROR") { + const workItemId = Number(rawWorkItemId); + if (!Number.isInteger(workItemId)) { + throw new Error(`Invalid packageWorkItemId in API review metadata: ${rawWorkItemId}`); + } + + return parseJson( + runAzsdkCli(["package", "get-work-item", "--work-item-id", String(workItemId), "-o", "json"]), + `package work item ${workItemId}` + ); + } + + const packageName = packageNameFromMetadata(metadata); + if (!packageName) { + throw new Error("API review metadata must include packageWorkItemId, packageName, or packageDir."); + } + + return parseJson( + runAzsdkCli(["package", "get-work-item", "--package-name", packageName, "-o", "json"]), + `package work item for ${packageName}` + ); +} + +function workItemId(workItem) { + const id = Number(workItem?.id); + if (!Number.isInteger(id)) { + throw new Error("Package work item response did not contain an integer id."); + } + return id; +} + +function pendingReviewUrls(workItem, fieldName) { + return normalizeUrlList(workItem?.fields?.[fieldName]); +} + +function updatePendingReviews(workItem, fieldName, urls) { + runAzsdkCli([ + "package", + "update-work-item", + "--work-item-id", + String(workItemId(workItem)), + "--field", + `${fieldName}=${urls.join("\n")}`, + "--multiline-fields-format", + `${fieldName}=markdown`, + ]); +} + +function updatedUrlsForAction(action, urls, prUrl) { + if (action === "reopened") { + return urls.includes(prUrl) ? urls : [...urls, prUrl]; + } + + if (action === "closed") { + return urls.filter((url) => url !== prUrl); + } + + throw new Error(`Unsupported pull request action: ${action}`); +} + +function isReviewPullRequest(pullRequest, marker = DEFAULT_METADATA_MARKER) { + return Boolean( + parseSyncMetadata(pullRequest.body || "", marker) || + pullRequest.title?.startsWith("[API Review]") || + pullRequest.head?.ref?.startsWith("apireview/review") + ); +} + +function main() { + const event = parseEvent(); + const action = process.env.API_REVIEW_PR_ACTION || event.action; + const pullRequest = event.pull_request; + const fieldName = process.env.PENDING_API_REVIEWS_FIELD || DEFAULT_PENDING_REVIEWS_FIELD; + const marker = process.env.API_REVIEW_METADATA_MARKER || DEFAULT_METADATA_MARKER; + + if (!pullRequest) { + warn("Event does not contain a pull_request payload; skipping."); + return; + } + + if (action !== "closed" && action !== "reopened") { + warn(`Pull request action ${action} does not affect pending API reviews; skipping.`); + return; + } + + if (!isReviewPullRequest(pullRequest, marker)) { + warn("Pull request does not look like an API review PR; skipping."); + return; + } + + const metadata = parseSyncMetadata(pullRequest.body || "", marker); + if (!metadata) { + warn(`Pull request does not contain ${marker} metadata; skipping package work item update.`); + return; + } + + const prUrl = process.env.API_REVIEW_PR_URL || pullRequest.html_url; + if (!prUrl) { + throw new Error("Pull request URL is required."); + } + + const workItem = getPackageWorkItem(metadata); + const existingUrls = pendingReviewUrls(workItem, fieldName); + const nextUrls = updatedUrlsForAction(action, existingUrls, prUrl); + + if (nextUrls.join("\n") === existingUrls.join("\n")) { + log(`Package work item ${workItemId(workItem)} already has desired ${fieldName} value.`); + return; + } + + updatePendingReviews(workItem, fieldName, nextUrls); + log(`Updated package work item ${workItemId(workItem)} ${fieldName} for PR ${prUrl}.`); +} + +main(); From d1cc840827be90fa9a89910ee9b7eb5a46d22c5b Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Thu, 18 Jun 2026 15:17:49 -0700 Subject: [PATCH 4/4] Remove ADO update from creation script. Move into Github Action. --- .github/workflows/api-review-pending.yml | 2 +- eng/common/scripts/sync-api-review-pending.js | 44 +++----- scripts/api_md_workflow/README.md | 31 ++---- .../api_md_workflow/create_api_review_pr.py | 101 +----------------- .../create_api_review_pr_test.py | 71 ++---------- 5 files changed, 38 insertions(+), 211 deletions(-) diff --git a/.github/workflows/api-review-pending.yml b/.github/workflows/api-review-pending.yml index 645214757cac..7b6a6d4ed652 100644 --- a/.github/workflows/api-review-pending.yml +++ b/.github/workflows/api-review-pending.yml @@ -3,7 +3,7 @@ name: API Review Pending Work Item Sync on: pull_request_target: # Merged pull requests are delivered as the closed action with pull_request.merged == true. - types: [closed, reopened] + types: [opened, closed, reopened] workflow_call: inputs: event-path: diff --git a/eng/common/scripts/sync-api-review-pending.js b/eng/common/scripts/sync-api-review-pending.js index 42e387ee24e3..eb9225bc8f1b 100644 --- a/eng/common/scripts/sync-api-review-pending.js +++ b/eng/common/scripts/sync-api-review-pending.js @@ -2,7 +2,6 @@ const { execFileSync } = require("child_process"); const fs = require("fs"); -const path = require("path"); const DEFAULT_METADATA_MARKER = "api-md-review-sync"; const DEFAULT_PENDING_REVIEWS_FIELD = "Custom.PendingAPIReviews"; @@ -88,40 +87,20 @@ function normalizeUrlList(value) { return urls; } -function packageNameFromMetadata(metadata) { - if (typeof metadata.packageName === "string" && metadata.packageName.trim()) { - return metadata.packageName.trim(); - } - - if (typeof metadata.packageDir === "string" && metadata.packageDir.trim()) { - return path.basename(metadata.packageDir.replace(/\\/g, "/")); - } - - return ""; -} - function getPackageWorkItem(metadata) { const rawWorkItemId = metadata.packageWorkItemId; - if (rawWorkItemId !== undefined && rawWorkItemId !== null && rawWorkItemId !== "" && rawWorkItemId !== "ERROR") { - const workItemId = Number(rawWorkItemId); - if (!Number.isInteger(workItemId)) { - throw new Error(`Invalid packageWorkItemId in API review metadata: ${rawWorkItemId}`); - } - - return parseJson( - runAzsdkCli(["package", "get-work-item", "--work-item-id", String(workItemId), "-o", "json"]), - `package work item ${workItemId}` - ); + if (rawWorkItemId === undefined || rawWorkItemId === null || rawWorkItemId === "" || rawWorkItemId === "ERROR") { + return null; } - const packageName = packageNameFromMetadata(metadata); - if (!packageName) { - throw new Error("API review metadata must include packageWorkItemId, packageName, or packageDir."); + const workItemId = Number(rawWorkItemId); + if (!Number.isInteger(workItemId)) { + throw new Error(`Invalid packageWorkItemId in API review metadata: ${rawWorkItemId}`); } return parseJson( - runAzsdkCli(["package", "get-work-item", "--package-name", packageName, "-o", "json"]), - `package work item for ${packageName}` + runAzsdkCli(["package", "get-work-item", "--work-item-id", String(workItemId), "-o", "json"]), + `package work item ${workItemId}` ); } @@ -151,7 +130,7 @@ function updatePendingReviews(workItem, fieldName, urls) { } function updatedUrlsForAction(action, urls, prUrl) { - if (action === "reopened") { + if (action === "opened" || action === "reopened") { return urls.includes(prUrl) ? urls : [...urls, prUrl]; } @@ -182,7 +161,7 @@ function main() { return; } - if (action !== "closed" && action !== "reopened") { + if (action !== "opened" && action !== "closed" && action !== "reopened") { warn(`Pull request action ${action} does not affect pending API reviews; skipping.`); return; } @@ -204,6 +183,11 @@ function main() { } const workItem = getPackageWorkItem(metadata); + if (!workItem) { + warn("API review metadata does not contain a packageWorkItemId; skipping package work item update."); + return; + } + const existingUrls = pendingReviewUrls(workItem, fieldName); const nextUrls = updatedUrlsForAction(action, existingUrls, prUrl); diff --git a/scripts/api_md_workflow/README.md b/scripts/api_md_workflow/README.md index 2b0984d89b72..011068fdfe4a 100644 --- a/scripts/api_md_workflow/README.md +++ b/scripts/api_md_workflow/README.md @@ -11,7 +11,7 @@ This folder contains the standalone helper used to create API review pull reques 3. Creating or reusing a draft GitHub PR with the `api.md` diff. 4. Assigning the PR to API review architects resolved from `.github/ARCHITECTS`. 5. Updating PR body sync metadata so future automation can identify branch relationships. -6. Updating the package ADO work item `Custom.PendingAPIReviews` field with the review PR URL. +6. Letting the API review pending workflow update the package ADO work item from PR events. The API consistency workflow helpers live under `.github/workflows/src/api-md-consistency`. @@ -36,7 +36,7 @@ Primary output: Secondary output: -- `Custom.PendingAPIReviews` on the package work item is updated to include the PR URL (idempotent append). +- PR body sync metadata includes the package work item ID when available, so `.github/workflows/api-review-pending.yml` can synchronize `Custom.PendingAPIReviews` on PR open, close, and reopen events. - Matching API review architects are requested as GitHub reviewers when a PR number is available. ## High-Level Flow @@ -49,12 +49,11 @@ Secondary output: 4. Capture target `api.md` + `api.metadata.yml` from resolved target ref. 5. Exit early if baseline and target `api.md` are identical. 6. Resolve branch reuse or branch creation for baseline and review branches. -7. Fetch the package work item once for working-branch sync flows. +7. Fetch the package work item ID for working-branch sync metadata. 8. Create or reuse PR between baseline and review branches. 9. Ensure PR sync metadata block in PR body. 10. Request API review architects as PR reviewers. -11. Update package work item `Custom.PendingAPIReviews` with PR URL. -12. Restore original local branch. +11. Restore original local branch. ## Detailed Decision Paths @@ -156,9 +155,9 @@ Behavior: - Existing stale sync block is replaced. - Block is omitted for target-tag flows (no working branch to track). -## ADO Package Work Item Update Flow +## ADO Package Work Item Sync Flow -For working-branch sync flows, the script fetches the package work item once before creating or reusing the API review PR: +For working-branch sync flows, the script fetches the package work item before creating or reusing the API review PR: 1. Fetch package work item via: @@ -166,18 +165,11 @@ For working-branch sync flows, the script fetches the package work item once bef azsdk package get-work-item --package-name -o json ``` -2. Extract: +2. Extract the work item `id` and write it to PR body sync metadata as `packageWorkItemId`. -- work item id: `id` -- pending review markdown field: `fields.Custom.PendingAPIReviews` +The `.github/workflows/api-review-pending.yml` workflow owns `Custom.PendingAPIReviews` updates. On API review PR `opened` or `reopened`, it appends the PR URL if missing. On `closed`, it removes the PR URL. -The same fetched work item is reused for both PR body sync metadata (`packageWorkItemId`) and the later pending-review update. After a PR URL is available (created or reused), the script: - -3. Parses the existing pending review field value as a newline-separated URL list. - -4. Appends the PR URL only when missing (idempotent behavior). - -5. Joins the list with newlines and updates the work item: +The workflow updates the work item with: ```bash azsdk package update-work-item \ @@ -188,9 +180,8 @@ azsdk package update-work-item \ Failure policy: -- Work item fetch failures set sync metadata `packageWorkItemId` to `ERROR` and skip the pending-review update. -- Work item update failures are logged as warnings. -- PR flow still succeeds (best-effort work item synchronization). +- Work item fetch failures set sync metadata `packageWorkItemId` to `ERROR`. +- PR flow still succeeds; the event-driven workflow skips the package work item update when `packageWorkItemId` is unavailable. ## Error Handling Strategy diff --git a/scripts/api_md_workflow/create_api_review_pr.py b/scripts/api_md_workflow/create_api_review_pr.py index ccb0d8dcccf5..4d4bf286d51e 100644 --- a/scripts/api_md_workflow/create_api_review_pr.py +++ b/scripts/api_md_workflow/create_api_review_pr.py @@ -71,7 +71,6 @@ class ArchitectReviewers: @dataclass class PackageWorkItem: id: int - pending_api_review_urls: list[str] GitRunner = Callable[[list[str], bool], CommandResult] @@ -627,7 +626,7 @@ def ensure_azsdk_work_item_commands_available() -> None: azsdk_executable = resolve_azsdk_executable() missing_commands: list[str] = [] command_details: list[str] = [] - for command_name in ["get-work-item", "update-work-item"]: + for command_name in ["get-work-item"]: result = run( [azsdk_executable, "package", command_name, "--help"], check=False, @@ -798,24 +797,6 @@ def architects_for_package( return matching_architects -def parse_pending_api_review_urls(markdown_urls: str | None) -> list[str]: - if not markdown_urls: - return [] - return [line.strip() for line in markdown_urls.splitlines() if line.strip()] - - -def dedupe_preserve_order(values: list[str]) -> list[str]: - seen: set[str] = set() - deduped: list[str] = [] - for value in values: - cleaned = value.strip() - if not cleaned or cleaned in seen: - continue - seen.add(cleaned) - deduped.append(cleaned) - return deduped - - def get_package_work_item(package_name: str) -> PackageWorkItem: result = run( [ @@ -854,75 +835,7 @@ def get_package_work_item(package_name: str) -> PackageWorkItem: f"Invalid package work item id: {raw_work_item_id}" ) from error - fields = data.get("fields") if isinstance(data, dict) else None - pending_reviews_markdown = "" - if isinstance(fields, dict): - pending_reviews_markdown = str(fields.get("Custom.PendingAPIReviews") or "") - - return PackageWorkItem( - id=work_item_id, - pending_api_review_urls=parse_pending_api_review_urls(pending_reviews_markdown), - ) - - -def update_package_pending_api_reviews( - work_item_id: int, api_reviews_string: str -) -> None: - result = run( - [ - "azsdk", - "package", - "update-work-item", - "--work-item-id", - str(work_item_id), - "--field", - f"Custom.PendingAPIReviews={api_reviews_string}", - "--multiline-fields-format", - "Custom.PendingAPIReviews=markdown", - ], - check=False, - capture=True, - ) - if result.status != 0: - raise RuntimeError( - f"Failed to update package work item {work_item_id} Pending API Reviews field." - + (f"\n stderr: {result.stderr.strip()}" if result.stderr.strip() else "") - ) - - -def update_package_pending_reviews( - package_work_item: PackageWorkItem | None, pr_url: str -) -> None: - if not pr_url.strip(): - log_warning("WARNING: empty PR URL; skipping package work item update.") - return - if package_work_item is None: - log_warning( - "WARNING: package work item is unavailable; skipping Pending API Reviews update." - ) - return - - try: - updated_urls = dedupe_preserve_order( - [*package_work_item.pending_api_review_urls, pr_url] - ) - if updated_urls == package_work_item.pending_api_review_urls: - log_info( - f"Package work item {package_work_item.id} already contains API review PR URL; no update needed." - ) - return - - api_reviews_string = "\n".join(updated_urls) - update_package_pending_api_reviews(package_work_item.id, api_reviews_string) - log_info( - f"Updated package work item {package_work_item.id} with API review PR URL." - ) - except Exception as error: # pylint: disable=broad-except - details = str(error) - log_warning( - "WARNING: failed to update package work item Pending API Reviews." - + (f"\n {details}" if details else "") - ) + return PackageWorkItem(id=work_item_id) def branch_remote_ref(branch: str) -> str: @@ -1557,7 +1470,6 @@ def main(argv: list[str] | None = None) -> int: working_reference = target_reference_info(working_selector, args.package_name) baseline_ref = baseline_reference_markdown(args.base) work_item_metadata_value: int | str | None = None - package_work_item: PackageWorkItem | None = None if sync_working_branch_info(working_selector, args.package_name): try: package_work_item = get_package_work_item(args.package_name) @@ -1598,9 +1510,6 @@ def main(argv: list[str] | None = None) -> int: existing_pr.get("authorLogin"), architect_reviewers, ) - update_package_pending_reviews( - package_work_item, str(existing_pr["url"]) - ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 @@ -1617,9 +1526,6 @@ def main(argv: list[str] | None = None) -> int: ) if pr_create.get("url"): log_info(pr_create["url"]) - update_package_pending_reviews( - package_work_item, str(pr_create["url"]) - ) else: existing_pr = find_open_pr_for_branches(base_branch, review_branch) if existing_pr: @@ -1630,9 +1536,6 @@ def main(argv: list[str] | None = None) -> int: existing_pr.get("authorLogin"), architect_reviewers, ) - update_package_pending_reviews( - package_work_item, str(existing_pr["url"]) - ) log_info(f"\n=== Reusing existing PR #{existing_pr['number']} ===") log_info(existing_pr["url"]) return 0 diff --git a/scripts/api_md_workflow/create_api_review_pr_test.py b/scripts/api_md_workflow/create_api_review_pr_test.py index 0eea8fae4c44..02b754556207 100644 --- a/scripts/api_md_workflow/create_api_review_pr_test.py +++ b/scripts/api_md_workflow/create_api_review_pr_test.py @@ -370,19 +370,9 @@ def test_sync_metadata_block_json_is_parseable(self): json.loads(json_text), {"schemaVersion": 1, "workingPrNumber": None} ) - def test_parse_pending_api_review_urls_splits_and_trims_lines(self): - markdown = "\nhttps://contoso/review/1\n https://contoso/review/2 \n\n" - self.assertEqual( - workflow.parse_pending_api_review_urls(markdown), - ["https://contoso/review/1", "https://contoso/review/2"], - ) - - def test_get_package_work_item_parses_pending_api_review_field(self): + def test_get_package_work_item_parses_id(self): payload = { "id": 12345, - "fields": { - "Custom.PendingAPIReviews": "https://contoso/review/1\nhttps://contoso/review/2\n" - }, } with patch.object( @@ -393,10 +383,6 @@ def test_get_package_work_item_parses_pending_api_review_field(self): item = workflow.get_package_work_item("azure-example") self.assertEqual(item.id, 12345) - self.assertEqual( - item.pending_api_review_urls, - ["https://contoso/review/1", "https://contoso/review/2"], - ) run_mock.assert_called_once_with( [ "azsdk", @@ -411,7 +397,7 @@ def test_get_package_work_item_parses_pending_api_review_field(self): capture=True, ) - def test_ensure_azsdk_work_item_commands_available_checks_get_and_update(self): + def test_ensure_azsdk_work_item_commands_available_checks_get(self): with patch.object(workflow, "resolve_azsdk_executable", return_value="azsdk"): with patch.object( workflow, @@ -427,57 +413,20 @@ def test_ensure_azsdk_work_item_commands_available_checks_get_and_update(self): ["azsdk", "package", "get-work-item", "--help"], check=False, capture=True, - ), - unittest.mock.call( - ["azsdk", "package", "update-work-item", "--help"], - check=False, - capture=True, - ), + ) ], ) - def test_ensure_azsdk_work_item_commands_available_reports_missing_update(self): - def run_stub(args, **_kwargs): - if args[2] == "update-work-item": - return workflow.CommandResult(1, "", "missing command") - return workflow.CommandResult(0, "", "") - + def test_ensure_azsdk_work_item_commands_available_reports_missing_get(self): with patch.object(workflow, "resolve_azsdk_executable", return_value="azsdk"): - with patch.object(workflow, "run", side_effect=run_stub): - with self.assertRaisesRegex(RuntimeError, "package update-work-item"): + with patch.object( + workflow, + "run", + return_value=workflow.CommandResult(1, "", "missing command"), + ): + with self.assertRaisesRegex(RuntimeError, "package get-work-item"): workflow.ensure_azsdk_work_item_commands_available() - def test_update_package_pending_reviews_is_idempotent(self): - with patch.object(workflow, "update_package_pending_api_reviews") as update_mock: - workflow.update_package_pending_reviews( - workflow.PackageWorkItem( - id=54321, - pending_api_review_urls=[ - "https://github.com/Azure/azure-sdk-for-python/pull/10" - ], - ), - "https://github.com/Azure/azure-sdk-for-python/pull/10", - ) - - update_mock.assert_not_called() - - def test_update_package_pending_reviews_appends_new_url(self): - with patch.object(workflow, "update_package_pending_api_reviews") as update_mock: - workflow.update_package_pending_reviews( - workflow.PackageWorkItem( - id=54321, - pending_api_review_urls=[ - "https://github.com/Azure/azure-sdk-for-python/pull/10" - ], - ), - "https://github.com/Azure/azure-sdk-for-python/pull/11", - ) - - update_mock.assert_called_once_with( - 54321, - "https://github.com/Azure/azure-sdk-for-python/pull/10\nhttps://github.com/Azure/azure-sdk-for-python/pull/11", - ) - if __name__ == "__main__": unittest.main()