From 3180113e7a62fb283298fa47364d2d6bef6d37b0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 3 Sep 2024 16:50:28 +0200 Subject: [PATCH] ci: Show what size limit jobs exceeded the limit (#13532) Noticed that it was a bit annoying, when we exceeded the size limit, to find out why/what has failed. Now, it should show in the GH comment which specific check has failed, and what the current limit is: ![image](https://github.com/user-attachments/assets/13495922-ac91-42bf-a398-18bb7b0e9e72) While I was at this, I also streamlined the code a bit, extracted it a bit into utils to de-clutter stuff, and fixed/improved some debug logging issues (e.g. the log group was not correctly closed before, ...) --- .../size-limit-gh-action/.eslintrc.cjs | 2 +- dev-packages/size-limit-gh-action/index.mjs | 319 +++--------------- .../utils/SizeLimitFormatter.mjs | 137 ++++++++ .../getArtifactsForBranchAndWorkflow.mjs | 128 +++++++ 4 files changed, 312 insertions(+), 274 deletions(-) create mode 100644 dev-packages/size-limit-gh-action/utils/SizeLimitFormatter.mjs create mode 100644 dev-packages/size-limit-gh-action/utils/getArtifactsForBranchAndWorkflow.mjs diff --git a/dev-packages/size-limit-gh-action/.eslintrc.cjs b/dev-packages/size-limit-gh-action/.eslintrc.cjs index 8c67e0037908..4a92730d3b0b 100644 --- a/dev-packages/size-limit-gh-action/.eslintrc.cjs +++ b/dev-packages/size-limit-gh-action/.eslintrc.cjs @@ -7,7 +7,7 @@ module.exports = { overrides: [ { - files: ['*.mjs'], + files: ['**/*.mjs'], extends: ['@sentry-internal/sdk/src/base'], }, ], diff --git a/dev-packages/size-limit-gh-action/index.mjs b/dev-packages/size-limit-gh-action/index.mjs index 3dbb8aa22127..680d12237bf5 100644 --- a/dev-packages/size-limit-gh-action/index.mjs +++ b/dev-packages/size-limit-gh-action/index.mjs @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { promises as fs } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -9,13 +8,22 @@ import { exec } from '@actions/exec'; import { context, getOctokit } from '@actions/github'; import * as glob from '@actions/glob'; import * as io from '@actions/io'; -import bytes from 'bytes'; import { markdownTable } from 'markdown-table'; +import { SizeLimitFormatter } from './utils/SizeLimitFormatter.mjs'; +import { getArtifactsForBranchAndWorkflow } from './utils/getArtifactsForBranchAndWorkflow.mjs'; + const SIZE_LIMIT_HEADING = '## size-limit report 📦 '; const ARTIFACT_NAME = 'size-limit-action'; const RESULTS_FILE = 'size-limit-results.json'; +function getResultsFilePath() { + const __dirname = path.dirname(fileURLToPath(import.meta.url)); + return path.resolve(__dirname, RESULTS_FILE); +} + +const { getInput, setFailed } = core; + async function fetchPreviousComment(octokit, repo, pr) { const { data: commentList } = await octokit.rest.issues.listComments({ ...repo, @@ -26,114 +34,6 @@ async function fetchPreviousComment(octokit, repo, pr) { return !sizeLimitComment ? null : sizeLimitComment; } -class SizeLimit { - formatBytes(size) { - return bytes.format(size, { unitSeparator: ' ' }); - } - - formatPercentageChange(base = 0, current = 0) { - if (base === 0) { - return 'added'; - } - - if (current === 0) { - return 'removed'; - } - - const value = ((current - base) / base) * 100; - const formatted = (Math.sign(value) * Math.ceil(Math.abs(value) * 100)) / 100; - - if (value > 0) { - return `+${formatted}%`; - } - - if (value === 0) { - return '-'; - } - - return `${formatted}%`; - } - - formatChange(base = 0, current = 0) { - if (base === 0) { - return 'added'; - } - - if (current === 0) { - return 'removed'; - } - - const value = current - base; - const formatted = this.formatBytes(value); - - if (value > 0) { - return `+${formatted} 🔺`; - } - - if (value === 0) { - return '-'; - } - - return `${formatted} 🔽`; - } - - formatLine(value, change) { - return `${value} (${change})`; - } - - formatSizeResult(name, base, current) { - return [ - name, - this.formatBytes(current.size), - this.formatPercentageChange(base.size, current.size), - this.formatChange(base.size, current.size), - ]; - } - - parseResults(output) { - const results = JSON.parse(output); - - return results.reduce((current, result) => { - return { - // biome-ignore lint/performance/noAccumulatingSpread: - ...current, - [result.name]: { - name: result.name, - size: +result.size, - }, - }; - }, {}); - } - - hasSizeChanges(base, current, threshold = 0) { - const names = [...new Set([...(base ? Object.keys(base) : []), ...Object.keys(current)])]; - - return !!names.find(name => { - const baseResult = base?.[name] || EmptyResult; - const currentResult = current[name] || EmptyResult; - - if (baseResult.size === 0 && currentResult.size === 0) { - return true; - } - - return Math.abs((currentResult.size - baseResult.size) / baseResult.size) * 100 > threshold; - }); - } - - formatResults(base, current) { - const names = [...new Set([...(base ? Object.keys(base) : []), ...Object.keys(current)])]; - const header = SIZE_RESULTS_HEADER; - const fields = names.map(name => { - const baseResult = base?.[name] || EmptyResult; - const currentResult = current[name] || EmptyResult; - - return this.formatSizeResult(name, baseResult, currentResult); - }); - - return [header, ...fields]; - } -} - async function execSizeLimit() { let output = ''; @@ -151,15 +51,8 @@ async function execSizeLimit() { return { status, output }; } -const SIZE_RESULTS_HEADER = ['Path', 'Size', '% Change', 'Change']; - -const EmptyResult = { - name: '-', - size: 0, -}; - async function run() { - const { getInput, setFailed } = core; + const __dirname = path.dirname(fileURLToPath(import.meta.url)); try { const { payload, repo } = context; @@ -174,36 +67,12 @@ async function run() { } const octokit = getOctokit(githubToken); - const limit = new SizeLimit(); - const artifactClient = artifact.create(); - const __dirname = path.dirname(fileURLToPath(import.meta.url)); - const resultsFilePath = path.resolve(__dirname, RESULTS_FILE); + const limit = new SizeLimitFormatter(); + const resultsFilePath = getResultsFilePath(); // If we have no comparison branch, we just run size limit & store the result as artifact if (!comparisonBranch) { - let base; - const { output: baseOutput } = await execSizeLimit(); - - try { - base = limit.parseResults(baseOutput); - } catch (error) { - core.error('Error parsing size-limit output. The output should be a json.'); - throw error; - } - - try { - await fs.writeFile(resultsFilePath, JSON.stringify(base), 'utf8'); - } catch (err) { - core.error(err); - } - const globber = await glob.create(resultsFilePath, { - followSymbolicLinks: false, - }); - const files = await globber.glob(); - - await artifactClient.uploadArtifact(ARTIFACT_NAME, files, __dirname); - - return; + return runSizeLimitOnComparisonBranch(); } // Else, we run size limit for the current branch, AND fetch it for the comparison branch @@ -213,12 +82,15 @@ async function run() { let baseWorkflowRun; try { + const workflowName = `${process.env.GITHUB_WORKFLOW || ''}`; + core.startGroup(`getArtifactsForBranchAndWorkflow - workflow:"${workflowName}", branch:"${comparisonBranch}"`); const artifacts = await getArtifactsForBranchAndWorkflow(octokit, { ...repo, artifactName: ARTIFACT_NAME, branch: comparisonBranch, - workflowName: `${process.env.GITHUB_WORKFLOW || ''}`, + workflowName, }); + core.endGroup(); if (!artifacts) { throw new Error('No artifacts found'); @@ -255,9 +127,12 @@ async function run() { const thresholdNumber = Number(threshold); - // @ts-ignore const sizeLimitComment = await fetchPreviousComment(octokit, repo, pr); + if (sizeLimitComment) { + core.debug('Found existing size limit comment, udpating it instead of creating a new one...'); + } + const shouldComment = isNaN(thresholdNumber) || limit.hasSizeChanges(base, current, thresholdNumber) || sizeLimitComment; @@ -269,8 +144,12 @@ async function run() { '⚠️ **Warning:** Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.', ); } - - bodyParts.push(markdownTable(limit.formatResults(base, current))); + try { + bodyParts.push(markdownTable(limit.formatResults(base, current))); + } catch (error) { + core.error('Error generating markdown table'); + core.error(error); + } if (baseWorkflowRun) { bodyParts.push(''); @@ -298,6 +177,8 @@ async function run() { "Error updating comment. This can happen for PR's originating from a fork without write permissions.", ); } + } else { + core.debug('Skipping comment because there are no changes.'); } if (status > 0) { @@ -309,136 +190,28 @@ async function run() { } } -// max pages of workflows to pagination through -const DEFAULT_MAX_PAGES = 50; -// max results per page -const DEFAULT_PAGE_LIMIT = 10; - -/** - * Fetch artifacts from a workflow run from a branch - * - * This is a bit hacky since GitHub Actions currently does not directly - * support downloading artifacts from other workflows - */ -/** - * Fetch artifacts from a workflow run from a branch - * - * This is a bit hacky since GitHub Actions currently does not directly - * support downloading artifacts from other workflows - */ -async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflowName, branch, artifactName }) { - core.startGroup(`getArtifactsForBranchAndWorkflow - workflow:"${workflowName}", branch:"${branch}"`); - - let repositoryWorkflow = null; +async function runSizeLimitOnComparisonBranch() { + const resultsFilePath = getResultsFilePath(); - // For debugging - const allWorkflows = []; - - // - // Find workflow id from `workflowName` - // - for await (const response of octokit.paginate.iterator(octokit.rest.actions.listRepoWorkflows, { - owner, - repo, - })) { - const targetWorkflow = response.data.find(({ name }) => name === workflowName); + const limit = new SizeLimitFormatter(); + const artifactClient = artifact.create(); - allWorkflows.push(...response.data.map(({ name }) => name)); + const { output: baseOutput } = await execSizeLimit(); - // If not found in responses, continue to search on next page - if (!targetWorkflow) { - continue; - } - - repositoryWorkflow = targetWorkflow; - break; - } - - if (!repositoryWorkflow) { - core.info( - `Unable to find workflow with name "${workflowName}" in the repository. Found workflows: ${allWorkflows.join( - ', ', - )}`, - ); - core.endGroup(); - return null; + try { + const base = limit.parseResults(baseOutput); + await fs.writeFile(resultsFilePath, JSON.stringify(base), 'utf8'); + } catch (error) { + core.error('Error parsing size-limit output. The output should be a json.'); + throw error; } - const workflow_id = repositoryWorkflow.id; - - let currentPage = 0; - let latestWorkflowRun = null; - - for await (const response of octokit.paginate.iterator(octokit.rest.actions.listWorkflowRuns, { - owner, - repo, - workflow_id, - branch, - per_page: DEFAULT_PAGE_LIMIT, - event: 'push', - })) { - if (!response.data.length) { - core.warning(`Workflow ${workflow_id} not found in branch ${branch}`); - core.endGroup(); - return null; - } - - // Do not allow downloading artifacts from a fork. - const filtered = response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`); - - // Sort to ensure the latest workflow run is the first - filtered.sort((a, b) => { - return new Date(b.created_at).getTime() - new Date(a.created_at).getTime(); - }); - - // Store the first workflow run, to determine if this is the latest one... - if (!latestWorkflowRun) { - latestWorkflowRun = filtered[0]; - } - - // Search through workflow artifacts until we find a workflow run w/ artifact name that we are looking for - for (const workflowRun of filtered) { - core.info(`Checking artifacts for workflow run: ${workflowRun.html_url}`); - - const { - data: { artifacts }, - } = await octokit.rest.actions.listWorkflowRunArtifacts({ - owner, - repo, - run_id: workflowRun.id, - }); - - if (!artifacts) { - core.warning( - `Unable to fetch artifacts for branch: ${branch}, workflow: ${workflow_id}, workflowRunId: ${workflowRun.id}`, - ); - } else { - const foundArtifact = artifacts.find(({ name }) => name === artifactName); - if (foundArtifact) { - core.info(`Found suitable artifact: ${foundArtifact.url}`); - return { - artifact: foundArtifact, - workflowRun, - isLatest: latestWorkflowRun.id === workflowRun.id, - }; - } else { - core.info(`No artifact found for ${artifactName}, trying next workflow run...`); - } - } - } - - if (currentPage > DEFAULT_MAX_PAGES) { - core.warning(`Workflow ${workflow_id} not found in branch: ${branch}`); - core.endGroup(); - return null; - } - - currentPage++; - } + const globber = await glob.create(resultsFilePath, { + followSymbolicLinks: false, + }); + const files = await globber.glob(); - core.warning(`Artifact not found: ${artifactName}`); - core.endGroup(); - return null; + await artifactClient.uploadArtifact(ARTIFACT_NAME, files, __dirname); } run(); diff --git a/dev-packages/size-limit-gh-action/utils/SizeLimitFormatter.mjs b/dev-packages/size-limit-gh-action/utils/SizeLimitFormatter.mjs new file mode 100644 index 000000000000..034281b38224 --- /dev/null +++ b/dev-packages/size-limit-gh-action/utils/SizeLimitFormatter.mjs @@ -0,0 +1,137 @@ +import * as core from '@actions/core'; +import bytes from 'bytes'; + +const SIZE_RESULTS_HEADER = ['Path', 'Size', '% Change', 'Change']; + +const EmptyResult = { + name: '-', + size: 0, +}; + +export class SizeLimitFormatter { + formatBytes(size) { + return bytes.format(size, { unitSeparator: ' ' }); + } + + formatName(name, sizeLimit, passed) { + if (passed) { + return name; + } + + return `⛔️ ${name} (max: ${this.formatBytes(sizeLimit)})`; + } + + formatPercentageChange(base = 0, current = 0) { + if (base === 0) { + return 'added'; + } + + if (current === 0) { + return 'removed'; + } + + const value = ((current - base) / base) * 100; + const formatted = (Math.sign(value) * Math.ceil(Math.abs(value) * 100)) / 100; + + if (value > 0) { + return `+${formatted}%`; + } + + if (value === 0) { + return '-'; + } + + return `${formatted}%`; + } + + formatChange(base = 0, current = 0) { + if (base === 0) { + return 'added'; + } + + if (current === 0) { + return 'removed'; + } + + const value = current - base; + const formatted = this.formatBytes(value); + + if (value > 0) { + return `+${formatted} 🔺`; + } + + if (value === 0) { + return '-'; + } + + return `${formatted} 🔽`; + } + + formatLine(value, change) { + return `${value} (${change})`; + } + + formatSizeResult(name, base, current) { + if (!current.passed) { + core.debug( + `Size limit exceeded for ${name} - ${this.formatBytes(current.size)} > ${this.formatBytes(current.sizeLimit)}`, + ); + } + + return [ + this.formatName(name, current.sizeLimit, current.passed), + this.formatBytes(current.size), + this.formatPercentageChange(base.size, current.size), + this.formatChange(base.size, current.size), + ]; + } + + parseResults(output) { + const results = JSON.parse(output); + + return results.reduce((current, result) => { + return { + // biome-ignore lint/performance/noAccumulatingSpread: + ...current, + [result.name]: { + name: result.name, + size: +result.size, + sizeLimit: +result.sizeLimit, + passed: result.passed || false, + }, + }; + }, {}); + } + + hasSizeChanges(base, current, threshold = 0) { + if (!base || !current) { + return true; + } + + const names = [...new Set([...Object.keys(base), ...Object.keys(current)])]; + + return names.some(name => { + const baseResult = base[name] || EmptyResult; + const currentResult = current[name] || EmptyResult; + + if (!baseResult.size || !currentResult.size) { + return true; + } + + return Math.abs((currentResult.size - baseResult.size) / baseResult.size) * 100 > threshold; + }); + } + + formatResults(base, current) { + const names = [...new Set([...(base ? Object.keys(base) : []), ...Object.keys(current)])]; + const header = SIZE_RESULTS_HEADER; + const fields = names.map(name => { + const baseResult = base?.[name] || EmptyResult; + const currentResult = current[name] || EmptyResult; + + return this.formatSizeResult(name, baseResult, currentResult); + }); + + return [header, ...fields]; + } +} diff --git a/dev-packages/size-limit-gh-action/utils/getArtifactsForBranchAndWorkflow.mjs b/dev-packages/size-limit-gh-action/utils/getArtifactsForBranchAndWorkflow.mjs new file mode 100644 index 000000000000..6d512b46afe1 --- /dev/null +++ b/dev-packages/size-limit-gh-action/utils/getArtifactsForBranchAndWorkflow.mjs @@ -0,0 +1,128 @@ +import * as core from '@actions/core'; + +// max pages of workflows to pagination through +const DEFAULT_MAX_PAGES = 50; +// max results per page +const DEFAULT_PAGE_LIMIT = 10; + +/** + * Fetch artifacts from a workflow run from a branch + * + * This is a bit hacky since GitHub Actions currently does not directly + * support downloading artifacts from other workflows + */ +/** + * Fetch artifacts from a workflow run from a branch + * + * This is a bit hacky since GitHub Actions currently does not directly + * support downloading artifacts from other workflows + */ +export async function getArtifactsForBranchAndWorkflow(octokit, { owner, repo, workflowName, branch, artifactName }) { + let repositoryWorkflow = null; + + // For debugging + const allWorkflows = []; + + // + // Find workflow id from `workflowName` + // + for await (const response of octokit.paginate.iterator(octokit.rest.actions.listRepoWorkflows, { + owner, + repo, + })) { + const targetWorkflow = response.data.find(({ name }) => name === workflowName); + + allWorkflows.push(...response.data.map(({ name }) => name)); + + // If not found in responses, continue to search on next page + if (!targetWorkflow) { + continue; + } + + repositoryWorkflow = targetWorkflow; + break; + } + + if (!repositoryWorkflow) { + core.info( + `Unable to find workflow with name "${workflowName}" in the repository. Found workflows: ${allWorkflows.join( + ', ', + )}`, + ); + return null; + } + + const workflow_id = repositoryWorkflow.id; + + let currentPage = 0; + let latestWorkflowRun = null; + + for await (const response of octokit.paginate.iterator(octokit.rest.actions.listWorkflowRuns, { + owner, + repo, + workflow_id, + branch, + per_page: DEFAULT_PAGE_LIMIT, + event: 'push', + })) { + if (!response.data.length) { + core.warning(`Workflow ${workflow_id} not found in branch ${branch}`); + return null; + } + + // Do not allow downloading artifacts from a fork. + const filtered = response.data.filter(workflowRun => workflowRun.head_repository.full_name === `${owner}/${repo}`); + + // Sort to ensure the latest workflow run is the first + filtered.sort((a, b) => { + return new Date(b.created_at).getTime() - new Date(a.created_at).getTime(); + }); + + // Store the first workflow run, to determine if this is the latest one... + if (!latestWorkflowRun) { + latestWorkflowRun = filtered[0]; + } + + // Search through workflow artifacts until we find a workflow run w/ artifact name that we are looking for + for (const workflowRun of filtered) { + core.info(`Checking artifacts for workflow run: ${workflowRun.html_url}`); + + const { + data: { artifacts }, + } = await octokit.rest.actions.listWorkflowRunArtifacts({ + owner, + repo, + run_id: workflowRun.id, + }); + + if (!artifacts) { + core.warning( + `Unable to fetch artifacts for branch: ${branch}, workflow: ${workflow_id}, workflowRunId: ${workflowRun.id}`, + ); + } else { + const foundArtifact = artifacts.find(({ name }) => name === artifactName); + if (foundArtifact) { + core.info(`Found suitable artifact: ${foundArtifact.url}`); + return { + artifact: foundArtifact, + workflowRun, + isLatest: latestWorkflowRun.id === workflowRun.id, + }; + } else { + core.info(`No artifact found for ${artifactName}, trying next workflow run...`); + } + } + } + + if (currentPage > DEFAULT_MAX_PAGES) { + core.warning(`Workflow ${workflow_id} not found in branch: ${branch}`); + return null; + } + + currentPage++; + } + + core.warning(`Artifact not found: ${artifactName}`); + core.endGroup(); + return null; +}