From f7aca4f4813e004946c4aa1353eabc8ce7c04ce5 Mon Sep 17 00:00:00 2001 From: Eli Reisman Date: Mon, 3 Jun 2024 15:42:37 -0700 Subject: [PATCH] refactor to dedup min summary generation --- src/comment-pr.ts | 19 +----- src/main.ts | 31 ++++++--- src/summary.ts | 169 +++++++++++++++++----------------------------- 3 files changed, 85 insertions(+), 134 deletions(-) diff --git a/src/comment-pr.ts b/src/comment-pr.ts index f1142b4..d4729ac 100644 --- a/src/comment-pr.ts +++ b/src/comment-pr.ts @@ -5,6 +5,8 @@ import * as retry from '@octokit/plugin-retry' import {RequestError} from '@octokit/request-error' import {ConfigurationOptions} from './schemas' +export const MAX_COMMENT_LENGTH = 65536 + const retryingOctokit = githubUtils.GitHub.plugin(retry.retry) const octo = new retryingOctokit( githubUtils.getOctokitOptions(core.getInput('repo-token', {required: true})) @@ -12,19 +14,11 @@ const octo = new retryingOctokit( // Comment Marker to identify an existing comment to update, so we don't spam the PR with comments const COMMENT_MARKER = '' -const MAX_COMMENT_LENGTH = 65536 export async function commentPr( - summary: typeof core.summary, + commentContent: string, config: ConfigurationOptions, - minComment: string ): Promise { - const commentContent = summary.stringify() - - // this should be truncated for us if it's too long but - // we could check len and sub in minSummary instead - core.setOutput('comment-content', commentContent) - if ( !( config.comment_summary_in_pr === 'always' || @@ -44,13 +38,6 @@ export async function commentPr( let commentBody = `${commentContent}\n\n${COMMENT_MARKER}` - if (commentBody.length >= MAX_COMMENT_LENGTH) { - core.debug( - 'The comment was too big for the GitHub API. Falling back on a minimum comment' - ) - commentBody = `${minComment}\n\n${COMMENT_MARKER}` - } - try { const existingCommentId = await findCommentByMarker(COMMENT_MARKER) diff --git a/src/main.ts b/src/main.ts index 282eb1d..2440672 100644 --- a/src/main.ts +++ b/src/main.ts @@ -22,9 +22,13 @@ import * as summary from './summary' import {getRefs} from './git-refs' import {groupDependenciesByManifest} from './utils' -import {commentPr} from './comment-pr' +import { + commentPr, + MAX_COMMENT_LENGTH +} from './comment-pr' import {getDeniedChanges} from './deny' + async function delay(ms: number): Promise { return new Promise(resolve => setTimeout(resolve, ms)) } @@ -127,14 +131,7 @@ async function run(): Promise { const scorecard = await getScorecardLevels(filteredChanges) - summary.addSummaryToSummary( - vulnerableChanges, - invalidLicenseChanges, - deniedChanges, - scorecard, - config - ) - const minSummary = summary.getMinSummaryForComment( + const minSummary = summary.addSummaryToSummary( vulnerableChanges, invalidLicenseChanges, deniedChanges, @@ -173,7 +170,21 @@ async function run(): Promise { core.setOutput('dependency-changes', JSON.stringify(changes)) summary.addScannedDependencies(changes) printScannedDependencies(changes) - await commentPr(core.summary, config, minSummary) + + // include full summary in output; Actions will truncate if oversized + let rendered = core.summary.stringify() + core.setOutput('content-comment', rendered) + + // if the summary is oversized, replace with minimal version + if (rendered.length >= MAX_COMMENT_LENGTH) { + core.debug( + 'The comment was too big for the GitHub API. Falling back on a minimum comment' + ) + rendered = minSummary + } + + // update the PR comment if needed with the right-sized summary + await commentPr(rendered, config) } catch (error) { if (error instanceof RequestError && error.status === 404) { core.setFailed( diff --git a/src/summary.ts b/src/summary.ts index 18ea6ce..02e3d26 100644 --- a/src/summary.ts +++ b/src/summary.ts @@ -10,81 +10,23 @@ const icons = { warning: '⚠️' } -export function getMinSummaryForComment( - vulnerableChanges: Changes, - invalidLicenseChanges: InvalidLicenseChanges, - deniedChanges: Changes, - scorecard: Scorecard, - config: ConfigurationOptions -): string { - const scorecardWarnings = countScorecardWarnings(scorecard, config) - const licenseIssues = countLicenseIssues(invalidLicenseChanges) - - let minSummary = '# Dependency Review\n' - - if ( - vulnerableChanges.length === 0 && - licenseIssues === 0 && - deniedChanges.length === 0 && - scorecardWarnings === 0 - ) { - const issueTypes = [ - config.vulnerability_check ? 'vulnerabilities' : '', - config.license_check ? 'license issues' : '', - config.show_openssf_scorecard ? 'OpenSSF Scorecard issues' : '' - ] - if (issueTypes.filter(Boolean).length === 0) { - minSummary += `${icons.check} No issues found.` - } else { - minSummary += `${icons.check} No ${issueTypes.filter(Boolean).join(' or ')} found.` - } - } - - minSummary += 'The following issues were found:\n' - minSummary += config.vulnerability_check - ? `* ${checkOrFailIcon(vulnerableChanges.length)} ${ - vulnerableChanges.length - } vulnerable package(s)\n` - : '' - minSummary += config.license_check - ? `* ${checkOrFailIcon(invalidLicenseChanges.forbidden.length)} ${ - invalidLicenseChanges.forbidden.length - } package(s) with incompatible licenses\n -* ${checkOrFailIcon(invalidLicenseChanges.unresolved.length)} ${ - invalidLicenseChanges.unresolved.length - } package(s) with invalid SPDX license definitions\n -* ${checkOrWarnIcon(invalidLicenseChanges.unlicensed.length)} ${ - invalidLicenseChanges.unlicensed.length - } package(s) with unknown licenses.\n` - : '' - minSummary += - deniedChanges.length > 0 - ? `* ${checkOrWarnIcon(deniedChanges.length)} ${ - deniedChanges.length - } package(s) denied.\n` - : '' - minSummary += - config.show_openssf_scorecard && scorecardWarnings > 0 - ? `* ${checkOrWarnIcon(scorecardWarnings)} ${scorecardWarnings ? scorecardWarnings : 'No'} packages with OpenSSF Scorecard issues.\n` - : '' - - // Add the link to the job summary provided by GitHub Actions for this workflow run - minSummary += `\n[View full job summary](${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID})` - - return minSummary -} - +// generates the initial DR summmary and applies to the Action's core.summary +// returns a string array of all the formatted values in the event we need +// to replace them in the PR comment due to length restrictions export function addSummaryToSummary( vulnerableChanges: Changes, invalidLicenseChanges: InvalidLicenseChanges, deniedChanges: Changes, scorecard: Scorecard, - config: ConfigurationOptions -): void { + config: ConfigurationOptions, +): string { + let out: string[] = []; + const scorecardWarnings = countScorecardWarnings(scorecard, config) const licenseIssues = countLicenseIssues(invalidLicenseChanges) core.summary.addHeading('Dependency Review', 1) + out.push('# Dependency Review') if ( vulnerableChanges.length === 0 && @@ -97,54 +39,65 @@ export function addSummaryToSummary( config.license_check ? 'license issues' : '', config.show_openssf_scorecard ? 'OpenSSF Scorecard issues' : '' ] + + let msg = '' if (issueTypes.filter(Boolean).length === 0) { - core.summary.addRaw(`${icons.check} No issues found.`) + msg = `${icons.check} No issues found.` } else { - core.summary.addRaw( - `${icons.check} No ${issueTypes.filter(Boolean).join(' or ')} found.` - ) + msg = `${icons.check} No ${issueTypes.filter(Boolean).join(' or ')} found.` } - return + core.summary.addRaw(msg) + out.push(msg) + return out.join('\n') } - core.summary - .addRaw('The following issues were found:') - .addList([ - ...(config.vulnerability_check - ? [ - `${checkOrFailIcon(vulnerableChanges.length)} ${ - vulnerableChanges.length - } vulnerable package(s)` - ] - : []), - ...(config.license_check - ? [ - `${checkOrFailIcon(invalidLicenseChanges.forbidden.length)} ${ - invalidLicenseChanges.forbidden.length - } package(s) with incompatible licenses`, - `${checkOrFailIcon(invalidLicenseChanges.unresolved.length)} ${ - invalidLicenseChanges.unresolved.length - } package(s) with invalid SPDX license definitions`, - `${checkOrWarnIcon(invalidLicenseChanges.unlicensed.length)} ${ - invalidLicenseChanges.unlicensed.length - } package(s) with unknown licenses.` - ] - : []), - ...(deniedChanges.length > 0 - ? [ - `${checkOrFailIcon(deniedChanges.length)} ${ - deniedChanges.length - } package(s) denied.` - ] - : []), - ...(config.show_openssf_scorecard && scorecardWarnings > 0 - ? [ - `${checkOrWarnIcon(scorecardWarnings)} ${scorecardWarnings ? scorecardWarnings : 'No'} packages with OpenSSF Scorecard issues.` - ] - : []) - ]) - .addRaw('See the Details below.') + const foundIssuesHeader = 'The following issues were found:' + core.summary.addRaw(foundIssuesHeader) + out.push(foundIssuesHeader) + + const summaryList: string[] = [ + ...(config.vulnerability_check + ? [ + `${checkOrFailIcon(vulnerableChanges.length)} ${ + vulnerableChanges.length + } vulnerable package(s)` + ] + : []), + ...(config.license_check + ? [ + `${checkOrFailIcon(invalidLicenseChanges.forbidden.length)} ${ + invalidLicenseChanges.forbidden.length + } package(s) with incompatible licenses`, + `${checkOrFailIcon(invalidLicenseChanges.unresolved.length)} ${ + invalidLicenseChanges.unresolved.length + } package(s) with invalid SPDX license definitions`, + `${checkOrWarnIcon(invalidLicenseChanges.unlicensed.length)} ${ + invalidLicenseChanges.unlicensed.length + } package(s) with unknown licenses.` + ] + : []), + ...(deniedChanges.length > 0 + ? [ + `${checkOrWarnIcon(deniedChanges.length)} ${ + deniedChanges.length + } package(s) denied.` + ] + : []), + ...(config.show_openssf_scorecard && scorecardWarnings > 0 + ? [ + `${checkOrWarnIcon(scorecardWarnings)} ${scorecardWarnings ? scorecardWarnings : 'No'} packages with OpenSSF Scorecard issues.` + ] + : []) + ]; + + core.summary.addList(summaryList) + out.push(...summaryList) + + core.summary.addRaw('See the Details below.') + out.push(`\n[View full job summary](${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID})`) + + return out.join('\n') } function countScorecardWarnings(