Skip to content

Commit 9ac549b

Browse files
AkhilaIllaakhilailla
AkhilaIlla
and
akhilailla
authored
Fix Incorrectly removing label ARMSignedOff for trivial PRs (#40172)
* Fix Incorrectly removing label ARMSignedOff for trivial PRs * fix prettier issue --------- Co-authored-by: akhilailla <akhilailla@microsoft.com>
1 parent 320ddd2 commit 9ac549b

2 files changed

Lines changed: 237 additions & 21 deletions

File tree

‎.github/workflows/src/arm-auto-signoff/arm-auto-signoff-status.js‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,15 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha,
105105
labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOff) ||
106106
labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP) ||
107107
labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest);
108+
109+
// Track if ARMSignedOff was auto-added (IncrementalTSP present) vs manually added
110+
const hasAutoAddedArmSignedOff =
111+
// need to consider the legacy label
112+
labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOff) ||
113+
labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP);
108114
core.info(`Labels: ${inspect(labelNames)}`);
109115
core.info(`Has auto signed-off labels: ${hasAutoSignedOffLabels}`);
116+
core.info(`Has auto-added ARMSignedOff: ${hasAutoAddedArmSignedOff}`);
110117

111118
// permissions: { actions: read }
112119
/** @type {WorkflowRun[]} */
@@ -136,12 +143,18 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha,
136143
return noneResult;
137144
}
138145

146+
// Only remove ARMSignedOff if it was auto-added (IncrementalTSP present)
147+
// Preserve manually-added ARMSignedOff labels
139148
return {
140149
...noneResult,
141150
labelActions: {
142151
...noneLabelActions,
143-
[ArmAutoSignoffLabel.ArmSignedOff]: LabelAction.Remove,
144-
[ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP]: LabelAction.Remove,
152+
[ArmAutoSignoffLabel.ArmSignedOff]: hasAutoAddedArmSignedOff
153+
? LabelAction.Remove
154+
: LabelAction.None,
155+
[ArmAutoSignoffLabel.ArmAutoSignedOffIncrementalTSP]: hasAutoAddedArmSignedOff
156+
? LabelAction.Remove
157+
: LabelAction.None,
145158
[ArmAutoSignoffLabel.ArmAutoSignedOffTrivialTest]: LabelAction.Remove,
146159
},
147160
};
@@ -221,9 +234,11 @@ export async function getLabelActionImpl({ owner, repo, issue_number, head_sha,
221234

222235
// Add ARMSignOff label only when the PR is identified as an incremental typespec
223236
// As the trivial changes sign-off is being released in test mode
237+
// Only remove ARMSignedOff if it was auto-added (IncrementalTSP present)
238+
// Preserve manually-added ARMSignedOff labels
224239
const armSignOffAction = autoIncrementalTSP
225240
? LabelAction.Add
226-
: hasAutoSignedOffLabels
241+
: hasAutoAddedArmSignedOff
227242
? LabelAction.Remove
228243
: LabelAction.None;
229244
const autoIncrementalTSPAction = autoIncrementalTSP ? LabelAction.Add : LabelAction.Remove;

‎.github/workflows/test/arm-auto-signoff/arm-auto-signoff-status.test.js‎

Lines changed: 219 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -175,24 +175,6 @@ describe("getLabelActionImpl", () => {
175175
).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123));
176176
});
177177

