From 299f6876b253501b333f5db274d3832961fbcae8 Mon Sep 17 00:00:00 2001 From: Jared Forsyth Date: Wed, 4 Aug 2021 13:56:55 -0600 Subject: [PATCH] Update to use new actions-utils that is aware of ALL_CHANGED_FILES (#28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: See https://github.com/Khan/actions-utils/pull/14 ## Test plan: I updated the workflow to set up the ALL_CHANGED_FILES variable. Our other workflows that use eslint-action will want to add those setup steps as well. Author: jaredly Reviewers: jeresig, jaredly Required Reviewers: Approved by: jeresig Checks: ❌ lint_and_unit, ✅ autofix Pull request URL: https://github.com/Khan/eslint-action/pull/28 --- .github/workflow-templates/_setup.yml | 21 +- .github/workflow-templates/pr-actions.yml | 10 +- .github/workflows/pr-actions.yml | 17 +- .github/workflows/pr-autofix.yml | 4 +- dist/index.js | 275 ++++++++++++---------- package.json | 2 +- yarn.lock | 9 + 7 files changed, 198 insertions(+), 140 deletions(-) diff --git a/.github/workflow-templates/_setup.yml b/.github/workflow-templates/_setup.yml index bf0810b..40f250a 100644 --- a/.github/workflow-templates/_setup.yml +++ b/.github/workflow-templates/_setup.yml @@ -1,9 +1,26 @@ setup: checkout: - - uses: actions/checkout@v1 - name: get the repo + - uses: actions/checkout@v2 yarn: setup: checkout steps: - run: yarn + + # This gets the list of files that changed in the current pull-request + # and puts then onto the ALL_CHANGED_FILES env variable, which several + # of our actions will use. + changed-files: + steps: + - name: Get All Changed Files + uses: jaredly/get-changed-files@v1.0.1 + id: changed + with: + format: 'json' # robust to filenames with spaces + absolute: true # our tooling expects absolute paths + + # Now we put it on the environment so that it can be picked up by + # anything that uses actions-utils (e.g. eslint-action, jest-action, etc.) + - uses: allenevans/set-env@v2.0.0 + with: + ALL_CHANGED_FILES: '${{ steps.changed.outputs.added_modified }}' diff --git a/.github/workflow-templates/pr-actions.yml b/.github/workflow-templates/pr-actions.yml index 5cfa67c..34273f7 100644 --- a/.github/workflow-templates/pr-actions.yml +++ b/.github/workflow-templates/pr-actions.yml @@ -11,7 +11,7 @@ jobs: steps: - name: Run eslint uses: Khan/eslint-action@main - setup: yarn + setup: [yarn, changed-files] with: eslint-lib: ./node_modules/eslint env: @@ -19,7 +19,7 @@ jobs: - name: Run jest tests uses: Khan/jest-action@main - setup: yarn + setup: [yarn, changed-files] with: jest-bin: ./node_modules/.bin/jest env: @@ -27,7 +27,7 @@ jobs: - name: Run jest coverage uses: Khan/jest-coverage-action@main - setup: yarn + setup: [yarn, changed-files] with: jest-bin: ./node_modules/.bin/jest coverage-data-path: ./coverage/coverage-final.json @@ -36,7 +36,7 @@ jobs: - name: Run flow uses: Khan/flow-action@main - setup: yarn + setup: [yarn, changed-files] with: flow-bin: ./node_modules/.bin/flow env: @@ -44,7 +44,7 @@ jobs: - name: Run flow coverage uses: Khan/flow-coverage-action@main - setup: yarn + setup: [yarn, changed-files] with: flow-bin: ./node_modules/.bin/flow env: diff --git a/.github/workflows/pr-actions.yml b/.github/workflows/pr-actions.yml index a12230c..e2920b6 100644 --- a/.github/workflows/pr-actions.yml +++ b/.github/workflows/pr-actions.yml @@ -7,8 +7,21 @@ jobs: lint_and_unit: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v1 - name: '▶️ Setup checkout: get the repo' + - uses: actions/checkout@v2 + name: '▶️ Setup checkout: ' + - name: "\U0001F53D Start setup [changed-files]" + run: echo "Setting something up" + - name: Get All Changed Files + uses: jaredly/get-changed-files@v1.0.1 + id: changed + with: + format: json + absolute: true + - uses: allenevans/set-env@v2.0.0 + with: + ALL_CHANGED_FILES: '${{ steps.changed.outputs.added_modified }}' + - name: "\U0001F53C Finished setup [changed-files]" + run: echo "Finished setting it up" - run: yarn name: '▶️ Setup yarn: ' - name: Run eslint diff --git a/.github/workflows/pr-autofix.yml b/.github/workflows/pr-autofix.yml index 719ee66..9dadae8 100644 --- a/.github/workflows/pr-autofix.yml +++ b/.github/workflows/pr-autofix.yml @@ -8,8 +8,8 @@ jobs: runs-on: ubuntu-latest if: github.actor != 'khan-actions-bot' steps: - - uses: actions/checkout@v1 - name: '▶️ Setup checkout: get the repo' + - uses: actions/checkout@v2 + name: '▶️ Setup checkout: ' - id: paths__github_workflow_templates_ name: 'Check paths: .github/workflow-templates/**' run: >- diff --git a/dist/index.js b/dist/index.js index 9d3a814..0e8d1f5 100755 --- a/dist/index.js +++ b/dist/index.js @@ -2577,94 +2577,94 @@ function _loadMjsDefault() { * TODO(jared): Consider using the github pull-request API (if we're online) * to determine the base branch. */ -const { execSync, spawnSync } = __webpack_require__(3129); +const {execSync, spawnSync} = __webpack_require__(3129); -const checkRef = (ref) => spawnSync("git", ["rev-parse", ref]).status === 0; +const checkRef = ref => spawnSync('git', ['rev-parse', ref]).status === 0; -const validateBaseRef = (baseRef /*:string*/) => { - // It's locally accessible! - if (checkRef(baseRef)) { - return baseRef; - } - // If it's not locally accessible, then it's probably a remote branch - const remote = `refs/remotes/origin/${baseRef}`; - if (checkRef(remote)) { - return remote; - } +const validateBaseRef = (baseRef /*:string*/) /*: string | null */ => { + // It's locally accessible! + if (checkRef(baseRef)) { + return baseRef; + } + // If it's not locally accessible, then it's probably a remote branch + const remote = `refs/remotes/origin/${baseRef}`; + if (checkRef(remote)) { + return remote; + } - // Otherwise return null - no valid ref provided - return null; + // Otherwise return null - no valid ref provided + return null; }; -const getBaseRef = (head /*:string*/ = "HEAD") => { - const { GITHUB_BASE_REF } = process.env; - if (GITHUB_BASE_REF) { - return validateBaseRef(GITHUB_BASE_REF); - } else { - let upstream = execSync(`git rev-parse --abbrev-ref '${head}@{upstream}'`, { - encoding: "utf8", - }); - upstream = upstream.trim(); +const getBaseRef = (head /*:string*/ = 'HEAD') /*: string | null */ => { + const {GITHUB_BASE_REF} = process.env; + if (GITHUB_BASE_REF) { + return validateBaseRef(GITHUB_BASE_REF); + } else { + let upstream = execSync(`git rev-parse --abbrev-ref '${head}@{upstream}'`, { + encoding: 'utf8', + }); + upstream = upstream.trim(); - // if upstream is local and not empty, use that. - if (upstream && !upstream.trim().startsWith("origin/")) { - return `refs/heads/${upstream}`; - } - let headRef = execSync(`git rev-parse --abbrev-ref ${head}`, { - encoding: "utf8", - }); - headRef = headRef.trim(); - for (let i = 1; i < 100; i++) { - try { - const stdout = execSync( - `git branch --contains ${head}~${i} --format='%(refname)'`, - { encoding: "utf8" } - ); - let lines = stdout.split("\n").filter(Boolean); - lines = lines.filter((line) => line !== `refs/heads/${headRef}`); - - // Note (Lilli): When running our actions locally, we want to be a little more - // aggressive in choosing a baseRef, going back to a shared commit on only `develop`, - // `master`, feature or release branches, so that we can cover more commits. In case, - // say, I create a bunch of experimental, first-attempt, throw-away branches that - // share commits higher in my stack... - for (const line of lines) { - if ( - line === "refs/heads/develop" || - line === "refs/heads/master" || - line.startsWith("refs/heads/feature/") || - line.startsWith("refs/heads/release/") - ) { - return line; - } + // if upstream is local and not empty, use that. + if (upstream && !upstream.trim().startsWith('origin/')) { + return `refs/heads/${upstream}`; + } + let headRef = execSync(`git rev-parse --abbrev-ref ${head}`, { + encoding: 'utf8', + }); + headRef = headRef.trim(); + for (let i = 1; i < 100; i++) { + try { + const stdout = execSync( + `git branch --contains ${head}~${i} --format='%(refname)'`, + {encoding: 'utf8'}, + ); + let lines = stdout.split('\n').filter(Boolean); + lines = lines.filter(line => line !== `refs/heads/${headRef}`); + + // Note (Lilli): When running our actions locally, we want to be a little more + // aggressive in choosing a baseRef, going back to a shared commit on only `develop`, + // `master`, feature or release branches, so that we can cover more commits. In case, + // say, I create a bunch of experimental, first-attempt, throw-away branches that + // share commits higher in my stack... + for (const line of lines) { + if ( + line === 'refs/heads/develop' || + line === 'refs/heads/master' || + line.startsWith('refs/heads/feature/') || + line.startsWith('refs/heads/release/') + ) { + return line; + } + } + } catch { + // Ran out of history, probably + return null; + } } - } catch { - // Ran out of history, probably + // We couldn't find it return null; - } } - // We couldn't find it - return null; - } }; // Multiple action microservices might encounter this, so give them a canned message to print. // Logging from inside this lib didn't seem to make it to the GitHub Actions console, so I'll // just pass the string back for them to log. const cannedGithubErrorMessage = () /*:string*/ => { - const { GITHUB_BASE_REF } = process.env; + const {GITHUB_BASE_REF} = process.env; - return GITHUB_BASE_REF - ? `No valid base ref given. Found \`${GITHUB_BASE_REF}\`, but \`${GITHUB_BASE_REF}\` does not ` + - `appear to be a valid branch. Perhaps this is coming from a GitHub pull-request that ` + - `you reparented, and the old parent no longer exists. This is a bug on GitHub; unless ` + - `you push a new commit, the old base ref won't update. You can try solving this by: \n` + - `\t1. Merging the new base branch into your pull-request and re-running your checks.\n` + - `\t2. Rebasing the new base branch into your pull-request and re-running your checks.\n` + - `\t3. Creating and pushing an empty commit (e.g., \`$ git commit --allow-empty -m ` + - `'Trigger checks' && git push\`).` - : `No valid base ref given. The envar \`GITHUB_BASE_REF\` was null and no other base ref could ` + - `be determined.`; + return GITHUB_BASE_REF + ? `No valid base ref given. Found \`${GITHUB_BASE_REF}\`, but \`${GITHUB_BASE_REF}\` does not ` + + `appear to be a valid branch. Perhaps this is coming from a GitHub pull-request that ` + + `you reparented, and the old parent no longer exists. This is a bug on GitHub; unless ` + + `you push a new commit, the old base ref won't update. You can try solving this by: \n` + + `\t1. Merging the new base branch into your pull-request and re-running your checks.\n` + + `\t2. Rebasing your pull-request branch onto the new base branch and re-running your checks.\n` + + `\t3. Creating and pushing an empty commit (e.g., \`$ git commit --allow-empty -m ` + + `'Trigger checks' && git push\`).` + : `No valid base ref given. The envar \`GITHUB_BASE_REF\` was null and no other base ref could ` + + `be determined.`; }; module.exports = getBaseRef; @@ -36290,7 +36290,7 @@ module.exports = keys; * the GITHUB_TOKEN env variable. */ -const { GITHUB_TOKEN, GITHUB_WORKSPACE } = process.env; +const {GITHUB_TOKEN, GITHUB_WORKSPACE} = process.env; const fs = __webpack_require__(5747); const path = __webpack_require__(5622); const chalk = __webpack_require__(4946); @@ -36318,7 +36318,7 @@ const localReport = async (title /*:string*/, messages /*:Array*/) => { console.log(chalk.yellow(`[[ ${title} ]]`)); console.log(); const fileCache /*: {[key: string]: Array}*/ = {}; - const getFile = (filePath) => { + const getFile = filePath => { if (!fileCache[filePath]) { const ext = path.extname(filePath).slice(1); fileCache[filePath] = highlight(fs.readFileSync(filePath, 'utf8'), { @@ -36329,7 +36329,7 @@ const localReport = async (title /*:string*/, messages /*:Array*/) => { return fileCache[filePath]; }; const byFile /*:{[key: string]: number}*/ = {}; - messages.forEach((message) => { + messages.forEach(message => { const lines = getFile(message.path); const lineStart = Math.max(message.start.line - 3, 0); const indexStart = lineStart + 1; @@ -36341,9 +36341,7 @@ const localReport = async (title /*:string*/, messages /*:Array*/) => { } console.error( ':error:', - chalk.cyan( - `${message.path}:${message.start.line}:${message.start.column}`, - ), + chalk.cyan(`${message.path}:${message.start.line}:${message.start.column}`), ); console.error(message.message); console.error( @@ -36395,8 +36393,8 @@ const githubReport = async ( messages /*: Array*/, ) => { /* flow-uncovered-block */ - const { GitHub, context } = __webpack_require__(1469); - const { owner, repo } /*: {owner: string, repo: string}*/ = context.repo; + const {GitHub, context} = __webpack_require__(1469); + const {owner, repo} /*: {owner: string, repo: string}*/ = context.repo; const client = new GitHub(token, {}); const headSha = context.payload.pull_request.head.sha; const check = await client.checks.create({ @@ -36423,7 +36421,7 @@ const githubReport = async ( } /* end flow-uncovered-block */ - const annotations = messages.map((message) => ({ + const annotations = messages.map(message => ({ path: removeWorkspace(message.path), start_line: message.start.line, end_line: message.end.line, @@ -36432,7 +36430,7 @@ const githubReport = async ( })); let errorCount = 0; let warningCount = 0; - messages.forEach((message) => { + messages.forEach(message => { if (message.annotationLevel === 'failure') { errorCount += 1; } else { @@ -36463,7 +36461,7 @@ const githubReport = async ( } }; -const makeReport = (title /*: string*/, messages /*: Array*/) => { +const makeReport = (title /*: string*/, messages /*: Array*/) /*: Promise */ => { if (GITHUB_TOKEN) { return githubReport(title, GITHUB_TOKEN, messages); } else { @@ -86535,12 +86533,13 @@ function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; const execProm = __webpack_require__(5822); const path = __webpack_require__(5622); const fs = __webpack_require__(5747); -const minimatch = __webpack_require__(7093); +const minimatch = __webpack_require__(7093); // flow-uncovered-line -const getIgnoredPatterns = (fileContents) => { +// ok +const getIgnoredPatterns = (fileContents /*: string*/) => { return fileContents .split('\n') - .map((line) => { + .map(line => { if (line.startsWith('#')) { return null; } @@ -86551,10 +86550,7 @@ const getIgnoredPatterns = (fileContents) => { return null; } const [pattern, ...attributes] = line.trim().split(' '); - if ( - attributes.includes('binary') || - attributes.includes('linguist-generated=true') - ) { + if (attributes.includes('binary') || attributes.includes('linguist-generated=true')) { return pattern; } return null; @@ -86562,8 +86558,8 @@ const getIgnoredPatterns = (fileContents) => { .filter(Boolean); }; -const ignoredPatternsByDirectory = {}; -const isFileIgnored = (workingDirectory, file) => { +const ignoredPatternsByDirectory /*: {[key: string]: Array}*/ = {}; +const isFileIgnored = (workingDirectory /*: string*/, file /*: string*/) => { // If it's outside of the "working directory", we ignore it if (!file.startsWith(workingDirectory)) { return true; @@ -86582,6 +86578,7 @@ const isFileIgnored = (workingDirectory, file) => { } } for (const pattern of ignoredPatternsByDirectory[dir]) { + // flow-next-uncovered-line if (minimatch(name, pattern)) { return true; } @@ -86595,24 +86592,46 @@ const isFileIgnored = (workingDirectory, file) => { /** * This lists the files that have changed when compared to `base` (a git ref), * limited to the files that are a descendent of `cwd`. + * It also respects '.gitattributes', filtering out files that have been marked + * as "binary" or "linguist-generated=true". */ -const gitChangedFiles = async ( - base /*:string*/, - cwd /*:string*/, -) /*: Promise>*/ => { +const gitChangedFiles = async (base /*:string*/, cwd /*:string*/) /*: Promise>*/ => { cwd = path.resolve(cwd); - const { stdout } = await execProm( - `git diff --name-only ${base} --relative`, - { cwd, encoding: 'utf8', rejectOnError: true }, - ); + + // Github actions jobs can run the following steps to get a fully accurate + // changed files list. Otherwise, we fallback to a simple diff between the + // current and base branch, which might give false positives if the base + // is ahead of the current branch. + // + // - name: Get All Changed Files + // uses: jaredly/get-changed-files@absolute + // id: changed + // with: + // format: 'json' + // absolute: true + // + // - uses: allenevans/set-env@v2.0.0 + // with: + // ALL_CHANGED_FILES: '${{ steps.changed.outputs.added_modified }}' + // + if (process.env.ALL_CHANGED_FILES) { + const files /*: Array */ = JSON.parse(process.env.ALL_CHANGED_FILES); // flow-uncovered-line + return files.filter(path => !isFileIgnored(cwd, path)); + } + + const {stdout} = await execProm(`git diff --name-only ${base} --relative`, { + cwd, + encoding: 'utf8', + rejectOnError: true, + }); return ( stdout .split('\n') .filter(Boolean) - .map((name) => path.join(cwd, name)) + .map(name => path.join(cwd, name)) // Filter out paths that were deleted - .filter((path) => fs.existsSync(path)) - .filter((path) => !isFileIgnored(cwd, path)) + .filter((path /*: string*/) => fs.existsSync(path)) + .filter((path /*: string*/) => !isFileIgnored(cwd, path)) ); }; @@ -96926,36 +96945,36 @@ exports.quickSort = function (ary, comparator) { /** * A simple promisified version of child_process.exec, so we can `await` it */ -const { spawn } = __webpack_require__(3129); +const {spawn} = __webpack_require__(3129); const execProm = ( - command /*: string*/, - { rejectOnError, ...options } /*: {rejectOnError: boolean} & mixed */ = {} + command /*: string*/, + {rejectOnError, ...options} /*: {rejectOnError: boolean} & mixed */ = {}, ) /*: Promise<{err: ?number, stdout: string, stderr: string}>*/ => - new Promise((res, rej) => { - const proc = spawn( - command, - // $FlowFixMe - { ...options, shell: true } - ); - let stdout = ""; - let stderr = ""; - proc.stdout.setEncoding("utf8"); - proc.stderr.setEncoding("utf8"); - proc.stdout.on("data", (data /*: string*/) => (stdout += data)); - proc.stderr.on("data", (data /*: string*/) => (stderr += data)); - proc.on("close", (code /*: number*/) => { - if (code !== 0 && rejectOnError) { - rej(new Error(`Exited with non-zero error code: ${code}`)); - } else { - res({ - err: code === 0 ? null : code, - stdout, - stderr, + new Promise((res, rej) => { + const proc = spawn( + command, + // $FlowFixMe + {...options, shell: true}, + ); + let stdout = ''; + let stderr = ''; + proc.stdout.setEncoding('utf8'); + proc.stderr.setEncoding('utf8'); + proc.stdout.on('data', (data /*: string*/) => (stdout += data)); + proc.stderr.on('data', (data /*: string*/) => (stderr += data)); + proc.on('close', (code /*: number*/) => { + if (code !== 0 && rejectOnError) { + rej(new Error(`Exited with non-zero error code: ${code}`)); + } else { + res({ + err: code === 0 ? null : code, + stdout, + stderr, + }); + } }); - } }); - }); module.exports = execProm; diff --git a/package.json b/package.json index 56bd1f9..de78f77 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "@babel/core": "^7.9.6", "@babel/register": "^7.9.0", "@zeit/ncc": "^0.22.1", - "actions-utils": "git+https://git@github.com/Khan/actions-utils#v1.2.5", + "actions-utils": "git+https://git@github.com/Khan/actions-utils#v1.3.0", "actions-workflow-tools": "git+https://git@github.com/Khan/actions-workflow-tools#v1.2.5", "babel-eslint": "^10.1.0", "chalk": "^2.4.2", diff --git a/yarn.lock b/yarn.lock index 816ceca..92cfac7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1177,6 +1177,15 @@ acorn@^7.1.1: cli-highlight "^2.1.4" minimatch "^3.0.4" +"actions-utils@git+https://git@github.com/Khan/actions-utils#v1.3.0": + version "1.3.0" + resolved "git+https://git@github.com/Khan/actions-utils#6692c8f36918713728936a957c7ac9660aca374e" + dependencies: + "@actions/github" "^2.1.1" + chalk "^2.4.2" + cli-highlight "^2.1.4" + minimatch "^3.0.4" + "actions-workflow-tools@git+https://git@github.com/Khan/actions-workflow-tools#v1.2.5": version "1.2.4" resolved "git+https://git@github.com/Khan/actions-workflow-tools#a3e71a5eace39ef189da8d7c8c70afe96aefc2fa"