Don't remove RPaaSException from summarize-checks

.github/workflows/src/summarize-checks/labelling.js

@@ -1,3 +1,3 @@
 /*
     This file covers two areas of enforcement:
     1. It calculates what set of label rules has been violated by the current PR, for the purposes of updating next steps to merge.
@@ -51,7 +51,6 @@
  * @property {boolean} dataPlaneRequired - Whether a data plane review is required.
  * @property {boolean} suppressionReviewRequired - Whether a suppression review is required.
  * @property {boolean} isNewApiVersion - Whether this PR introduces a new API version.
- * @property {boolean} rpaasExceptionRequired - Whether an RPaaS exception is required.
  * @property {boolean} rpaasRpNotInPrivateRepo - Whether the RPaaS RP is not present in the private repo.
  * @property {boolean} rpaasChange - Whether this PR includes RPaaS changes.
  * @property {boolean} newRP - Whether this PR introduces a new resource provider.
@@ -505,8 +504,7 @@
   );
   ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = impactAssessment.rpaasRPMissing || false;
 
-  const rpaasExceptionLabel = new Label("RPaaSException", labelContext.present);
-  rpaasExceptionLabel.shouldBePresent = impactAssessment.rpaasExceptionRequired || false;
+  const rpaasExceptionLabelPresent = "RPaaSException" in labelContext.present;
 
   const ciRpaasRPNotInPrivateRepoLabel = new Label(
     "CI-RpaaSRPNotInPrivateRepo",
@@ -541,7 +539,7 @@
       labelContext,
       armReviewLabel.shouldBePresent,
       impactAssessment.rpaasRPMissing,
-      impactAssessment.rpaasExceptionRequired,
+      rpaasExceptionLabelPresent,
       impactAssessment.rpaasRpNotInPrivateRepo,
     );
   }
@@ -555,7 +553,6 @@
   rpassReviewRequiredLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
   newRPNamespaceLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
   ciNewRPNamespaceWithoutRpaaSLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
-  rpaasExceptionLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
   ciRpaasRPNotInPrivateRepoLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
 
   // this is the only labelling that was part of original pipelinebot logic, it handles the rotation of

.github/workflows/test/summarize-checks/labelling.test.js

@@ -1,3 +1,3 @@
 import { describe, expect, it } from "vitest";
 import { processArmReviewLabels } from "../../src/summarize-checks/labelling.js";
 import { updateLabels } from "../../src/summarize-checks/summarize-checks.js";
@@ -17,7 +17,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,
@@ -37,7 +36,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: false,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,
@@ -57,7 +55,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,
@@ -77,7 +74,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -103,7 +99,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -123,7 +118,25 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
+        typeSpecChanged: true,
+        isNewApiVersion: true,
+        isDraft: false,
+        targetBranch: "a-test-branch",
+      },
+    },
+    {
+      description: "Shouldn't remove RPaasException label accidentally",
+      existingLabels: ["other-label", "CI-NewRPNamespaceWithoutRPaaS", "RPaaSException"],
+      expectedLabelsToAdd: ["TypeSpec", "resource-manager"],
+      expectedLabelsToRemove: [],
+      impactAssessment: {
+        suppressionReviewRequired: false,
+        rpaasChange: false,
+        newRP: false,
+        rpaasRPMissing: true,
+        rpaasRpNotInPrivateRepo: false,
+        resourceManagerRequired: true,
+        dataPlaneRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -150,7 +163,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -180,7 +192,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,
@@ -211,7 +222,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,

.github/workflows/test/summarize-checks/summarize-checks.test.js

@@ -1,3 +1,3 @@
 import { Octokit } from "@octokit/rest";
 import { strToU8, zipSync } from "fflate";
 import { describe, expect, it } from "vitest";
@@ -433,6 +433,41 @@
       expect(output).toEqual(expectedOutput);
     });
 
+    it("should generate error summary when CI-NewRPNamespaceWithoutRPaaS is present, and RPaaSException is as well.", async () => {
+      const repo = "azure-rest-api-specs";
+      const targetBranch = "main";
+      const labelNames = ["CI-NewRPNamespaceWithoutRPaaS", "RPaaSException"];
+      const fyiCheckRuns = [];
+      const expectedCommentBody = `<h2>Next Steps to Merge</h2>✅ All automated merging requirements have been met! To get your PR merged, see <a href="https://aka.ms/azsdk/specreview/merge">aka.ms/azsdk/specreview/merge</a>.<br /><br />Comment generated by <a href="undefined">summarize-checks</a> workflow run.`;
+      const expectedCheckOutput = {
+        name: "Automated merging requirements met",
+        result: "SUCCESS",
+        summary: `✅ All automated merging requirements have been met.<br/>To merge this PR, refer to <a href="https://aka.ms/azsdk/specreview/merge">aka.ms/azsdk/specreview/merge</a>.<br/>For help, consult comments on this PR and see [aka.ms/azsdk/pr-getting-help](https://aka.ms/azsdk/pr-getting-help).`,
+      };
+
+      const requiredCheckRuns = [
+        {
+          name: "SpellCheck",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("SpellCheck"),
+        },
+      ];
+
+      const [commentBody, automatedCheckOutput] = await createNextStepsComment(
+        mockCore,
+        repo,
+        labelNames,
+        targetBranch,
+        requiredCheckRuns,
+        fyiCheckRuns,
+        true, // assessmentCompleted
+      );
+
+      expect(commentBody).toEqual(expectedCommentBody);
+      expect(automatedCheckOutput).toEqual(expectedCheckOutput);
+    });
+
     it("should generate pending summary when checks are partially in progress", async () => {
       const repo = "azure-rest-api-specs";
       const targetBranch = "main";
@@ -1116,6 +1151,42 @@
       expect(automatedCheckOutput).toEqual(expectedCheckOutput);
     });
 
+    it("should generate error summary when CI-NewRPNamespaceWithoutRPaaS is present, but RPaaSException is not.", async () => {
+      const repo = "azure-rest-api-specs";
+      const targetBranch = "main";
+      const labelNames = ["CI-NewRPNamespaceWithoutRPaaS"];
+      const fyiCheckRuns = [];
+      const expectedCommentBody = `<h2>Next Steps to Merge</h2>Next steps that must be taken to merge this PR: <br/><ul><li>❌ This PR has <code>CI-NewRPNamespaceWithoutRPaaS</code> label. This means it is introducing a new RP (Resource Provider) namespace that has not been onboarded with <a href="https://armwiki.azurewebsites.net/rpaas/overview.html">RPaaS</a>. Merging this PR to the <code>main</code> branch is blocked as RPaaS is required for new RPs.<br/>To resolve, <a href="https://armwiki.azurewebsites.net/rpaas/gettingstarted.html">use RPaaS to onboard the new RP</a>. To apply for exception, see <a href="https://aka.ms/RPaaSException">aka.ms/RPaaSException</a>.</li></ul><br /><br />Comment generated by <a href="undefined">summarize-checks</a> workflow run.`;
+      const expectedCheckOutput = {
+        name: "Automated merging requirements met",
+        result: "FAILURE",
+        summary:
+          "❌ This PR cannot be merged because some requirements are not met. See the details.",
+      };
+
+      const requiredCheckRuns = [
+        {
+          name: "SpellCheck",
+          status: "COMPLETED",
+          conclusion: "SUCCESS",
+          checkInfo: getCheckInfo("SpellCheck"),
+        },
+      ];
+
+      const [commentBody, automatedCheckOutput] = await createNextStepsComment(
+        mockCore,
+        repo,
+        labelNames,
+        targetBranch,
+        requiredCheckRuns,
+        fyiCheckRuns,
+        true, // assessmentCompleted
+      );
+
+      expect(commentBody).toEqual(expectedCommentBody);
+      expect(automatedCheckOutput).toEqual(expectedCheckOutput);
+    });
+
     it("should generate error summary when checks are in error state", async () => {
       const repo = "azure-rest-api-specs";
       const targetBranch = "main";
@@ -1165,7 +1236,6 @@
         dataPlaneRequired: true,
         suppressionReviewRequired: false,
         isNewApiVersion: true,
-        rpaasExceptionRequired: false,
         rpaasRpNotInPrivateRepo: true,
         rpaasChange: false,
         newRP: true,

eng/tools/summarize-impact/src/ImpactAssessment.ts

@@ -1,9 +1,8 @@
 export type ImpactAssessment = {
   resourceManagerRequired: boolean;
   dataPlaneRequired: boolean;
   suppressionReviewRequired: boolean;
   isNewApiVersion: boolean;
-  rpaasExceptionRequired: boolean;
   rpaasRpNotInPrivateRepo: boolean;
   rpaasChange: boolean;
   newRP: boolean;

eng/tools/summarize-impact/src/impact.ts

@@ -1,3 +1,3 @@
 #!/usr/bin/env node
 
 import { existsSync, readFileSync } from "fs";
@@ -120,7 +120,7 @@
   // doesn't necessarily need to be in the PR context.
   // Uses the previous outputs that DID need to be a PR context, but otherwise only examines targetBranch and those
   // output labels.
-  const { ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent, rpaasExceptionLabelShouldBePresent } =
+  const ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent =
     await processNewRpNamespaceWithoutRpaasLabel(
       context,
       labelContext,
@@ -149,7 +149,6 @@
     rpaasRpNotInPrivateRepo: ciRpaasRPNotInPrivateRepoLabelShouldBePresent,
     resourceManagerRequired: resourceManagerLabelShouldBePresent,
     dataPlaneRequired: dataPlaneShouldBePresent,
-    rpaasExceptionRequired: rpaasExceptionLabelShouldBePresent,
     typeSpecChanged: typeSpecLabelShouldBePresent,
     isNewApiVersion: newApiVersion,
     isDraft: context.isDraft,
@@ -626,10 +625,7 @@
   resourceManagerLabelShouldBePresent: boolean,
   newRPNamespaceLabelShouldBePresent: boolean,
   rpaasLabelShouldBePresent: boolean,
-): Promise<{
-  ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent: boolean;
-  rpaasExceptionLabelShouldBePresent: boolean;
-}> {
+): Promise<boolean> {
   console.log("ENTER definition processNewRpNamespaceWithoutRpaasLabel");
   const ciNewRPNamespaceWithoutRpaaSLabel = new Label(
     "CI-NewRPNamespaceWithoutRPaaS",
@@ -638,8 +634,6 @@
   // By default this label should not be present. We may determine later in this function that it should be present after all.
   ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = false;
 
-  const rpaasExceptionLabel = new Label("RPaaSException", labelContext.present);
-
   let skip = false;
 
   if (!resourceManagerLabelShouldBePresent) {
@@ -664,18 +658,10 @@
     ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = true;
   }
 
-  rpaasExceptionLabel.shouldBePresent =
-    rpaasExceptionLabel.present && ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent;
-
   ciNewRPNamespaceWithoutRpaaSLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
-  rpaasExceptionLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
   console.log("RETURN definition processNewRpNamespaceWithoutRpaasLabel");
 
-  return {
-    ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent:
-      ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent,
-    rpaasExceptionLabelShouldBePresent: rpaasExceptionLabel.shouldBePresent as boolean,
-  };
+  return ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent;
 }
 
 export const getRPaaSFolderList = async (

eng/tools/summarize-impact/test/impact.test.ts

@@ -1,3 +1,3 @@
 import fs from "fs";
 import path from "path";
 import { afterEach, beforeEach, describe, expect, it } from "vitest"; //vi
@@ -47,7 +47,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: false,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: false,
         isNewApiVersion: false,
         isDraft: false,
@@ -71,7 +70,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: false,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: false,
         isDraft: false,
@@ -95,7 +93,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: false,
         isNewApiVersion: false,
         isDraft: false,
@@ -121,7 +118,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: false,
         dataPlaneRequired: true,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -148,7 +144,6 @@
         rpaasRpNotInPrivateRepo: true,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,
@@ -175,7 +170,6 @@
         rpaasRpNotInPrivateRepo: false,
         resourceManagerRequired: true,
         dataPlaneRequired: false,
-        rpaasExceptionRequired: false,
         typeSpecChanged: true,
         isNewApiVersion: true,
         isDraft: false,