diff --git a/.github/workflows/validate-github-github-docs-urls.yml b/.github/workflows/validate-github-github-docs-urls.yml index cbf03d3a1a..7092d8565c 100644 --- a/.github/workflows/validate-github-github-docs-urls.yml +++ b/.github/workflows/validate-github-github-docs-urls.yml @@ -8,7 +8,15 @@ on: workflow_dispatch: schedule: - cron: '20 16 * * *' # Run every day at 16:20 UTC / 8:20 PST - # pull_request: # Temporarily commented out. See internal issue 4152 + pull_request: + paths: + - 'content/**' + # In case a relevant dependency changes + - 'package*.json' + # The scripts + - 'src/links/scripts/validate-github-github-docs-urls/**' + # The workflow + - .github/workflows/validate-github-github-docs-urls.yml permissions: contents: read @@ -90,13 +98,28 @@ jobs: # In the latter case, you might want to update the URL in docs-urls.json # after this PR has landed, or consider using `` as a # workaround for the time being. + # First, gather the URLs that were relevant + - name: Get changed content/data files + id: changed-files + uses: tj-actions/changed-files@77af4bed286740ef1a6387dc4e4e4dec39f96054 # v43.0.0 + with: + # No need to escape the file names because we make the output of + # tj-actions/changed-files be set as an environment variable. Not + # as a direct input to the line of bash that uses it. + safe_output: false + files: | + content/** - name: Generate PR comment - if: ${{ github.event_name == 'pull_request' }} + if: ${{ github.event_name == 'pull_request' && steps.changed-files.outputs.any_changed == 'true' }} env: + # Make it an environment variable so that its value doesn't need to be escaped. + # See https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable + CHANGED_FILES: |- + ${{ steps.changed-files.outputs.all_changed_files }} GITHUB_TOKEN: ${{ secrets.DOCS_BOT_PAT_WRITEORG_PROJECT }} ISSUE_NUMBER: ${{ github.event.pull_request.number }} REPOSITORY: ${{ github.repository }} - run: npm run validate-github-github-docs-urls -- post-pr-comment checks.json + run: npm run validate-github-github-docs-urls -- post-pr-comment checks.json --changed-files $CHANGED_FILES - uses: ./.github/actions/slack-alert if: ${{ failure() && github.event_name == 'schedule' }} diff --git a/src/links/scripts/validate-github-github-docs-urls/index.ts b/src/links/scripts/validate-github-github-docs-urls/index.ts index af1da7f3f9..79cac3a2aa 100644 --- a/src/links/scripts/validate-github-github-docs-urls/index.ts +++ b/src/links/scripts/validate-github-github-docs-urls/index.ts @@ -32,6 +32,11 @@ program ) .option('--dry-run', "Don't post any comment. Only print what it would post.") .option('--fail-on-error', 'Any error will make the process exit with a non-zero code') + .option( + '--changed-files [paths...]', + 'Content files that we can map to URLs for inclusion', + process.env.CHANGED_FILES || '', + ) .argument('', 'JSON file that has all checks') .action(postPRComment) diff --git a/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts b/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts index cd8c88e74e..0baf13af9b 100644 --- a/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts +++ b/src/links/scripts/validate-github-github-docs-urls/post-pr-comment.ts @@ -11,6 +11,13 @@ type PostPRCommentOptions = { repository: string dryRun: boolean failOnError?: boolean + // If someone uses ` ... --changed-files`, Commander will set this to + // boolean `true`. + // If someone uses ` ... --changed-files foo bar`, the value + // becomes `['foo', 'bar']`. + // And since it defaults to an env var called `CHANGED_FILES`, + // it could be a string like `'foo bar'`. + changedFiles?: string | string[] | true } // This function is designed to be able to run and potentially do nothing. @@ -19,25 +26,59 @@ export async function postPRComment(filePath: string, options: PostPRCommentOpti if (!options.dryRun) { if (!options.issueNumber) { throw new Error( - 'You must provide an issue number. Either set ISSUE_NUMBER env var or pass the --issue-number flag. Remember, a PR is an issue actually.', + 'If not in dry-run mode, you must provide an issue number. Either set ISSUE_NUMBER env var or pass the --issue-number flag. Remember, a PR is an issue actually.', ) } if (!options.repository) { throw new Error( - 'You must provide a repository name. Either set REPOSITORY env var or pass the --repository flag.', + 'If not in dry-run mode, you must provide a repository name. Either set REPOSITORY env var or pass the --repository flag.', ) } } + // See note on `PostPRCommentOptions` type about this + if (options.changedFiles === true) { + throw new Error( + 'If you use --changed-files, you must provide at least one file path. For example, --changed-files foo.md bar.md', + ) + } + // Exit early if there's absolutely nothing to "complain" about const checks: Check[] = JSON.parse(fs.readFileSync(filePath, 'utf8')) + const changedFiles: string[] = [] + if (options.changedFiles) { + if (Array.isArray(options.changedFiles)) { + changedFiles.push(...options.changedFiles) + } else if (typeof options.changedFiles === 'string') { + changedFiles.push(...options.changedFiles.split(/\s+/g)) + } else { + throw new Error('Unexpected type for changedFiles') + } + } + + const checksFiltered = checks.filter((check: Check) => { + return ( + !changedFiles.length || + changedFiles.some((changedFile) => contentFileMatchesURL(changedFile, check.url)) + ) + }) + + if (checksFiltered.length !== checks.length && checks.length > 0) { + console.warn( + boxen( + `Due to filtering by changed files (${changedFiles.length}) there are now ${checksFiltered.length} checks to process instead of ${checks.length}.`, + { padding: 1 }, + ), + ) + } + // Really bad. This could lead to a 404 from links in GitHub. - const failedChecks = checks.filter((check) => !check.found) + const failedChecks = checksFiltered.filter((check) => !check.found) // Bad. This could lead to the fragment not finding the right // heading in the found page. - const failedFragmentChecks = checks.filter( + const failedFragmentChecks = checksFiltered.filter( (check) => check.found && check.fragment && !check.fragmentFound, ) @@ -143,6 +184,16 @@ Remember, this workflow check is not required because it's not guaranteed to be } } +function contentFileMatchesURL(filePath: string, url: string) { + if (!filePath.startsWith('content/')) return false + + // This strips and omits any query string or hash + const pathname = new URL(url, 'https://docs.github.com').pathname + + const fileUrl = filePath.replace('content', '').replace('/index.md', '').replace(/\.md$/, '') + return pathname === fileUrl +} + function makeAbsoluteDocsURL(check: Check) { let absURL = `https://docs.github.com${check.pageURL}` if (check.fragment) {