178-
it("removes ARMAutoSignedOff-Trivial-Test if analysis no longer trivial", async () => {
179-
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: false });
180-
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
181-
data: [{ name: managedLabels.autoSignedOffTrivialTest }],
182-
});
183-
184-
await expect(
185-
getLabelActionImpl({
186-
owner: "TestOwner",
187-
repo: "TestRepo",
188-
issue_number: 123,
189-
head_sha: "abc123",
190-
github: github,
191-
core: core,
192-
}),
193-
).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123));
194-
});
195-
196178
it("removes label if analysis artifacts missing (fail closed)", async () => {
197179
const github = createMockGithub({ incrementalTypeSpec: true });
198180
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
@@ -560,4 +542,223 @@ describe("getLabelActionImpl", () => {
560542
}),
561543
).resolves.toEqual(createRemoveManagedLabelsResult("abc123", 123));
562544
});
545+
546+
it("preserves existing ARMSignedOff and adds trivial test label when PR qualifies by trivial changes only", async () => {
547+
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: true });
548+
549+
// PR already has ARMSignedOff (manually added) and ARMReview, but no auto-signoff labels
550+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
551+
data: [{ name: "ARMSignedOff" }, { name: "ARMReview" }],
552+
});
553+
554+
// All required checks pass
555+
github.rest.repos.listCommitStatusesForRef.mockResolvedValue({
556+
data: [
557+
{
558+
context: "Swagger LintDiff",
559+
state: CommitStatusState.SUCCESS,
560+
},
561+
{
562+
context: "Swagger Avocado",
563+
state: CommitStatusState.SUCCESS,
564+
},
565+
],
566+
});
567+
568+
const result = await getLabelActionImpl({
569+
owner: "TestOwner",
570+
repo: "TestRepo",
571+
issue_number: 123,
572+
head_sha: "abc123",
573+
github: github,
574+
core: core,
575+
});
576+
577+
// ARMSignedOff should remain unchanged (LabelAction.None) since it already exists
578+
// and is not managed by auto-signoff when hasAutoSignedOffLabels is false
579+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.None);
580+
// Trivial test label should be added
581+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Add);
582+
// Incremental TSP label should be removed (not applicable)
583+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.Remove);
584+
});
585+
586+
it("preserves manual ARMSignedOff when trivial test label present and no longer qualifies", async () => {
587+
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: false });
588+
589+
// PR has manually added ARMSignedOff + trivial test label (from previous run)
590+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
591+
data: [
592+
{ name: "ARMSignedOff" },
593+
{ name: managedLabels.autoSignedOffTrivialTest },
594+
{ name: "ARMReview" },
595+
],
596+
});
597+
598+
const result = await getLabelActionImpl({
599+
owner: "TestOwner",
600+
repo: "TestRepo",
601+
issue_number: 123,
602+
head_sha: "abc123",
603+
github: github,
604+
core: core,
605+
});
606+
607+
// ARMSignedOff was manually added (no IncrementalTSP), so preserve it
608+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.None);
609+
// Trivial test label should be removed (no longer qualifies)
610+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Remove);
611+
// Incremental TSP label was never present, so no-op
612+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.None);
613+
});
614+
615+
it("removes all auto-signoff labels when IncrementalTSP was present and no longer qualifies", async () => {
616+
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: false });
617+
618+
// PR had auto-added ARMSignedOff via IncrementalTSP
619+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
620+
data: [
621+
{ name: "ARMSignedOff" },
622+
{ name: managedLabels.autoSignedOffIncrementalTsp },
623+
{ name: "ARMReview" },
624+
],
625+
});
626+
627+
const result = await getLabelActionImpl({
628+
owner: "TestOwner",
629+
repo: "TestRepo",
630+
issue_number: 123,
631+
head_sha: "abc123",
632+
github: github,
633+
core: core,
634+
});
635+
636+
// ARMSignedOff was auto-added (IncrementalTSP present), so remove it
637+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.Remove);
638+
// Both auto-signoff labels should be removed
639+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Remove);
640+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.Remove);
641+
});
642+
643+
it("transitions from trivial to incremental typespec correctly", async () => {
644+
const github = createMockGithub({ incrementalTypeSpec: true, isTrivial: false });
645+
646+
// PR previously had trivial test label, now qualifies via incremental typespec
647+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
648+
data: [{ name: managedLabels.autoSignedOffTrivialTest }, { name: "ARMReview" }],
649+
});
650+
651+
github.rest.repos.listCommitStatusesForRef.mockResolvedValue({
652+
data: [
653+
{ context: "Swagger LintDiff", state: CommitStatusState.SUCCESS },
654+
{ context: "Swagger Avocado", state: CommitStatusState.SUCCESS },
655+
],
656+
});
657+
658+
const result = await getLabelActionImpl({
659+
owner: "TestOwner",
660+
repo: "TestRepo",
661+
issue_number: 123,
662+
head_sha: "abc123",
663+
github: github,
664+
core: core,
665+
});
666+
667+
// Now qualifies via incremental typespec, so add ARMSignedOff
668+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.Add);
669+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.Add);
670+
// Trivial test label should be removed (not applicable)
671+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Remove);
672+
});
673+
674+
it("transitions from incremental typespec to trivial correctly", async () => {
675+
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: true });
676+
677+
// PR previously had incremental typespec labels, now qualifies only via trivial
678+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
679+
data: [
680+
{ name: "ARMSignedOff" },
681+
{ name: managedLabels.autoSignedOffIncrementalTsp },
682+
{ name: "ARMReview" },
683+
],
684+
});
685+
686+
github.rest.repos.listCommitStatusesForRef.mockResolvedValue({
687+
data: [
688+
{ context: "Swagger LintDiff", state: CommitStatusState.SUCCESS },
689+
{ context: "Swagger Avocado", state: CommitStatusState.SUCCESS },
690+
],
691+
});
692+
693+
const result = await getLabelActionImpl({
694+
owner: "TestOwner",
695+
repo: "TestRepo",
696+
issue_number: 123,
697+
head_sha: "abc123",
698+
github: github,
699+
core: core,
700+
});
701+
702+
// IncrementalTSP was present, so ARMSignedOff should be removed (it was auto-added)
703+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.Remove);
704+
// Incremental TSP label should be removed
705+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.Remove);
706+
// Trivial test label should be added
707+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Add);
708+
});
709+
710+
it("adds both labels when PR qualifies for both incremental typespec and trivial", async () => {
711+
const github = createMockGithub({ incrementalTypeSpec: true, isTrivial: true });
712+
713+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
714+
data: [{ name: "ARMReview" }],
715+
});
716+
717+
github.rest.repos.listCommitStatusesForRef.mockResolvedValue({
718+
data: [
719+
{ context: "Swagger LintDiff", state: CommitStatusState.SUCCESS },
720+
{ context: "Swagger Avocado", state: CommitStatusState.SUCCESS },
721+
],
722+
});
723+
724+
const result = await getLabelActionImpl({
725+
owner: "TestOwner",
726+
repo: "TestRepo",
727+
issue_number: 123,
728+
head_sha: "abc123",
729+
github: github,
730+
core: core,
731+
});
732+
733+
// Incremental typespec takes precedence for ARMSignedOff
734+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.Add);
735+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.Add);
736+
// Trivial test label should also be added
737+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Add);
738+
});
739+
740+
it("removes only trivial test label when only TrivialTest present and no longer qualifies", async () => {
741+
const github = createMockGithub({ incrementalTypeSpec: false, isTrivial: false });
742+
743+
// PR only has trivial test label (no ARMSignedOff, no IncrementalTSP)
744+
github.rest.issues.listLabelsOnIssue.mockResolvedValue({
745+
data: [{ name: managedLabels.autoSignedOffTrivialTest }, { name: "ARMReview" }],
746+
});
747+
748+
const result = await getLabelActionImpl({
749+
owner: "TestOwner",
750+
repo: "TestRepo",
751+
issue_number: 123,
752+
head_sha: "abc123",
753+
github: github,
754+
core: core,
755+
});
756+
757+
// No IncrementalTSP, so ARMSignedOff action is None
758+
expect(result.labelActions[managedLabels.armSignedOff]).toBe(LabelAction.None);
759+
// Trivial test label should be removed
760+
expect(result.labelActions[managedLabels.autoSignedOffTrivialTest]).toBe(LabelAction.Remove);
761+
// Incremental TSP was never present, so no-op
762+
expect(result.labelActions[managedLabels.autoSignedOffIncrementalTsp]).toBe(LabelAction.None);
763+
});
563764
});

0 commit comments

Comments
 (0)
⚡