From 5d1a760c0ab6ee6f8df20c759735ef20a7ada1fb Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 5 Apr 2025 18:21:53 +0200 Subject: [PATCH 1/4] feat(ncu-ci): parse comments to find a safe commit --- lib/ci/run_ci.js | 26 +++++++++++++++++++++++--- lib/queries/PRComments.gql | 1 + 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index 1561030b..404eace1 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -16,6 +16,26 @@ export const CI_PR_URL = `https://${CI_DOMAIN}/job/${CI_PR_NAME}/build`; const CI_V8_NAME = CI_TYPES.get(CI_TYPES_KEYS.V8).jobName; export const CI_V8_URL = `https://${CI_DOMAIN}/job/${CI_V8_NAME}/build`; +const REQUEST_CI_COMMENT = /^@nodejs-github-bot .+([0-9a-f]{40})$/; + +async function findSafeFullCommitSHA(cli, prData, request) { + // First, let's check if the head was approved. + await Promise.all([prData.getReviews(), prData.getCommits()]); + + const approvedHead = new PRChecker(cli, prData, request, {}).getApprovedTipOfHead(); + if (approvedHead) return approvedHead; + + // Else, let's find out if a collaborator added a comment with a full commit SHA. + await Promise.all([prData.getCollaborators(), prData.getComments()]); + const collaborators = Array.from(prData.collaborators.values(), + (c) => c.login.toLowerCase()); + return prData.comments + .findLast(c => + collaborators.includes(c.author.login.toLowerCase()) && + REQUEST_CI_COMMENT.test(c.body)) + ?.body.match(REQUEST_CI_COMMENT)[1]; +} + export class RunPRJob { constructor(cli, request, owner, repo, prid, certifySafe) { this.cli = cli; @@ -26,9 +46,7 @@ export class RunPRJob { this.prData = new PRData({ prid, owner, repo }, cli, request); this.certifySafe = certifySafe || - Promise.all([this.prData.getReviews(), this.prData.getCommits()]).then(() => - (this.certifySafe = new PRChecker(cli, this.prData, request, {}).getApprovedTipOfHead()) - ); + findSafeFullCommitSHA(cli, this.prData, request).then((h) => (this.certifySafe = h)); } async getCrumb() { @@ -72,6 +90,8 @@ export class RunPRJob { const { cli, certifySafe } = this; if (!(await certifySafe)) { + cli.info('If the tip of this PR looks safe, add a comment in the form of:'); + cli.info('@nodejs-github-bot test '); cli.error('Refusing to run CI on potentially unsafe PR'); return false; } diff --git a/lib/queries/PRComments.gql b/lib/queries/PRComments.gql index 5234dc44..faafa9d1 100644 --- a/lib/queries/PRComments.gql +++ b/lib/queries/PRComments.gql @@ -9,6 +9,7 @@ query Comments($prid: Int!, $owner: String!, $repo: String!, $after: String) { } nodes { publishedAt + body bodyText author { login From 51ce389dc1d539216d30bba0986a8c270801d65f Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 16:12:21 +0200 Subject: [PATCH 2/4] fix tests --- test/unit/ci_start.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index c56e5308..2dc58902 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -11,6 +11,7 @@ import { CI_PR_URL } from '../../lib/ci/run_ci.js'; import PRChecker from '../../lib/pr_checker.js'; +import PRData from '../../lib/pr_data.js'; import TestCLI from '../fixtures/test_cli.js'; @@ -125,6 +126,10 @@ describe('Jenkins', () => { }safe`, async() => { const cli = new TestCLI(); + sinon.replace(PRData.prototype, 'getCollaborators', + function() { this.collaborators = []; }); + sinon.replace(PRData.prototype, 'getComments', + function() { this.comments = []; }); sinon.replace(PRChecker.prototype, 'getApprovedTipOfHead', sinon.fake.returns(certifySafe && 'deadbeef')); From 37696bd8512f56e2aac8dc9a11a7417b911e36ac Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 17:28:11 +0200 Subject: [PATCH 3/4] add tests --- lib/ci/run_ci.js | 2 +- test/unit/ci_start.test.js | 67 ++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js index 404eace1..e8e00b4a 100644 --- a/lib/ci/run_ci.js +++ b/lib/ci/run_ci.js @@ -16,7 +16,7 @@ export const CI_PR_URL = `https://${CI_DOMAIN}/job/${CI_PR_NAME}/build`; const CI_V8_NAME = CI_TYPES.get(CI_TYPES_KEYS.V8).jobName; export const CI_V8_URL = `https://${CI_DOMAIN}/job/${CI_V8_NAME}/build`; -const REQUEST_CI_COMMENT = /^@nodejs-github-bot .+([0-9a-f]{40})$/; +const REQUEST_CI_COMMENT = /^@nodejs-github-bot .+([0-9a-f]{40})\.*\n*$/; async function findSafeFullCommitSHA(cli, prData, request) { // First, let's check if the head was approved. diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index 2dc58902..dc9bce85 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -20,6 +20,7 @@ describe('Jenkins', () => { const repo = 'node-auto-test'; const prid = 123456; const crumb = 'asdf1234'; + const dummySHA = '51ce389dc1d539216d30bba0986a8c270801d65f'; before(() => { sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) { @@ -27,7 +28,7 @@ describe('Jenkins', () => { const { parameter } = JSON.parse(value); const expectedParameters = { CERTIFY_SAFE: 'on', - COMMIT_SHA_CHECK: 'deadbeef', + COMMIT_SHA_CHECK: dummySHA, TARGET_GITHUB_ORG: owner, TARGET_REPO_NAME: repo, PR_ID: prid, @@ -42,7 +43,7 @@ describe('Jenkins', () => { this._validated = true; - return FormData.prototype.append.wrappedMethod.bind(this)(key, value); + return Reflect.apply(FormData.prototype.append.wrappedMethod, this, arguments); }); }); @@ -55,7 +56,7 @@ describe('Jenkins', () => { .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); @@ -65,7 +66,7 @@ describe('Jenkins', () => { json: sinon.stub().throws() }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, true); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); @@ -93,7 +94,7 @@ describe('Jenkins', () => { json: sinon.stub().withArgs(CI_CRUMB_URL) .returns(Promise.resolve({ crumb })) }; - const jobRunner = new RunPRJob(cli, request, owner, repo, prid, 'deadbeef'); + const jobRunner = new RunPRJob(cli, request, owner, repo, prid, dummySHA); assert.ok(await jobRunner.start()); }); @@ -112,26 +113,56 @@ 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, dummySHA); assert.strictEqual(await jobRunner.start(), false); }); describe('without --certify-safe flag', { concurrency: false }, () => { + before(() => { + sinon.replace(PRData.prototype, 'getReviews', function() {}); + sinon.replace(PRData.prototype, 'getCommits', function() {}); + }); afterEach(() => { - sinon.restore(); + PRData.prototype.getCollaborators.restore(); + PRData.prototype.getComments.restore(); + PRChecker.prototype.getApprovedTipOfHead.restore(); }); - for (const certifySafe of [true, false]) { - it(`should return ${certifySafe} if PR checker reports it as ${ - certifySafe ? '' : 'potentially un' - }safe`, async() => { + for (const { headIsApproved = false, collaborators = [], comments = [], expected } of [{ + headIsApproved: true, + expected: true, + }, { + headIsApproved: false, + expected: false, + }, { + collaborators: ['foo'], + comments: [{ login: 'foo' }], + expected: true, + }, { + // Validates that passing full commit URL also works. + collaborators: ['foo'], + comments: [{ login: 'foo', body: `@nodejs-github-bot test https://github.com/nodejs/node/commit/${dummySHA}.\n` }], + expected: true, + }, { + // Validates that non-collaborator commenting should have no effect. + collaborators: ['foo'], + comments: [{ login: 'bar' }], + expected: false, + }]) { + it(`should return ${expected} with ${ + JSON.stringify({ headIsApproved, collaborators, comments })}`, async() => { const cli = new TestCLI(); - sinon.replace(PRData.prototype, 'getCollaborators', - function() { this.collaborators = []; }); - sinon.replace(PRData.prototype, 'getComments', - function() { this.comments = []; }); - sinon.replace(PRChecker.prototype, 'getApprovedTipOfHead', - sinon.fake.returns(certifySafe && 'deadbeef')); + sinon.stub(PRData.prototype, 'getCollaborators').callsFake(function() { + this.collaborators = collaborators.map(login => ({ login })); + }); + sinon.stub(PRData.prototype, 'getComments').callsFake(function() { + this.comments = comments.map(({ body, login }) => ({ + body: body ?? `@nodejs-github-bot test ${dummySHA}`, + author: { login } + })); + }); + sinon.stub(PRChecker.prototype, 'getApprovedTipOfHead').callsFake( + sinon.fake.returns(headIsApproved && dummySHA)); const request = { gql: sinon.stub().returns({ @@ -156,7 +187,7 @@ describe('Jenkins', () => { }; const jobRunner = new RunPRJob(cli, request, owner, repo, prid, false); - assert.strictEqual(await jobRunner.start(), certifySafe); + assert.strictEqual(await jobRunner.start(), expected); }); } }); From 733bd44a743adcce14bc17a789e297a1907fed99 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 6 Apr 2025 17:33:57 +0200 Subject: [PATCH 4/4] cleaner teardown --- test/unit/ci_start.test.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/unit/ci_start.test.js b/test/unit/ci_start.test.js index dc9bce85..e03b4fbe 100644 --- a/test/unit/ci_start.test.js +++ b/test/unit/ci_start.test.js @@ -23,7 +23,7 @@ describe('Jenkins', () => { const dummySHA = '51ce389dc1d539216d30bba0986a8c270801d65f'; before(() => { - sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) { + const stubbed = sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) { assert.strictEqual(key, 'json'); const { parameter } = JSON.parse(value); const expectedParameters = { @@ -45,6 +45,8 @@ describe('Jenkins', () => { return Reflect.apply(FormData.prototype.append.wrappedMethod, this, arguments); }); + + return () => stubbed.restore(); }); it('should fail if starting node-pull-request throws', async() => { @@ -121,12 +123,17 @@ describe('Jenkins', () => { before(() => { sinon.replace(PRData.prototype, 'getReviews', function() {}); sinon.replace(PRData.prototype, 'getCommits', function() {}); + return () => { + PRData.prototype.getReviews.restore(); + PRData.prototype.getCommits.restore(); + }; }); afterEach(() => { PRData.prototype.getCollaborators.restore(); PRData.prototype.getComments.restore(); PRChecker.prototype.getApprovedTipOfHead.restore(); }); + for (const { headIsApproved = false, collaborators = [], comments = [], expected } of [{ headIsApproved: true, expected: true,