From 4d5e9bb410c8daf77eff85fb85f9edb8adcb78ef Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 14 Aug 2020 21:40:30 -0700 Subject: [PATCH] feat(land): add skipChecks option skipChecks will allow skipping certain checks. Useful to skip "commit after last approval" on commit queue. Ref: https://github.com/nodejs/node/issues/34770 --- components/git/land.js | 25 +++++++++++++++++-- components/metadata.js | 4 +-- lib/pr_checker.js | 43 ++++++++++++++++++++++++-------- test/unit/pr_checker.test.js | 48 ++++++++++++++++++++++++++++-------- 4 files changed, 96 insertions(+), 24 deletions(-) diff --git a/components/git/land.js b/components/git/land.js index 0c5bfc76..599d8d2d 100644 --- a/components/git/land.js +++ b/components/git/land.js @@ -4,11 +4,14 @@ const { parsePRFromURL } = require('../../lib/links'); const getMetadata = require('../metadata'); const CLI = require('../../lib/cli'); const Request = require('../../lib/request'); +const PRChecker = require('../../lib/pr_checker'); const { runPromise } = require('../../lib/run'); const LandingSession = require('../../lib/landing_session'); const epilogue = require('./epilogue'); const yargs = require('yargs'); +const availableChecks = Object.values(PRChecker.availableChecks); + const landOptions = { apply: { describe: 'Apply a patch with the given PR id', @@ -42,6 +45,21 @@ const landOptions = { describe: 'Land a backport PR onto a staging branch', default: false, type: 'boolean' + }, + skipChecks: { + describe: 'Comma-separated list of checks to skip while landing. ' + + `Available options are: ${availableChecks}`, + coerce: (arg) => { + const checks = arg.split(','); + for (const check of checks) { + if (!availableChecks.includes(check)) { + throw new Error(`Invalid check '${check}'. Available options are: ` + + `${availableChecks}`); + } + } + return checks; + }, + type: 'string' } }; @@ -89,7 +107,10 @@ function handler(argv) { const provided = []; for (const type of Object.keys(landOptions)) { - if (type === 'yes') continue; // --yes is not an action + if (['yes', 'skipChecks'].includes(type)) { + // Those are not actions + continue; + } if (argv[type]) { provided.push(type); } @@ -160,7 +181,7 @@ async function main(state, argv, cli, req, dir) { return; } session = new LandingSession(cli, req, dir, argv.prid, argv.backport); - const metadata = await getMetadata(session.argv, cli); + const metadata = await getMetadata(session.argv, cli, argv.skipChecks); if (argv.backport) { const split = metadata.metadata.split('\n')[0]; if (split === 'PR-URL: ') { diff --git a/components/metadata.js b/components/metadata.js index 499bd99f..bfc9f9c0 100644 --- a/components/metadata.js +++ b/components/metadata.js @@ -9,7 +9,7 @@ const MetadataGenerator = require('../lib/metadata_gen'); const fs = require('fs'); -module.exports = async function getMetadata(argv, cli) { +module.exports = async function getMetadata(argv, cli, skipChecks = []) { const credentials = await auth({ github: true, jenkins: true @@ -40,7 +40,7 @@ module.exports = async function getMetadata(argv, cli) { cli.separator(); const checker = new PRChecker(cli, data, request, argv); - const status = await checker.checkAll(argv.checkComments); + const status = await checker.checkAll(argv.checkComments, skipChecks); return { status, request, diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 15f20921..a678e80d 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -28,6 +28,16 @@ const { PRBuild } = require('./ci/build-types/pr_build'); const GIT_CONFIG_GUIDE_URL = 'https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork'; +const CHECKS = { + COMMIT_AFTER_REVIEW: 'commit-after-review', + CI: 'ci', + REVIEWS_AND_WAIT: 'reviews-and-wait', + MERGEABLE_STATE: 'mergeable-state', + PR_STATE: 'pr-state', + GIT_CONFIG: 'git-config', + AUTHOR: 'author' +}; + class PRChecker { /** * @param {{}} cli @@ -55,6 +65,10 @@ class PRChecker { ); } + static get availableChecks() { + return CHECKS; + } + get waitTimeSingleApproval() { if (this.argv.waitTimeSingleApproval === undefined) { return WAIT_TIME_SINGLE_APPROVAL; @@ -69,24 +83,33 @@ class PRChecker { return this.argv.waitTimeMultiApproval; } - async checkAll(checkComments = false) { - const status = [ - this.checkCommitsAfterReview(), - await this.checkCI(), - this.checkReviewsAndWait(new Date(), checkComments), - this.checkMergeableState(), - this.checkPRState(), - this.checkGitConfig() + async checkAll(checkComments = false, exclude = []) { + const checks = [ + [CHECKS.COMMIT_AFTER_REVIEW, async() => this.checkCommitsAfterReview()], + [CHECKS.CI, async() => this.checkCI()], + [CHECKS.REVIEWS_AND_WAIT, + async() => this.checkReviewsAndWait(new Date(), checkComments)], + [CHECKS.MERGEABLE_STATE, async() => this.checkMergeableState()], + [CHECKS.PR_STATE, async() => this.checkPRState()], + [CHECKS.GIT_CONFIG, async() => this.checkGitConfig()] ]; if (this.data.authorIsNew()) { - status.push(this.checkAuthor()); + checks.push([CHECKS.AUTHOR, async() => this.checkAuthor()]); + } + + const status = []; + for (const [checkName, check] of checks) { + if (exclude.includes(checkName)) { + continue; + } + status.push(await check()); } // TODO: check for pre-backport, Github API v4 // does not support reading files changed - return status.every((i) => i); + return status.every(s => s); } getTSC(people) { diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 202be715..be678e29 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -59,6 +59,7 @@ describe('PRChecker', () => { return PRData.prototype.getThread.call(this); } }; + const checks = PRChecker.availableChecks; const checker = new PRChecker(cli, data, {}, argv); let checkReviewsAndWaitStub; @@ -69,18 +70,20 @@ describe('PRChecker', () => { let checkPRState; let checkGitConfigStub; - before(() => { - checkReviewsAndWaitStub = sinon.stub(checker, 'checkReviewsAndWait'); - checkCIStub = sinon.stub(checker, 'checkCI'); - checkAuthorStub = sinon.stub(checker, 'checkAuthor'); + beforeEach(() => { + checkReviewsAndWaitStub = + sinon.stub(checker, 'checkReviewsAndWait').returns(true); + checkCIStub = sinon.stub(checker, 'checkCI').returns(true); + checkAuthorStub = sinon.stub(checker, 'checkAuthor').returns(true); checkCommitsAfterReviewStub = - sinon.stub(checker, 'checkCommitsAfterReview'); - checkMergeableStateStub = sinon.stub(checker, 'checkMergeableState'); - checkPRState = sinon.stub(checker, 'checkPRState'); - checkGitConfigStub = sinon.stub(checker, 'checkGitConfig'); + sinon.stub(checker, 'checkCommitsAfterReview').returns(true); + checkMergeableStateStub = + sinon.stub(checker, 'checkMergeableState').returns(true); + checkPRState = sinon.stub(checker, 'checkPRState').returns(true); + checkGitConfigStub = sinon.stub(checker, 'checkGitConfig').returns(true); }); - after(() => { + afterEach(() => { checkReviewsAndWaitStub.restore(); checkCIStub.restore(); checkAuthorStub.restore(); @@ -92,7 +95,7 @@ describe('PRChecker', () => { it('should run necessary checks', async() => { const status = await checker.checkAll(); - assert.strictEqual(status, false); + assert.strictEqual(status, true); assert.strictEqual(checkReviewsAndWaitStub.calledOnce, true); assert.strictEqual(checkCIStub.calledOnce, true); assert.strictEqual(checkAuthorStub.calledOnce, true); @@ -101,6 +104,31 @@ describe('PRChecker', () => { assert.strictEqual(checkPRState.calledOnce, true); assert.strictEqual(checkGitConfigStub.calledOnce, true); }); + + it('should run no checks if all excluded', async() => { + const status = await checker.checkAll(false, Object.values(checks)); + assert.strictEqual(status, true); + assert.strictEqual(checkReviewsAndWaitStub.called, false); + assert.strictEqual(checkCIStub.called, false); + assert.strictEqual(checkAuthorStub.called, false); + assert.strictEqual(checkCommitsAfterReviewStub.called, false); + assert.strictEqual(checkMergeableStateStub.called, false); + assert.strictEqual(checkPRState.called, false); + assert.strictEqual(checkGitConfigStub.called, false); + }); + + it('should skip excluded checks', async() => { + const status = + await checker.checkAll(false, [checks.COMMIT_AFTER_REVIEW, checks.CI]); + assert.strictEqual(status, true); + assert.strictEqual(checkReviewsAndWaitStub.calledOnce, true); + assert.strictEqual(checkCIStub.called, false); + assert.strictEqual(checkAuthorStub.calledOnce, true); + assert.strictEqual(checkCommitsAfterReviewStub.called, false); + assert.strictEqual(checkMergeableStateStub.calledOnce, true); + assert.strictEqual(checkPRState.calledOnce, true); + assert.strictEqual(checkGitConfigStub.calledOnce, true); + }); }); describe('checkReviewsAndWait', () => {