Ensure blocked based on versioning review is honored (#36630)

scbedd · web-flow · commit 6cbb9eb68257 · 2025-08-12T17:29:39.000-07:00
* and add tests for the same
diff --git a/.github/workflows/src/summarize-checks/labelling.js b/.github/workflows/src/summarize-checks/labelling.js
@@ -642,13 +642,15 @@ async function processARMReviewWorkflowLabels(
 
   const armSignedOffLabel = new Label("ARMSignedOff", labelContext.present);
 
+  const blockedOnVersioningPolicy = getBlockedOnVersioningPolicy(labelContext);
+
   const blockedOnRpaas = getBlockedOnRpaas(
     ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent,
     rpaasExceptionLabelShouldBePresent,
     ciRpaasRPNotInPrivateRepoLabelShouldBePresent,
   );
 
-  const blocked = blockedOnRpaas;
+  const blocked = blockedOnRpaas || blockedOnVersioningPolicy;
 
   // If given PR is in scope of ARM review and it is blocked for any reason,
   // the "NotReadyForARMReview" label should be present, to the exclusion
@@ -710,6 +712,23 @@ async function processARMReviewWorkflowLabels(
   return;
 }
 
+/**
+ * @param {LabelContext} labelContext
+ * @returns {boolean}
+ */
+function getBlockedOnVersioningPolicy(labelContext) {
+  const pendingVersioningReview =
+    labelContext.present.has("VersioningReviewRequired") &&
+    !anyApprovalLabelPresent("SameVersion", [...labelContext.present]);
+
+  const pendingBreakingChangeReview =
+    labelContext.present.has("BreakingChangeReviewRequired") &&
+    !anyApprovalLabelPresent("CrossVersion", [...labelContext.present]);
+
+  const blockedOnVersioningPolicy = pendingVersioningReview || pendingBreakingChangeReview;
+  return blockedOnVersioningPolicy;
+}
+
 /**
  * @param {boolean} ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent
  * @param {boolean} rpaasExceptionLabelShouldBePresent
diff --git a/.github/workflows/src/summarize-checks/summarize-checks.js b/.github/workflows/src/summarize-checks/summarize-checks.js
@@ -295,8 +295,6 @@ export default async function summarizeChecks({ github, context, core }) {
     return;
   }
 
-  // TODO: This is triggered by pull_request_target AND workflow_run.  If workflow_run, targetBranch will be undefined.
-  //       Is this OK? If not, we should be able to get the base ref by calling a GH API to fetch the PR metadata.
   const targetBranch = context.payload.pull_request?.base?.ref;
   core.info(`PR target branch: ${targetBranch}`);
 
diff --git a/.github/workflows/test/summarize-checks/labelling.test.js b/.github/workflows/test/summarize-checks/labelling.test.js
@@ -157,6 +157,67 @@ describe("update labels", () => {
         targetBranch: "main",
       },
     },
+    {
+      description:
+        "Shouldn't remove NotReadyForARMReview label when not ready for ARM review due to being blocked on versioning or breaking change review.",
+      existingLabels: [
+        "ARMReview",
+        "NotReadyForARMReview",
+        "PipelineBotTrigger",
+        "PublishToCustomers",
+        "resource-manager",
+        "RPaaS",
+        "TypeSpec",
+        "VersioningReviewRequired",
+      ],
+      expectedLabelsToAdd: [],
+      expectedLabelsToRemove: [],
+      impactAssessment: {
+        suppressionReviewRequired: false,
+        rpaasChange: true,
+        newRP: false,
+        rpaasRPMissing: false,
+        rpaasRpNotInPrivateRepo: false,
+        resourceManagerRequired: true,
+        dataPlaneRequired: false,
+        rpaasExceptionRequired: false,
+        typeSpecChanged: true,
+        isNewApiVersion: false,
+        isDraft: false,
+        targetBranch: "RPSaaSMaster",
+      },
+    },
+    {
+      description:
+        "Should remove NotReadyForARMReview and add WaitForARMFeedback when VersioningReviewRequired is approved.",
+      existingLabels: [
+        "ARMReview",
+        "NotReadyForARMReview",
+        "PipelineBotTrigger",
+        "PublishToCustomers",
+        "resource-manager",
+        "RPaaS",
+        "TypeSpec",
+        "VersioningReviewRequired",
+        "Versioning-Approved-Benign",
+      ],
+      expectedLabelsToAdd: ["WaitForARMFeedback"],
+      expectedLabelsToRemove: ["NotReadyForARMReview"],
+      impactAssessment: {
+        suppressionReviewRequired: false,
+        rpaasChange: true,
+        newRP: false,
+        rpaasRPMissing: false,
+        rpaasRpNotInPrivateRepo: false,
+        resourceManagerRequired: true,
+        dataPlaneRequired: false,
+        rpaasExceptionRequired: false,
+        typeSpecChanged: true,
+        isNewApiVersion: false,
+        isDraft: false,
+        targetBranch: "RPSaaSMaster",
+      },
+    },
   ];
   it.each(testCases)(
     "$description",
diff --git a/.github/workflows/test/summarize-checks/summarize-checks.test.js b/.github/workflows/test/summarize-checks/summarize-checks.test.js
@@ -763,6 +763,140 @@ describe("Summarize Checks Unit Tests", () => {
       expect(output).toEqual(expectedOutput);
     });
 
+    it("Should generate error summary for a PR scenario where NotReadyForARMReview is present", async () => {
+      const repo = "azure-rest-api-specs-pr";
+      const targetBranch = "RPSaaSMaster";
+      const labelNames = [
+        "ARMReview",
+        "NotReadyForARMReview",
+        "PipelineBotTrigger",
+        "PublishToCustomers",
+        "resource-manager",
+        "RPaaS",
+        "TypeSpec",
+        "VersioningReviewRequired",
+      ];
+      const fyiCheckRuns = [];
+      const expectedComment =
+        `<h2>Next Steps to Merge</h2>Next steps that must be taken to merge this PR: <br/><ul><li>❌ ` +
+        `This PR is in purview of the ARM review (label: <code>ARMReview</code>). This PR must get <code>ARMSignedOff</code> label ` +
+        `from an ARM reviewer.<br/>This PR is not ready for ARM review (label: <code>NotReadyForARMReview</code>). This PR will not ` +
+        `be reviewed by ARM until relevant problems are fixed. Consult the rest of this <code>Next Steps to Merge</code> comment ` +
+        `for details.<br/>Once the blocking problems are addressed, add to the PR a comment with contents <code>/azp run</code>. ` +
+        `Automation will re-evaluate this PR and if everything looks good, it will add <code>WaitForARMFeedback</code> label which ` +
+        `will put this PR on the ARM review queue.<br/>For details of the ARM review, see <a href="https://aka.ms/azsdk/pr-arm-review">` +
+        `aka.ms/azsdk/pr-arm-review</a><br/></li><li>❌ This PR is <code>NotReadyForARMReview</code> because it has the <code>VersioningReviewRequired</code> ` +
+        `label.<br/></li><li>❌ This PR has at least one change violating Azure versioning policy ` +
+        `(label: <code>VersioningReviewRequired</code>).<br/>To unblock this PR, either a) introduce a new API version with these ` +
+        `changes instead of modifying an existing API version, or b) follow the process at <a href="https://aka.ms/brch">aka.ms/brch</a>.</li>` +
+        `<li>❌ The required check named <code>Swagger BreakingChange</code> has failed. To unblock this PR, follow the process at <a href="https://aka.ms/brch">` +
+        `aka.ms/brch</a>.</li></ul><br /><br />Comment generated by <a href="http://github.com/a/fake/workflowrun/url">summarize-checks</a> workflow run.`;
+
+      const expectedOutput = [
+        expectedComment,
+        {
+          name: "[TEST-IGNORE] Automated merging requirements met",
+          result: "FAILURE",
+          summary:
+            "❌ This PR cannot be merged because some requirements are not met. See the details.",
+        },
+      ];
+
+      const requiredCheckRuns = [
+        {
+          name: "license/cla",
+          status: "QUEUED",
+          conclusion: null,
+          checkInfo: getCheckInfo("license/cla"),
+        },
+        {
+          name: "Swagger PrettierCheck",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Swagger PrettierCheck"),
+        },
+        {
+          name: "TypeSpec Validation",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("TypeSpec Validation"),
+        },
+        {
+          name: "Swagger SemanticValidation",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Swagger SemanticValidation"),
+        },
+        {
+          name: "TypeSpec Requirement",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("TypeSpec Requirement"),
+        },
+        {
+          name: "Protected Files",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Protected Files"),
+        },
+        {
+          name: "SpellCheck",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("SpellCheck"),
+        },
+        {
+          name: "Swagger ModelValidation",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Swagger ModelValidation"),
+        },
+        {
+          name: "SDK Validation Status",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("SDK Validation Status"),
+        },
+        {
+          name: "Breaking Change(Cross-Version)",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Breaking Change(Cross-Version)"),
+        },
+        {
+          name: "Swagger BreakingChange",
+          status: "COMPLETED",
+          conclusion: "FAILURE",
+          checkInfo: getCheckInfo("Swagger BreakingChange"),
+        },
+        {
+          name: "Swagger Avocado",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Swagger Avocado"),
+        },
+        {
+          name: "Swagger LintDiff",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("Swagger LintDiff"),
+        },
+      ];
+
+      const output = await createNextStepsComment(
+        mockCore,
+        repo,
+        labelNames,
+        targetBranch,
+        requiredCheckRuns,
+        fyiCheckRuns,
+        true, // assessmentCompleted
+        WORKFLOW_URL,
+      );
+
+      expect(output).toEqual(expectedOutput);
+    });
+
     it("Should generate error summary for a PR scenario with labeling issues", async () => {
       const repo = "azure-rest-api-specs";
       const targetBranch = "main";

Original file line number	Diff line number	Diff line change
`@@ -295,8 +295,6 @@ export default async function summarizeChecks({ github, context, core }) {`
`295`	`295`	`return;`
`296`	`296`	`}`
`297`	`297`
`298`		`- // TODO: This is triggered by pull_request_target AND workflow_run. If workflow_run, targetBranch will be undefined.`
`299`		`- // Is this OK? If not, we should be able to get the base ref by calling a GH API to fetch the PR metadata.`
`300`	`298`	`const targetBranch = context.payload.pull_request?.base?.ref;`
`301`	`299`	core.info(`PR target branch: ${targetBranch}`);
`302`	`300`