diff --git a/bin/ncu-ci.js b/bin/ncu-ci.js index 41db7ccb..3ecf9ee0 100755 --- a/bin/ncu-ci.js +++ b/bin/ncu-ci.js @@ -115,9 +115,9 @@ const args = yargs(hideBin(process.argv)) type: 'number' }) .positional('certify-safe', { - describe: 'If not provided, the command will reject PRs that have ' + - 'been pushed since the last review', - type: 'boolean' + describe: 'SHA of the commit that is expected to be at the tip of the PR head. ' + + 'If not provided, the command will use the SHA of the last approved commit.', + type: 'string' }) .option('owner', { default: '', diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index 6edd5833..0c79b99b 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -27,7 +27,7 @@ export class RunPRJob { this.certifySafe = certifySafe || Promise.all([this.prData.getReviews(), this.prData.getPR()]).then(() => - new PRChecker(cli, this.prData, request, {}).checkCommitsAfterReviewOrLabel() + (this.certifySafe = new PRChecker(cli, this.prData, request, {}).getApprovedTipOfHead()) ); } @@ -45,6 +45,7 @@ export class RunPRJob { payload.append('json', JSON.stringify({ parameter: [ { name: 'CERTIFY_SAFE', value: 'on' }, + { name: 'COMMIT_SHA_CHECK', value: this.certifySafe }, { name: 'TARGET_GITHUB_ORG', value: this.owner }, { name: 'TARGET_REPO_NAME', value: this.repo }, { name: 'PR_ID', value: this.prid }, diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 754a89c2..8fff6b32 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -524,38 +524,17 @@ export default class PRChecker { return true; } - async checkCommitsAfterReviewOrLabel() { - if (this.checkCommitsAfterReview()) return true; - - await Promise.all([this.data.getLabeledEvents(), this.data.getCollaborators()]); - - const { - cli, data, pr - } = this; - - const { updatedAt } = pr.timelineItems; - const requestCiLabels = data.labeledEvents.findLast( - ({ createdAt, label: { name } }) => name === 'request-ci' && createdAt > updatedAt - ); - if (requestCiLabels == null) return false; - - const { actor: { login } } = requestCiLabels; - const collaborators = Array.from(data.collaborators.values(), - (c) => c.login.toLowerCase()); - if (collaborators.includes(login.toLowerCase())) { - cli.info('request-ci label was added by a Collaborator after the last push event.'); - return true; - } - - return false; - } - - checkCommitsAfterReview() { + getApprovedTipOfHead() { const { commits, reviews, cli, argv } = this; const { maxCommits } = argv; + if (commits.length === 0) { + cli.warn('No commits found'); + return false; + } + const reviewIndex = reviews.findLastIndex( review => review.authorCanPushToRepository && review.state === 'APPROVED' ); @@ -565,45 +544,32 @@ export default class PRChecker { return false; } - const reviewDate = reviews[reviewIndex].publishedAt; - - const afterCommits = []; - commits.forEach((commit) => { - commit = commit.commit; - if (commit.committedDate > reviewDate) { - afterCommits.push(commit); - } - }); - - const totalCommits = afterCommits.length; - if (totalCommits === 0 && this.pr.timelineItems.updatedAt > reviewDate) { - // Some commits were pushed, but all the commits have a commit date prior - // to the last review. It means that either that a long time elapsed - // between the commit and the push, or that the clock on the dev machine - // is wrong, or the commit date was forged. - cli.warn('Something was pushed to the Pull Request branch since the last approving review.'); - return false; - } + const reviewedCommitIndex = commits + .findLastIndex(({ commit }) => commit.oid === reviews[reviewIndex].commit.oid); - if (totalCommits > 0) { + if (reviewedCommitIndex !== commits.length - 1) { cli.warn('Commits were pushed since the last approving review:'); - const sliceLength = maxCommits === 0 ? totalCommits : -maxCommits; - afterCommits.slice(sliceLength) - .forEach(commit => { + commits.slice(Math.max(reviewedCommitIndex + 1, commits.length - maxCommits)) + .forEach(({ commit }) => { cli.warn(`- ${commit.messageHeadline}`); }); + const totalCommits = commits.length - reviewedCommitIndex - 1; if (totalCommits > maxCommits) { const infoMsg = '...(use `' + - `--max-commits ${totalCommits}` + - '` to see the full list of commits)'; + `--max-commits ${totalCommits}` + + '` to see the full list of commits)'; cli.warn(infoMsg); } return false; } - return true; + return reviews[reviewIndex].commit.oid; + } + + checkCommitsAfterReview() { + return !!this.getApprovedTipOfHead(); } checkMergeableState() { diff --git a/lib/pr_data.js b/lib/pr_data.js index 1ee4adec..53379b8a 100644 --- a/lib/pr_data.js +++ b/lib/pr_data.js @@ -5,7 +5,6 @@ import { } from './user_status.js'; // lib/queries/*.gql file names -const LABELED_EVENTS_QUERY = 'PRLabeledEvents'; const PR_QUERY = 'PR'; const REVIEWS_QUERY = 'Reviews'; const COMMENTS_QUERY = 'PRComments'; @@ -34,7 +33,6 @@ export default class PRData { this.comments = []; this.commits = []; this.reviewers = []; - this.labeledEvents = []; } getThread() { @@ -92,14 +90,6 @@ export default class PRData { ]); } - async getLabeledEvents() { - const { prid, owner, repo, cli, request, prStr } = this; - const vars = { prid, owner, repo }; - cli.updateSpinner(`Getting labels from ${prStr}`); - this.labeledEvents = (await request.gql(LABELED_EVENTS_QUERY, vars)) - .repository.pullRequest.timelineItems.nodes; - } - async getComments() { const { prid, owner, repo, cli, request, prStr } = this; const vars = { prid, owner, repo }; diff --git a/lib/queries/PR.gql b/lib/queries/PR.gql index 22bb803b..27cc1396 100644 --- a/lib/queries/PR.gql +++ b/lib/queries/PR.gql @@ -23,9 +23,6 @@ query PR($prid: Int!, $owner: String!, $repo: String!) { path } }, - timelineItems(itemTypes: [HEAD_REF_FORCE_PUSHED_EVENT, PULL_REQUEST_COMMIT]) { - updatedAt - }, title, baseRefName, headRefName, diff --git a/lib/queries/PRLabeledEvents.gql b/lib/queries/PRLabeledEvents.gql deleted file mode 100644 index f3a0f233..00000000 --- a/lib/queries/PRLabeledEvents.gql +++ /dev/null @@ -1,19 +0,0 @@ -query PRLabeledEvents($prid: Int!, $owner: String!, $repo: String!, $after: String) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $prid) { - timelineItems(itemTypes: LABELED_EVENT, after: $after, last: 100) { - nodes { - ... on LabeledEvent { - actor { - login - } - label { - name - } - createdAt - } - } - } - } - } -} diff --git a/test/fixtures/data.js b/test/fixtures/data.js index b78333fe..b60fe933 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -134,12 +134,3 @@ for (const subdir of readdirSync(path('./jenkins'))) { readJSON(`./jenkins/${subdir}/${item}`); } }; - -export const labeledEvents = {}; - -for (const item of readdirSync(path('./labeled_events'))) { - if (!item.endsWith('.json')) { - continue; - } - labeledEvents[basename(item, '.json')] = readJSON(`./labeled_events/${item}`); -} diff --git a/test/fixtures/first_timer_pr.json b/test/fixtures/first_timer_pr.json index 658434c3..0ae29b6a 100644 --- a/test/fixtures/first_timer_pr.json +++ b/test/fixtures/first_timer_pr.json @@ -22,7 +22,6 @@ } ] }, - "timelineItems": { "updatedAt": "2017-10-24T11:13:43Z" }, "title": "test: awesome changes", "baseRefName": "main", "headRefName": "awesome-changes" diff --git a/test/fixtures/labeled_events/no-request-ci.json b/test/fixtures/labeled_events/no-request-ci.json deleted file mode 100644 index edee9def..00000000 --- a/test/fixtures/labeled_events/no-request-ci.json +++ /dev/null @@ -1,12 +0,0 @@ -[ - { - "actor": { "login": "nodejs-github-bot" }, - "label": { "name": "doc" }, - "createdAt": "2024-05-13T15:57:10Z" - }, - { - "actor": { "login": "nodejs-github-bot" }, - "label": { "name": "test_runner" }, - "createdAt": "2024-05-13T15:57:10Z" - } -] diff --git a/test/fixtures/labeled_events/old-request-ci-collaborator.json b/test/fixtures/labeled_events/old-request-ci-collaborator.json deleted file mode 100644 index 24a16f78..00000000 --- a/test/fixtures/labeled_events/old-request-ci-collaborator.json +++ /dev/null @@ -1,12 +0,0 @@ -[ - { - "actor": { "login": "nodejs-github-bot" }, - "label": { "name": "doc" }, - "createdAt": "2024-05-13T15:57:10Z" - }, - { - "actor": { "login": "foo" }, - "label": { "name": "request-ci" }, - "createdAt": "1999-10-24T11:13:43Z" - } -] diff --git a/test/fixtures/labeled_events/recent-request-ci-collaborator.json b/test/fixtures/labeled_events/recent-request-ci-collaborator.json deleted file mode 100644 index 54792093..00000000 --- a/test/fixtures/labeled_events/recent-request-ci-collaborator.json +++ /dev/null @@ -1,12 +0,0 @@ -[ - { - "actor": { "login": "nodejs-github-bot" }, - "label": { "name": "doc" }, - "createdAt": "2024-05-13T15:57:10Z" - }, - { - "actor": { "login": "foo" }, - "label": { "name": "request-ci" }, - "createdAt": "2024-05-13T15:57:10Z" - } -] diff --git a/test/fixtures/labeled_events/recent-request-ci-non-collaborator.json b/test/fixtures/labeled_events/recent-request-ci-non-collaborator.json deleted file mode 100644 index f799a03c..00000000 --- a/test/fixtures/labeled_events/recent-request-ci-non-collaborator.json +++ /dev/null @@ -1,12 +0,0 @@ -[ - { - "actor": { "login": "nodejs-github-bot" }, - "label": { "name": "doc" }, - "createdAt": "2024-05-13T15:57:10Z" - }, - { - "actor": { "login": "random-person" }, - "label": { "name": "request-ci" }, - "createdAt": "2024-05-13T15:57:10Z" - } -] diff --git a/test/fixtures/more_than_three_commits_after_review_commits.json b/test/fixtures/more_than_three_commits_after_review_commits.json index 49949058..cfd620e9 100644 --- a/test/fixtures/more_than_three_commits_after_review_commits.json +++ b/test/fixtures/more_than_three_commits_after_review_commits.json @@ -21,7 +21,7 @@ },{ "commit": { "committedDate": "2017-09-25T11:27:02Z", - "oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3", + "oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba4", "messageHeadline": "src: add requested feature", "message": "src: new functionality\n works really great", "author": { @@ -31,7 +31,7 @@ },{ "commit": { "committedDate": "2017-10-25T12:42:02Z", - "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1", + "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db2", "messageHeadline": "nit: edit mistakes", "message": "nit: fix mistakes\n fixed requested errors", "author": { @@ -41,7 +41,7 @@ },{ "commit": { "committedDate": "2017-10-25T12:42:02Z", - "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db1", + "oid": "9416475a6dc1b27c3e343dcbc07674a439c88db3", "messageHeadline": "final: we should be good to go", "message": "final: we should be good to go\n fixed requested errors", "author": { diff --git a/test/fixtures/more_than_three_commits_after_review_reviews.json b/test/fixtures/more_than_three_commits_after_review_reviews.json index 039b51d3..e1b3db5f 100644 --- a/test/fixtures/more_than_three_commits_after_review_reviews.json +++ b/test/fixtures/more_than_three_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "commit": {"oid": "f230e691459f8b0f448e1eec1b83fbf7f708eba3"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-07-23T11:19:25Z" diff --git a/test/fixtures/multiple_commits_after_review_reviews.json b/test/fixtures/multiple_commits_after_review_reviews.json index 1307b1eb..6dd2b335 100644 --- a/test/fixtures/multiple_commits_after_review_reviews.json +++ b/test/fixtures/multiple_commits_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-89923489", "publishedAt": "2017-09-23T11:19:25Z" diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json index 4024c5ad..12ccf9a6 100644 --- a/test/fixtures/reviews_approved.json +++ b/test/fixtures/reviews_approved.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:00Z" @@ -14,6 +15,7 @@ "author": { "login": "Baz" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488392", "publishedAt": "2017-10-24T11:50:52Z" @@ -24,6 +26,7 @@ "author": { "login": "Baz" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-714882992", "publishedAt": "2017-10-24T12:30:52Z" @@ -34,6 +37,7 @@ "author": { "login": "Quux" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T14:49:01Z" @@ -44,6 +48,7 @@ "author": { "login": "Baz" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236", "publishedAt": "2017-10-24T14:49:02Z" @@ -54,6 +59,7 @@ "author": { "login": "Quo" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T19:09:52Z" @@ -64,6 +70,7 @@ "author": { "login": "bot" }, + "commit": {"oid": "ffdef335209c77f66d933bd873950747bfe42264"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71839232", "publishedAt": "2017-10-28T19:21:52Z" diff --git a/test/fixtures/semver_major_pr.json b/test/fixtures/semver_major_pr.json index e62a29b5..c5bbe786 100644 --- a/test/fixtures/semver_major_pr.json +++ b/test/fixtures/semver_major_pr.json @@ -16,9 +16,6 @@ } ] }, - "timelineItems": { - "updatedAt": "2017-10-24T11:13:43Z" - }, "title": "lib: awesome changes", "baseRefName": "main", "headRefName": "awesome-changes" diff --git a/test/fixtures/single_commit_after_review_reviews.json b/test/fixtures/single_commit_after_review_reviews.json index c37c880c..2e3f9755 100644 --- a/test/fixtures/single_commit_after_review_reviews.json +++ b/test/fixtures/single_commit_after_review_reviews.json @@ -4,6 +4,7 @@ "author": { "login": "foo" }, + "commit": {"oid": "deadbeef"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624", "publishedAt": "2017-10-24T11:19:25Z" @@ -13,6 +14,7 @@ "author": { "login": "foo" }, + "commit": {"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985"}, "authorCanPushToRepository": true, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480625", "publishedAt": "2017-10-26T11:19:25Z" @@ -22,6 +24,7 @@ "author": { "login": "bar" }, + "commit": {"oid": "6c0945cbeea2cbbc97d13a3d9e3fe68bd145b985"}, "authorCanPushToRepository": false, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71480626", "publishedAt": "2017-10-26T11:19:25Z" diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index 9a89a7f5..c56e5308 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -26,6 +26,7 @@ describe('Jenkins', () => { const { parameter } = JSON.parse(value); const expectedParameters = { CERTIFY_SAFE: 'on', + COMMIT_SHA_CHECK: 'deadbeef', TARGET_GITHUB_ORG: owner, TARGET_REPO_NAME: repo, PR_ID: prid, @@ -91,7 +92,7 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, 'deadbeef'); assert.ok(await jobRunner.start()); }); @@ -124,8 +125,8 @@ describe('Jenkins', () => { }safe`, async() => { const cli = new TestCLI(); - sinon.replace(PRChecker.prototype, 'checkCommitsAfterReview', - sinon.fake.returns(Promise.resolve(certifySafe))); + sinon.replace(PRChecker.prototype, 'getApprovedTipOfHead', + sinon.fake.returns(certifySafe && 'deadbeef')); const request = { gql: sinon.stub().returns({ diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 9b5fa43c..bd98c6e4 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -41,7 +41,6 @@ import { semverMajorPR, conflictingPR, closedPR, - labeledEvents, mergedPR, pullRequests } from '../fixtures/data.js'; @@ -2050,100 +2049,6 @@ describe('PRChecker', () => { }); }); - describe('checkCommitsAfterReviewOrLabel', () => { - it('should return true if PR passes post review checks', async() => { - const cli = new TestCLI(); - const checker = new PRChecker(cli, { - pr: semverMajorPR, - reviewers: allGreenReviewers, - comments: commentsWithLGTM, - reviews: approvingReviews, - commits: simpleCommits, - collaborators, - authorIsNew: () => true, - getThread: PRData.prototype.getThread - }, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, true); - }); - - describe('without approvals', () => { - const data = { - pr: firstTimerPR, - reviewers: noReviewers, - comments: [], - reviews: [], - commits: [], - collaborators: [], - labeledEvents: [], - authorIsNew: () => true, - getThread: PRData.prototype.getThread, - getLabeledEvents: async() => { - data.labeledEvents = []; - }, - getCollaborators: async() => { - data.collaborators = collaborators; - } - }; - - it('should return false if PR has no labels', async() => { - const cli = new TestCLI(); - data.getLabeledEvents = async() => { - data.labeledEvents = []; - }; - const checker = new PRChecker(cli, data, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, false); - }); - - it('should return false if PR has no request-ci label', async() => { - const cli = new TestCLI(); - data.getLabeledEvents = async() => { - data.labeledEvents = labeledEvents['no-request-ci']; - }; - const checker = new PRChecker(cli, data, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, false); - }); - - it('should return false if PR has request-ci from non-collaborator', async() => { - const cli = new TestCLI(); - data.getLabeledEvents = async() => { - data.labeledEvents = labeledEvents['recent-request-ci-non-collaborator']; - }; - const checker = new PRChecker(cli, data, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, false); - }); - - it('should return false if PR has outdated request-ci from a collaborator', async() => { - const cli = new TestCLI(); - data.getLabeledEvents = async() => { - data.labeledEvents = labeledEvents['old-request-ci-collaborator']; - }; - const checker = new PRChecker(cli, data, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, false); - }); - - it('should return true if PR has recent request-ci from a collaborator', async() => { - const cli = new TestCLI(); - data.getLabeledEvents = async() => { - data.labeledEvents = labeledEvents['recent-request-ci-collaborator']; - }; - const checker = new PRChecker(cli, data, {}, argv); - - const status = await checker.checkCommitsAfterReviewOrLabel(); - assert.strictEqual(status, true); - }); - }); - }); - describe('checkCommitsAfterReview', () => { const cli = new TestCLI(); @@ -2248,31 +2153,6 @@ describe('PRChecker', () => { cli.assertCalledWith(expectedLogs); }); - it('should return true if PR can be landed', () => { - const expectedLogs = { - warn: [ - ['Something was pushed to the Pull Request branch since the last approving review.'] - ], - info: [], - error: [] - }; - - const checker = new PRChecker(cli, { - pr: { ...semverMajorPR, timelineItems: { updatedAt: new Date().toISOString() } }, - reviewers: allGreenReviewers, - comments: commentsWithLGTM, - reviews: approvingReviews, - commits: simpleCommits, - collaborators, - authorIsNew: () => true, - getThread: PRData.prototype.getThread - }, {}, argv); - - const status = checker.checkCommitsAfterReview(); - assert.deepStrictEqual(status, false); - cli.assertCalledWith(expectedLogs); - }); - it('should skip the check if there are no reviews', () => { const { commits } = multipleCommitsAfterReview; const expectedLogs = {