Skip to content

feat(ncu-ci): parse comments to find a safe commit #920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions lib/ci/run_ci.js
Original file line number Diff line number Diff line change
Expand Up @@ -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})\.*\n*$/;

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;
Expand All @@ -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() {
Expand Down Expand Up @@ -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 <commit-full-sha-or-URL>');
cli.error('Refusing to run CI on potentially unsafe PR');
return false;
}
Expand Down
1 change: 1 addition & 0 deletions lib/queries/PRComments.gql
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ query Comments($prid: Int!, $owner: String!, $repo: String!, $after: String) {
}
nodes {
publishedAt
body
bodyText
author {
login
Expand Down
73 changes: 58 additions & 15 deletions test/unit/ci_start.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -19,14 +20,15 @@ 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) {
const stubbed = sinon.stub(FormData.prototype, 'append').callsFake(function(key, value) {
assert.strictEqual(key, 'json');
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,
Expand All @@ -41,8 +43,10 @@ describe('Jenkins', () => {

this._validated = true;

return FormData.prototype.append.wrappedMethod.bind(this)(key, value);
return Reflect.apply(FormData.prototype.append.wrappedMethod, this, arguments);
});

return () => stubbed.restore();
});

it('should fail if starting node-pull-request throws', async() => {
Expand All @@ -54,7 +58,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);
});

Expand All @@ -64,7 +68,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);
});

Expand Down Expand Up @@ -92,7 +96,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());
});

Expand All @@ -111,22 +115,61 @@ 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() {});
return () => {
PRData.prototype.getReviews.restore();
PRData.prototype.getCommits.restore();
};
});
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(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({
Expand All @@ -151,7 +194,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);
});
}
});
Expand Down
Loading