Skip to content

Commit 251ac2d

Browse files
mikehardermikeharder
authored
[summarize-checks] Code cleanup and refactoring (#36367)
- Move set.intersect() to shared - Move dump-trigger-metadata to JS file - Update type comments
1 parent 24bd27a commit 251ac2d

File tree

6 files changed

+119
-49
lines changed

6 files changed

+119
-49
lines changed

‎.github/shared/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
"./path": "./src/path.js",
1515
"./readme": "./src/readme.js",
1616
"./sdk-types": "./src/sdk-types.js",
17+
"./set": "./src/set.js",
1718
"./sleep": "./src/sleep.js",
1819
"./sort": "./src/sort.js",
1920
"./spec-model-error": "./src/spec-model-error.js",

‎.github/shared/src/set.js‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// @ts-check
2+
3+
/**
4+
* @template T
5+
* @param {Set<T>} a
6+
* @param {Set<T>} b
7+
* @returns {Set<T>}
8+
*/
9+
export function intersect(a, b) {
10+
// Since set lookup is O(1), iterate over the smaller set for better perf: O(small) vs O(large)
11+
const [small, large] = a.size < b.size ? [a, b] : [b, a];
12+
return new Set([...small].filter((value) => large.has(value)));
13+
}

‎.github/shared/test/set.test.js‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// @ts-check
2+
3+
import { describe, expect, it } from "vitest";
4+
import { intersect } from "../src/set";
5+
6+
describe("set", () => {
7+
it.each([
8+
[[], [], []],
9+
[[1, 2, 3], [], []],
10+
[
11+
[1, 2, 3],
12+
[2, 3, 4],
13+
[2, 3],
14+
],
15+
[[1, 2, 3], [4, 5, 6], []],
16+
[
17+
[1, 2, 3],
18+
[1, 2, 3],
19+
[1, 2, 3],
20+
],
21+
[
22+
["a", "b", "c"],
23+
["b", "c", "d"],
24+
["b", "c"],
25+
],
26+
])(
27+
"intersect(%o, %o, %o)",
28+
async (
29+
/** @type {(string|number)[]} */ a,
30+
/** @type {(string|number)[]} */ b,
31+
/** @type {(string|number)[]} */ result,
32+
) => {
33+
const setA = new Set(a);
34+
const setB = new Set(b);
35+
const setResult = new Set(result);
36+
37+
// Check both orders, result should be same
38+
expect(intersect(setA, setB)).toEqual(setResult);
39+
expect(intersect(setB, setA)).toEqual(setResult);
40+
},
41+
);
42+
});

‎.github/workflows/src/summarize-checks/dump-trigger-metadata.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// @ts-check
2+
3+
/**
4+
* @param {import('@actions/github-script').AsyncFunctionArguments} AsyncFunctionArguments
5+
* @returns {Promise<void>}
6+
*/
7+
export default async function dumpTriggerMetadata({ context, core }) {
8+
core.info(`Event name: ${context.eventName}`);
9+
core.info(`Action: ${context.payload.action}`);
10+
11+
if (
12+
context.eventName === "workflow_run" &&
13+
context.payload.action === "completed" &&
14+
context.payload.workflow_run
15+
) {
16+
const payload = /** @type {import("@octokit/webhooks-types").WorkflowRunCompletedEvent} */ (
17+
context.payload
18+
);
19+
20+
const run = payload.workflow_run;
21+
core.info(`Triggering workflow: ${run.name}`);
22+
core.info(`Triggering workflow ID: ${run.id}`);
23+
core.info(`Triggering run ID: ${run.run_number}`);
24+
core.info(`Triggering event: ${run.event}`);
25+
core.info(`Triggering status: ${run.status}`);
26+
core.info(`Triggering conclusion: ${run.conclusion}`);
27+
core.info(`Triggering branch: ${run.head_branch}`);
28+
core.info(`Triggering SHA: ${run.head_sha}`);
29+
core.info(`Triggering actor: ${run.actor?.login || "unknown"}`);
30+
31+
// Create clickable URL to the triggering workflow run
32+
const triggeringRunUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${run.id}`;
33+
core.info(`🔗 Triggering workflow run URL: ${triggeringRunUrl}`);
34+
35+
// If there's a pull request associated, show that too
36+
if (run.pull_requests && run.pull_requests.length > 0) {
37+
run.pull_requests.forEach((pr) => {
38+
const prUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/pull/${pr.number}`;
39+
core.info(`🔗 Associated PR: ${prUrl}`);
40+
});
41+
}
42+
} else if (context.eventName === "pull_request_target" && context.payload.pull_request) {
43+
const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ (
44+
context.payload
45+
);
46+
47+
const pr = payload.pull_request;
48+
core.info(`PR number: ${pr.number}`);
49+
core.info(`PR title: ${pr.title}`);
50+
core.info(`PR state: ${pr.state}`);
51+
core.info(`PR head SHA: ${pr.head.sha}`);
52+
core.info(`PR base branch: ${pr.base.ref}`);
53+
core.info(`🔗 PR URL: ${pr.html_url}`);
54+
}
55+
}

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

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { extractInputs } from "../context.js";
2323
// import { commentOrUpdate } from "../comment.js";
2424
import { execFile } from "../../../shared/src/exec.js";
2525
import { CheckConclusion, PER_PAGE_MAX } from "../../../shared/src/github.js";
26+
import { intersect } from "../../../shared/src/set.js";
2627
import {
2728
brChRevApproval,
2829
getViolatedRequiredLabelsRules,
@@ -292,12 +293,13 @@ export default async function summarizeChecks({ github, context, core }) {
292293
return;
293294
}
294295

296+
// TODO: This is triggered by pull_request_target AND workflow_run. If workflow_run, targetBranch will be undefined.
297+
// Is this OK? If not, we should be able to get the base ref by calling a GH API to fetch the PR metadata.
295298
const targetBranch = context.payload.pull_request?.base?.ref;
296299
core.info(`PR target branch: ${targetBranch}`);
297300

298301
await summarizeChecksImpl(
299302
github,
300-
context,
301303
core,
302304
owner,
303305
repo,
@@ -332,7 +334,6 @@ export function outputRunDetails(core, requiredCheckRuns, fyiCheckRuns) {
332334

333335
/**
334336
* @param {import('@actions/github-script').AsyncFunctionArguments['github']} github
335-
* @param {import('@actions/github').context } context
336337
* @param {typeof import("@actions/core")} core
337338
* @param {string} owner
338339
* @param {string} repo
@@ -344,7 +345,6 @@ export function outputRunDetails(core, requiredCheckRuns, fyiCheckRuns) {
344345
*/
345346
export async function summarizeChecksImpl(
346347
github,
347-
context,
348348
core,
349349
owner,
350350
repo,
@@ -507,8 +507,7 @@ export async function getExistingLabels(github, owner, repo, issue_number) {
507507
issue_number: issue_number,
508508
per_page: PER_PAGE_MAX,
509509
});
510-
/** @type {string[]} */
511-
return labels.map((/** @type {{ name: string; }} */ label) => label.name);
510+
return labels.map((label) => label.name);
512511
}
513512

514513
// #endregion
@@ -518,7 +517,7 @@ export async function getExistingLabels(github, owner, repo, issue_number) {
518517
* @param {Set<string>} labelsToRemove
519518
*/
520519
function warnIfLabelSetsIntersect(labelsToAdd, labelsToRemove) {
521-
const intersection = Array.from(labelsToAdd).filter((label) => labelsToRemove.has(label));
520+
const intersection = [...intersect(labelsToAdd, labelsToRemove)];
522521
if (intersection.length > 0) {
523522
console.warn(
524523
"ASSERTION VIOLATION! The intersection of labelsToRemove and labelsToAdd is non-empty! " +
@@ -550,8 +549,6 @@ export function updateLabels(existingLabels, impactAssessment) {
550549
};
551550

552551
if (impactAssessment) {
553-
console.log(`Downloaded impact assessment: ${JSON.stringify(impactAssessment)}`);
554-
555552
// will further update the label context if necessary
556553
processImpactAssessment(labelContext, impactAssessment);
557554
}
@@ -719,15 +716,9 @@ export async function getCheckRunTuple(
719716
}
720717

721718
const filteredReqCheckRuns = reqCheckRuns.filter(
722-
/**
723-
* @param {CheckRunData} checkRun
724-
*/
725719
(checkRun) => !excludedCheckNames.includes(checkRun.name),
726720
);
727721
const filteredFyiCheckRuns = fyiCheckRuns.filter(
728-
/**
729-
* @param {CheckRunData} checkRun
730-
*/
731722
(checkRun) => !excludedCheckNames.includes(checkRun.name),
732723
);
733724

‎.github/workflows/summarize-checks.yaml‎

Lines changed: 3 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -55,41 +55,9 @@ jobs:
5555
uses: actions/github-script@v7
5656
with:
5757
script: |
58-
console.log(`Event name: ${context.eventName}`);
59-
console.log(`Action: ${context.payload.action}`);
60-
61-
if (context.eventName === 'workflow_run' && context.payload.workflow_run) {
62-
const run = context.payload.workflow_run;
63-
console.log(`Triggering workflow: ${run.name}`);
64-
console.log(`Triggering workflow ID: ${run.id}`);
65-
console.log(`Triggering run ID: ${run.run_number}`);
66-
console.log(`Triggering event: ${run.event}`);
67-
console.log(`Triggering status: ${run.status}`);
68-
console.log(`Triggering conclusion: ${run.conclusion}`);
69-
console.log(`Triggering branch: ${run.head_branch}`);
70-
console.log(`Triggering SHA: ${run.head_sha}`);
71-
console.log(`Triggering actor: ${run.actor?.login || 'unknown'}`);
72-
73-
// Create clickable URL to the triggering workflow run
74-
const triggeringRunUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${run.id}`;
75-
console.log(`🔗 Triggering workflow run URL: ${triggeringRunUrl}`);
76-
77-
// If there's a pull request associated, show that too
78-
if (run.pull_requests && run.pull_requests.length > 0) {
79-
run.pull_requests.forEach(pr => {
80-
const prUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/pull/${pr.number}`;
81-
console.log(`🔗 Associated PR: ${prUrl}`);
82-
});
83-
}
84-
} else if (context.eventName === 'pull_request_target' && context.payload.pull_request) {
85-
const pr = context.payload.pull_request;
86-
console.log(`PR number: ${pr.number}`);
87-
console.log(`PR title: ${pr.title}`);
88-
console.log(`PR state: ${pr.state}`);
89-
console.log(`PR head SHA: ${pr.head.sha}`);
90-
console.log(`PR base branch: ${pr.base.ref}`);
91-
console.log(`🔗 PR URL: ${pr.html_url}`);
92-
}
58+
const { default: dumpTriggerMetadata } =
59+
await import('${{ github.workspace }}/.github/workflows/src/summarize-checks/dump-trigger-metadata.js');
60+
return await dumpTriggerMetadata({ context, core });
9361
9462
- id: summarize-checks
9563
name: Summarize Checks

0 commit comments

Comments
 (0)