-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[summarize-checks] Code cleanup and refactoring #36367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2a19324
e2e2693
b7ca7c2
c1f8417
15ee5a3
7c9c1e3
e5da68b
a3c99ae
a2c1981
dbc2b45
1cbb062
b553939
8791ea3
db0ae69
4e23c82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // @ts-check | ||
|
|
||
| /** | ||
| * @template T | ||
| * @param {Set<T>} a | ||
| * @param {Set<T>} b | ||
| * @returns {Set<T>} | ||
| */ | ||
| export function intersect(a, b) { | ||
| // Since set lookup is O(1), iterate over the smaller set for better perf: O(small) vs O(large) | ||
| const [small, large] = a.size < b.size ? [a, b] : [b, a]; | ||
| return new Set([...small].filter((value) => large.has(value))); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // @ts-check | ||
|
|
||
| import { describe, expect, it } from "vitest"; | ||
| import { intersect } from "../src/set"; | ||
|
|
||
| describe("set", () => { | ||
| it.each([ | ||
| [[], [], []], | ||
| [[1, 2, 3], [], []], | ||
| [ | ||
| [1, 2, 3], | ||
| [2, 3, 4], | ||
| [2, 3], | ||
| ], | ||
| [[1, 2, 3], [4, 5, 6], []], | ||
| [ | ||
| [1, 2, 3], | ||
| [1, 2, 3], | ||
| [1, 2, 3], | ||
| ], | ||
| [ | ||
| ["a", "b", "c"], | ||
| ["b", "c", "d"], | ||
| ["b", "c"], | ||
| ], | ||
| ])( | ||
| "intersect(%o, %o, %o)", | ||
| async ( | ||
| /** @type {(string|number)[]} */ a, | ||
| /** @type {(string|number)[]} */ b, | ||
| /** @type {(string|number)[]} */ result, | ||
| ) => { | ||
| const setA = new Set(a); | ||
| const setB = new Set(b); | ||
| const setResult = new Set(result); | ||
|
|
||
| // Check both orders, result should be same | ||
| expect(intersect(setA, setB)).toEqual(setResult); | ||
| expect(intersect(setB, setA)).toEqual(setResult); | ||
| }, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // @ts-check | ||
|
|
||
| /** | ||
| * @param {import('@actions/github-script').AsyncFunctionArguments} AsyncFunctionArguments | ||
| * @returns {Promise<void>} | ||
| */ | ||
| export default async function dumpTriggerMetadata({ context, core }) { | ||
| core.info(`Event name: ${context.eventName}`); | ||
| core.info(`Action: ${context.payload.action}`); | ||
|
|
||
| if ( | ||
| context.eventName === "workflow_run" && | ||
| context.payload.action === "completed" && | ||
scbedd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| context.payload.workflow_run | ||
| ) { | ||
| const payload = /** @type {import("@octokit/webhooks-types").WorkflowRunCompletedEvent} */ ( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added type comment |
||
| context.payload | ||
| ); | ||
|
|
||
| const run = payload.workflow_run; | ||
| core.info(`Triggering workflow: ${run.name}`); | ||
| core.info(`Triggering workflow ID: ${run.id}`); | ||
| core.info(`Triggering run ID: ${run.run_number}`); | ||
| core.info(`Triggering event: ${run.event}`); | ||
| core.info(`Triggering status: ${run.status}`); | ||
| core.info(`Triggering conclusion: ${run.conclusion}`); | ||
| core.info(`Triggering branch: ${run.head_branch}`); | ||
| core.info(`Triggering SHA: ${run.head_sha}`); | ||
| core.info(`Triggering actor: ${run.actor?.login || "unknown"}`); | ||
|
|
||
| // Create clickable URL to the triggering workflow run | ||
| const triggeringRunUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${run.id}`; | ||
| core.info(`🔗 Triggering workflow run URL: ${triggeringRunUrl}`); | ||
|
|
||
| // If there's a pull request associated, show that too | ||
| if (run.pull_requests && run.pull_requests.length > 0) { | ||
| run.pull_requests.forEach((pr) => { | ||
| const prUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/pull/${pr.number}`; | ||
| core.info(`🔗 Associated PR: ${prUrl}`); | ||
| }); | ||
| } | ||
| } else if (context.eventName === "pull_request_target" && context.payload.pull_request) { | ||
| const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added type comment |
||
| context.payload | ||
| ); | ||
|
|
||
| const pr = payload.pull_request; | ||
| core.info(`PR number: ${pr.number}`); | ||
| core.info(`PR title: ${pr.title}`); | ||
| core.info(`PR state: ${pr.state}`); | ||
| core.info(`PR head SHA: ${pr.head.sha}`); | ||
| core.info(`PR base branch: ${pr.base.ref}`); | ||
| core.info(`🔗 PR URL: ${pr.html_url}`); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| // @ts-check | ||
|
|
||
| /* | ||
|
|
@@ -23,6 +23,7 @@ | |
| // import { commentOrUpdate } from "../comment.js"; | ||
| import { execFile } from "../../../shared/src/exec.js"; | ||
| import { CheckConclusion, PER_PAGE_MAX } from "../../../shared/src/github.js"; | ||
| import { intersect } from "../../../shared/src/set.js"; | ||
| import { | ||
| brChRevApproval, | ||
| getViolatedRequiredLabelsRules, | ||
|
|
@@ -292,12 +293,13 @@ | |
| return; | ||
| } | ||
|
|
||
| // TODO: This is triggered by pull_request_target AND workflow_run. If workflow_run, targetBranch will be undefined. | ||
scbedd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 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}`); | ||
|
|
||
| await summarizeChecksImpl( | ||
| github, | ||
| context, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unused param |
||
| core, | ||
| owner, | ||
| repo, | ||
|
|
@@ -332,7 +334,6 @@ | |
|
|
||
| /** | ||
| * @param {import('@actions/github-script').AsyncFunctionArguments['github']} github | ||
| * @param {import('@actions/github').context } context | ||
| * @param {typeof import("@actions/core")} core | ||
| * @param {string} owner | ||
| * @param {string} repo | ||
|
|
@@ -344,7 +345,6 @@ | |
| */ | ||
| export async function summarizeChecksImpl( | ||
| github, | ||
| context, | ||
| core, | ||
| owner, | ||
| repo, | ||
|
|
@@ -507,8 +507,7 @@ | |
| issue_number: issue_number, | ||
| per_page: PER_PAGE_MAX, | ||
| }); | ||
| /** @type {string[]} */ | ||
| return labels.map((/** @type {{ name: string; }} */ label) => label.name); | ||
| return labels.map((label) => label.name); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to omit type comments if the type can be inferred |
||
| } | ||
|
|
||
| // #endregion | ||
|
|
@@ -518,7 +517,7 @@ | |
| * @param {Set<string>} labelsToRemove | ||
| */ | ||
| function warnIfLabelSetsIntersect(labelsToAdd, labelsToRemove) { | ||
| const intersection = Array.from(labelsToAdd).filter((label) => labelsToRemove.has(label)); | ||
| const intersection = [...intersect(labelsToAdd, labelsToRemove)]; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. our (unofficial) standard is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved |
||
| if (intersection.length > 0) { | ||
| console.warn( | ||
| "ASSERTION VIOLATION! The intersection of labelsToRemove and labelsToAdd is non-empty! " + | ||
|
|
@@ -550,8 +549,6 @@ | |
| }; | ||
|
|
||
| if (impactAssessment) { | ||
| console.log(`Downloaded impact assessment: ${JSON.stringify(impactAssessment)}`); | ||
scbedd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // will further update the label context if necessary | ||
| processImpactAssessment(labelContext, impactAssessment); | ||
| } | ||
|
|
@@ -719,15 +716,9 @@ | |
| } | ||
|
|
||
| const filteredReqCheckRuns = reqCheckRuns.filter( | ||
| /** | ||
| * @param {CheckRunData} checkRun | ||
| */ | ||
| (checkRun) => !excludedCheckNames.includes(checkRun.name), | ||
| ); | ||
| const filteredFyiCheckRuns = fyiCheckRuns.filter( | ||
| /** | ||
| * @param {CheckRunData} checkRun | ||
| */ | ||
| (checkRun) => !excludedCheckNames.includes(checkRun.name), | ||
| ); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from summarize-checks.yaml mostly unchanged. I prefer scripts over a few lines in a separate JS file, for editing ergonomics if nothing else.