Skip to content
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

Fix the max comment length issue #767

Merged
merged 15 commits into from
Jun 4, 2024
Merged
4 changes: 2 additions & 2 deletions __tests__/fixtures/config-allow-sample.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
fail_on_severity: critical
allow_licenses:
- "BSD"
- "GPL 2"
- 'BSD'
elireisman marked this conversation as resolved.
Show resolved Hide resolved
- 'GPL 2'
2 changes: 1 addition & 1 deletion __tests__/fixtures/inline-license-config-sample.yml
Original file line number Diff line number Diff line change
@@ -1 +1 @@
allow-licenses: "MIT, GPL-2.0-only"
allow-licenses: 'MIT, GPL-2.0-only'
38 changes: 37 additions & 1 deletion __tests__/summary.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {expect, jest, test} from '@jest/globals'
import {Changes, ConfigurationOptions, Scorecard} from '../src/schemas'
import {Change, Changes, ConfigurationOptions, Scorecard} from '../src/schemas'
import * as summary from '../src/summary'
import * as core from '@actions/core'
import {createTestChange} from './fixtures/create-test-change'
Expand Down Expand Up @@ -109,6 +109,42 @@ test('prints headline as h1', () => {
expect(text).toContain('<h1>Dependency Review</h1>')
})

test('returns minimal summary in case the core.summary is too large for a PR comment', () => {
let changes: Changes = [
createTestChange({name: 'lodash', version: '1.2.3'}),
createTestChange({name: 'colors', version: '2.3.4'}),
createTestChange({name: '@foo/bar', version: '*'})
]

let minSummary: string = summary.addSummaryToSummary(
elireisman marked this conversation as resolved.
Show resolved Hide resolved
changes,
emptyInvalidLicenseChanges,
emptyChanges,
scorecard,
defaultConfig
)

// side effect DR report into core.summary as happens in main.ts
summary.addScannedDependencies(changes)
const text = core.summary.stringify()

expect(text).toContain('<h1>Dependency Review</h1>')
expect(minSummary).toContain('# Dependency Review')

expect(text).toContain('❌ 3 vulnerable package(s)')
expect(text).not.toContain('* ❌ 3 vulnerable package(s)')
expect(text).toContain('lodash')
expect(text).toContain('colors')
expect(text).toContain('@foo/bar')

expect(minSummary).toContain('* ❌ 3 vulnerable package(s)')
expect(minSummary).not.toContain('lodash')
expect(minSummary).not.toContain('colors')
expect(minSummary).not.toContain('@foo/bar')
elireisman marked this conversation as resolved.
Show resolved Hide resolved

expect(text.length).toBeGreaterThan(minSummary.length)
})

test('only includes "No vulnerabilities or license issues found"-message if both are configured and nothing was found', () => {
summary.addSummaryToSummary(
emptyChanges,
Expand Down
53 changes: 38 additions & 15 deletions dist/index.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

8 changes: 3 additions & 5 deletions src/comment-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}))
Expand All @@ -14,13 +16,9 @@ const octo = new retryingOctokit(
const COMMENT_MARKER = '<!-- dependency-review-pr-comment-marker -->'

export async function commentPr(
summary: typeof core.summary,
commentContent: string,
config: ConfigurationOptions
): Promise<void> {
const commentContent = summary.stringify()

core.setOutput('comment-content', commentContent)
elireisman marked this conversation as resolved.
Show resolved Hide resolved

if (
!(
config.comment_summary_in_pr === 'always' ||
Expand Down
20 changes: 17 additions & 3 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ 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<void> {
Expand Down Expand Up @@ -127,7 +127,7 @@ async function run(): Promise<void> {

const scorecard = await getScorecardLevels(filteredChanges)

summary.addSummaryToSummary(
const minSummary = summary.addSummaryToSummary(
vulnerableChanges,
invalidLicenseChanges,
deniedChanges,
Expand Down Expand Up @@ -166,7 +166,21 @@ async function run(): Promise<void> {
core.setOutput('dependency-changes', JSON.stringify(changes))
summary.addScannedDependencies(changes)
printScannedDependencies(changes)
await commentPr(core.summary, config)

// include full summary in output; Actions will truncate if oversized
let rendered = core.summary.stringify()
core.setOutput('content-comment', rendered)
elireisman marked this conversation as resolved.
Show resolved Hide resolved

// 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(
Expand Down
107 changes: 64 additions & 43 deletions src/summary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,23 @@ const icons = {
warning: '⚠️'
}

// generates the DR report summmary and caches it to the Action's core.summary.
// returns the DR summary string, ready to be posted as a PR comment if the
// final DR report is too large
export function addSummaryToSummary(
vulnerableChanges: Changes,
invalidLicenseChanges: InvalidLicenseChanges,
deniedChanges: Changes,
scorecard: Scorecard,
config: ConfigurationOptions
): void {
): string {
const 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 &&
Expand All @@ -33,54 +39,69 @@ 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)
for (const line of summaryList) {
out.push(`* ${line}`)
}

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(
Expand Down