Skip to content

Commit 8c87aeb

Browse files
scbeddmikeharder
scbedd
and
mikeharder
authored
Don't remove RPaaSException from summarize-checks (#37874)
* fix summarize checks to not erroneously remove the RPaasException label * remove rpassExceptionRequired from impactAssessment, as this input is no longer required to summarize-checks * additional tests exercising cases around RPaaSException label, lint and format Co-authored-by: Mike Harder <[email protected]>
1 parent 2c6dacc commit 8c87aeb

File tree

6 files changed

+95
-39
lines changed

6 files changed

+95
-39
lines changed

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ import {
5151
* @property {boolean} dataPlaneRequired - Whether a data plane review is required.
5252
* @property {boolean} suppressionReviewRequired - Whether a suppression review is required.
5353
* @property {boolean} isNewApiVersion - Whether this PR introduces a new API version.
54-
* @property {boolean} rpaasExceptionRequired - Whether an RPaaS exception is required.
5554
* @property {boolean} rpaasRpNotInPrivateRepo - Whether the RPaaS RP is not present in the private repo.
5655
* @property {boolean} rpaasChange - Whether this PR includes RPaaS changes.
5756
* @property {boolean} newRP - Whether this PR introduces a new resource provider.
@@ -505,8 +504,7 @@ export async function processImpactAssessment(labelContext, impactAssessment) {
505504
);
506505
ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = impactAssessment.rpaasRPMissing || false;
507506

508-
const rpaasExceptionLabel = new Label("RPaaSException", labelContext.present);
509-
rpaasExceptionLabel.shouldBePresent = impactAssessment.rpaasExceptionRequired || false;
507+
const rpaasExceptionLabelPresent = "RPaaSException" in labelContext.present;
510508

511509
const ciRpaasRPNotInPrivateRepoLabel = new Label(
512510
"CI-RpaaSRPNotInPrivateRepo",
@@ -541,7 +539,7 @@ export async function processImpactAssessment(labelContext, impactAssessment) {
541539
labelContext,
542540
armReviewLabel.shouldBePresent,
543541
impactAssessment.rpaasRPMissing,
544-
impactAssessment.rpaasExceptionRequired,
542+
rpaasExceptionLabelPresent,
545543
impactAssessment.rpaasRpNotInPrivateRepo,
546544
);
547545
}
@@ -555,7 +553,6 @@ export async function processImpactAssessment(labelContext, impactAssessment) {
555553
rpassReviewRequiredLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
556554
newRPNamespaceLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
557555
ciNewRPNamespaceWithoutRpaaSLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
558-
rpaasExceptionLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
559556
ciRpaasRPNotInPrivateRepoLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
560557

561558
// 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‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ describe("update labels", () => {
1717
rpaasRpNotInPrivateRepo: false,
1818
resourceManagerRequired: true,
1919
dataPlaneRequired: false,
20-
rpaasExceptionRequired: false,
2120
typeSpecChanged: true,
2221
isNewApiVersion: false,
2322
isDraft: false,
@@ -37,7 +36,6 @@ describe("update labels", () => {
3736
rpaasRpNotInPrivateRepo: false,
3837
resourceManagerRequired: false,
3938
dataPlaneRequired: false,
40-
rpaasExceptionRequired: false,
4139
typeSpecChanged: true,
4240
isNewApiVersion: false,
4341
isDraft: false,
@@ -57,7 +55,6 @@ describe("update labels", () => {
5755
rpaasRpNotInPrivateRepo: false,
5856
resourceManagerRequired: true,
5957
dataPlaneRequired: false,
60-
rpaasExceptionRequired: false,
6158
typeSpecChanged: true,
6259
isNewApiVersion: false,
6360
isDraft: false,
@@ -77,7 +74,6 @@ describe("update labels", () => {
7774
rpaasRpNotInPrivateRepo: false,
7875
resourceManagerRequired: true,
7976
dataPlaneRequired: false,
80-
rpaasExceptionRequired: false,
8177
typeSpecChanged: true,
8278
isNewApiVersion: true,
8379
isDraft: false,
@@ -103,7 +99,6 @@ describe("update labels", () => {
10399
rpaasRpNotInPrivateRepo: false,
104100
resourceManagerRequired: true,
105101
dataPlaneRequired: false,
106-
rpaasExceptionRequired: false,
107102
typeSpecChanged: true,
108103
isNewApiVersion: true,
109104
isDraft: false,
@@ -123,7 +118,25 @@ describe("update labels", () => {
123118
rpaasRpNotInPrivateRepo: false,
124119
resourceManagerRequired: true,
125120
dataPlaneRequired: false,
126-
rpaasExceptionRequired: false,
121+
typeSpecChanged: true,
122+
isNewApiVersion: true,
123+
isDraft: false,
124+
targetBranch: "a-test-branch",
125+
},
126+
},
127+
{
128+
description: "Shouldn't remove RPaasException label accidentally",
129+
existingLabels: ["other-label", "CI-NewRPNamespaceWithoutRPaaS", "RPaaSException"],
130+
expectedLabelsToAdd: ["TypeSpec", "resource-manager"],
131+
expectedLabelsToRemove: [],
132+
impactAssessment: {
133+
suppressionReviewRequired: false,
134+
rpaasChange: false,
135+
newRP: false,
136+
rpaasRPMissing: true,
137+
rpaasRpNotInPrivateRepo: false,
138+
resourceManagerRequired: true,
139+
dataPlaneRequired: false,
127140
typeSpecChanged: true,
128141
isNewApiVersion: true,
129142
isDraft: false,
@@ -150,7 +163,6 @@ describe("update labels", () => {
150163
rpaasRpNotInPrivateRepo: false,
151164
resourceManagerRequired: true,
152165
dataPlaneRequired: false,
153-
rpaasExceptionRequired: false,
154166
typeSpecChanged: true,
155167
isNewApiVersion: true,
156168
isDraft: false,
@@ -180,7 +192,6 @@ describe("update labels", () => {
180192
rpaasRpNotInPrivateRepo: false,
181193
resourceManagerRequired: true,
182194
dataPlaneRequired: false,
183-
rpaasExceptionRequired: false,
184195
typeSpecChanged: true,
185196
isNewApiVersion: false,
186197
isDraft: false,
@@ -211,7 +222,6 @@ describe("update labels", () => {
211222
rpaasRpNotInPrivateRepo: false,
212223
resourceManagerRequired: true,
213224
dataPlaneRequired: false,
214-
rpaasExceptionRequired: false,
215225
typeSpecChanged: true,
216226
isNewApiVersion: false,
217227
isDraft: false,

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

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,41 @@ describe("Summarize Checks Unit Tests", () => {
433433
expect(output).toEqual(expectedOutput);
434434
});
435435

436+
it("should generate success summary when CI-NewRPNamespaceWithoutRPaaS is present, and RPaaSException is as well.", async () => {
437+
const repo = "azure-rest-api-specs";
438+
const targetBranch = "main";
439+
const labelNames = ["CI-NewRPNamespaceWithoutRPaaS", "RPaaSException"];
440+
const fyiCheckRuns = [];
441+
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.`;
442+
const expectedCheckOutput = {
443+
name: "Automated merging requirements met",
444+
result: "SUCCESS",
445+
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).`,
446+
};
447+
448+
const requiredCheckRuns = [
449+
{
450+
name: "SpellCheck",
451+
status: "COMPLETED",
452+
conclusion: "SUCCESS",
453+
checkInfo: getCheckInfo("SpellCheck"),
454+
},
455+
];
456+
457+
const [commentBody, automatedCheckOutput] = await createNextStepsComment(
458+
mockCore,
459+
repo,
460+
labelNames,
461+
targetBranch,
462+
requiredCheckRuns,
463+
fyiCheckRuns,
464+
true, // assessmentCompleted
465+
);
466+
467+
expect(commentBody).toEqual(expectedCommentBody);
468+
expect(automatedCheckOutput).toEqual(expectedCheckOutput);
469+
});
470+
436471
it("should generate pending summary when checks are partially in progress", async () => {
437472
const repo = "azure-rest-api-specs";
438473
const targetBranch = "main";
@@ -1116,6 +1151,42 @@ describe("Summarize Checks Unit Tests", () => {
11161151
expect(automatedCheckOutput).toEqual(expectedCheckOutput);
11171152
});
11181153

1154+
it("should generate error summary when CI-NewRPNamespaceWithoutRPaaS is present, but RPaaSException is not.", async () => {
1155+
const repo = "azure-rest-api-specs";
1156+
const targetBranch = "main";
1157+
const labelNames = ["CI-NewRPNamespaceWithoutRPaaS"];
1158+
const fyiCheckRuns = [];
1159+
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.`;
1160+
const expectedCheckOutput = {
1161+
name: "Automated merging requirements met",
1162+
result: "FAILURE",
1163+
summary:
1164+
"❌ This PR cannot be merged because some requirements are not met. See the details.",
1165+
};
1166+
1167+
const requiredCheckRuns = [
1168+
{
1169+
name: "SpellCheck",
1170+
status: "COMPLETED",
1171+
conclusion: "SUCCESS",
1172+
checkInfo: getCheckInfo("SpellCheck"),
1173+
},
1174+
];
1175+
1176+
const [commentBody, automatedCheckOutput] = await createNextStepsComment(
1177+
mockCore,
1178+
repo,
1179+
labelNames,
1180+
targetBranch,
1181+
requiredCheckRuns,
1182+
fyiCheckRuns,
1183+
true, // assessmentCompleted
1184+
);
1185+
1186+
expect(commentBody).toEqual(expectedCommentBody);
1187+
expect(automatedCheckOutput).toEqual(expectedCheckOutput);
1188+
});
1189+
11191190
it("should generate error summary when checks are in error state", async () => {
11201191
const repo = "azure-rest-api-specs";
11211192
const targetBranch = "main";
@@ -1165,7 +1236,6 @@ describe("Summarize Checks Unit Tests", () => {
11651236
dataPlaneRequired: true,
11661237
suppressionReviewRequired: false,
11671238
isNewApiVersion: true,
1168-
rpaasExceptionRequired: false,
11691239
rpaasRpNotInPrivateRepo: true,
11701240
rpaasChange: false,
11711241
newRP: true,

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ export type ImpactAssessment = {
33
dataPlaneRequired: boolean;
44
suppressionReviewRequired: boolean;
55
isNewApiVersion: boolean;
6-
rpaasExceptionRequired: boolean;
76
rpaasRpNotInPrivateRepo: boolean;
87
rpaasChange: boolean;
98
newRP: boolean;

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

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ export async function evaluateImpact(
120120
// doesn't necessarily need to be in the PR context.
121121
// Uses the previous outputs that DID need to be a PR context, but otherwise only examines targetBranch and those
122122
// output labels.
123-
const { ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent, rpaasExceptionLabelShouldBePresent } =
123+
const ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent =
124124
await processNewRpNamespaceWithoutRpaasLabel(
125125
context,
126126
labelContext,
@@ -149,7 +149,6 @@ export async function evaluateImpact(
149149
rpaasRpNotInPrivateRepo: ciRpaasRPNotInPrivateRepoLabelShouldBePresent,
150150
resourceManagerRequired: resourceManagerLabelShouldBePresent,
151151
dataPlaneRequired: dataPlaneShouldBePresent,
152-
rpaasExceptionRequired: rpaasExceptionLabelShouldBePresent,
153152
typeSpecChanged: typeSpecLabelShouldBePresent,
154153
isNewApiVersion: newApiVersion,
155154
isDraft: context.isDraft,
@@ -626,10 +625,7 @@ async function processNewRpNamespaceWithoutRpaasLabel(
626625
resourceManagerLabelShouldBePresent: boolean,
627626
newRPNamespaceLabelShouldBePresent: boolean,
628627
rpaasLabelShouldBePresent: boolean,
629-
): Promise<{
630-
ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent: boolean;
631-
rpaasExceptionLabelShouldBePresent: boolean;
632-
}> {
628+
): Promise<boolean> {
633629
console.log("ENTER definition processNewRpNamespaceWithoutRpaasLabel");
634630
const ciNewRPNamespaceWithoutRpaaSLabel = new Label(
635631
"CI-NewRPNamespaceWithoutRPaaS",
@@ -638,8 +634,6 @@ async function processNewRpNamespaceWithoutRpaasLabel(
638634
// By default this label should not be present. We may determine later in this function that it should be present after all.
639635
ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = false;
640636

641-
const rpaasExceptionLabel = new Label("RPaaSException", labelContext.present);
642-
643637
let skip = false;
644638

645639
if (!resourceManagerLabelShouldBePresent) {
@@ -664,18 +658,10 @@ async function processNewRpNamespaceWithoutRpaasLabel(
664658
ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent = true;
665659
}
666660

667-
rpaasExceptionLabel.shouldBePresent =
668-
rpaasExceptionLabel.present && ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent;
669-
670661
ciNewRPNamespaceWithoutRpaaSLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
671-
rpaasExceptionLabel.applyStateChange(labelContext.toAdd, labelContext.toRemove);
672662
console.log("RETURN definition processNewRpNamespaceWithoutRpaasLabel");
673663

674-
return {
675-
ciNewRPNamespaceWithoutRpaaSLabelShouldBePresent:
676-
ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent,
677-
rpaasExceptionLabelShouldBePresent: rpaasExceptionLabel.shouldBePresent as boolean,
678-
};
664+
return ciNewRPNamespaceWithoutRpaaSLabel.shouldBePresent;
679665
}
680666

681667
export const getRPaaSFolderList = async (

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ describe("Impact Detection - default fixture", () => {
4747
rpaasRpNotInPrivateRepo: false,
4848
resourceManagerRequired: false,
4949
dataPlaneRequired: false,
50-
rpaasExceptionRequired: false,
5150
typeSpecChanged: false,
5251
isNewApiVersion: false,
5352
isDraft: false,
@@ -71,7 +70,6 @@ describe("Impact Detection - default fixture", () => {
7170
rpaasRpNotInPrivateRepo: false,
7271
resourceManagerRequired: false,
7372
dataPlaneRequired: false,
74-
rpaasExceptionRequired: false,
7573
typeSpecChanged: true,
7674
isNewApiVersion: false,
7775
isDraft: false,
@@ -95,7 +93,6 @@ describe("Impact Detection - default fixture", () => {
9593
rpaasRpNotInPrivateRepo: false,
9694
resourceManagerRequired: true,
9795
dataPlaneRequired: false,
98-
rpaasExceptionRequired: false,
9996
typeSpecChanged: false,
10097
isNewApiVersion: false,
10198
isDraft: false,
@@ -121,7 +118,6 @@ describe("Impact Detection - default fixture", () => {
121118
rpaasRpNotInPrivateRepo: false,
122119
resourceManagerRequired: false,
123120
dataPlaneRequired: true,
124-
rpaasExceptionRequired: false,
125121
typeSpecChanged: true,
126122
isNewApiVersion: true,
127123
isDraft: false,
@@ -148,7 +144,6 @@ describe("Impact Detection - default fixture", () => {
148144
rpaasRpNotInPrivateRepo: true,
149145
resourceManagerRequired: true,
150146
dataPlaneRequired: false,
151-
rpaasExceptionRequired: false,
152147
typeSpecChanged: true,
153148
isNewApiVersion: true,
154149
isDraft: false,
@@ -175,7 +170,6 @@ describe("Impact Detection - default fixture", () => {
175170
rpaasRpNotInPrivateRepo: false,
176171
resourceManagerRequired: true,
177172
dataPlaneRequired: false,
178-
rpaasExceptionRequired: false,
179173
typeSpecChanged: true,
180174
isNewApiVersion: true,
181175
isDraft: false,

0 commit comments

Comments
 (0)