diff --git a/packages/auto-approve/README.md b/packages/auto-approve/README.md index 6e03f9d3f6b..f04c01d5433 100644 --- a/packages/auto-approve/README.md +++ b/packages/auto-approve/README.md @@ -33,9 +33,10 @@ processes: - "PythonSampleAppDependency" - "JavaSampleAppDependency" - "NodeGeneratorDependency" + - "OwlBotTemplateChangesNode" ``` -These seven processes represent different workflows for what auto-approve will approve and merge in a given repository. To see their logic in full, see the corresponding file in /src/process-checks. +These processes represent different workflows for what auto-approve will approve and merge in a given repository. To see their logic in full, see the corresponding file in /src/process-checks. Below is what each process checks for: @@ -187,7 +188,13 @@ Below is what each process checks for: - /package.json$/, /\.bzl$/, or /pnpm-lock\.yaml$/ - Increase the non-major package version of a dependency - Change the dependency that was there previously, and that is on the title of the PR - +* OwlBotTemplateChangesNode: + - Checks that the author is 'gcf-owl-bot[bot]' + - Checks that the title of the PR does NOT include BREAKING, feat, fix, ! + - Checks that the title of the PR starts with chore, build, test, refactor + - Checks that the body of the PR does not contain 'PiperOrigin-RevId' + - Checks that the PR is the first of owlbot PRs in the repo (so that they are not merged out of order) + - Checks that there are no other commit authors other than owlbot on the repo This change in configuration permits the following: diff --git a/packages/auto-approve/__snapshots__/check-config.test.js b/packages/auto-approve/__snapshots__/check-config.test.js index 19abb37ff99..603a3d684e4 100644 --- a/packages/auto-approve/__snapshots__/check-config.test.js +++ b/packages/auto-approve/__snapshots__/check-config.test.js @@ -35,7 +35,7 @@ exports['check for config whether YAML file has valid schema should fail if ther ` exports['check for config whether YAML file has valid schema V2 should fail if YAML has any other properties than the ones specified 1'] = ` -[{"wrongProperty":{"allowedValues":["UpdateDiscoveryArtifacts","RegenerateReadme","DiscoveryDocUpdate","PythonDependency","PythonSampleDependency","NodeDependency","NodeRelease","JavaApiaryCodegen","JavaDependency","OwlBotTemplateChanges","OwlBotAPIChanges","PHPApiaryCodegen","PythonSampleAppDependency","JavaSampleAppDependency","DockerDependency","GoDependency","GoApiaryCodegen","NodeGeneratorDependency"]},"message":"must be equal to one of the allowed values"}] +[{"wrongProperty":{"allowedValues":["UpdateDiscoveryArtifacts","RegenerateReadme","DiscoveryDocUpdate","PythonDependency","PythonSampleDependency","NodeDependency","NodeRelease","JavaApiaryCodegen","JavaDependency","OwlBotTemplateChanges","OwlBotTemplateChangesNode","OwlBotAPIChanges","PHPApiaryCodegen","PythonSampleAppDependency","JavaSampleAppDependency","DockerDependency","GoDependency","GoApiaryCodegen","NodeGeneratorDependency"]},"message":"must be equal to one of the allowed values"}] ` exports['check for config whether YAML file has valid schema V2 should fail if the property is wrong 1'] = ` diff --git a/packages/auto-approve/src/check-pr-v2.ts b/packages/auto-approve/src/check-pr-v2.ts index 4b5badfad39..39ec7ba577c 100644 --- a/packages/auto-approve/src/check-pr-v2.ts +++ b/packages/auto-approve/src/check-pr-v2.ts @@ -28,6 +28,7 @@ import {DockerDependency} from './process-checks/sample-application-repos/docker import {NodeDependency} from './process-checks/node/dependency'; import {NodeGeneratorDependency} from './process-checks/node/generator-dependency'; import {NodeRelease} from './process-checks/node/release'; +import {OwlBotTemplateChangesNode} from './process-checks/node/owlbot-template-changes'; import {JavaDependency} from './process-checks/java/dependency'; import {OwlBotTemplateChanges} from './process-checks/owl-bot-template-changes'; import {OwlBotAPIChanges} from './process-checks/owl-bot-api-changes'; @@ -68,6 +69,10 @@ const typeMap = [ configValue: 'NodeRelease', configType: NodeRelease, }, + { + configValue: 'OwlBotTemplateChangesNode', + configType: OwlBotTemplateChangesNode, + }, { configValue: 'JavaApiaryCodegen', configType: JavaApiaryCodegen, diff --git a/packages/auto-approve/src/process-checks/node/owlbot-template-changes.ts b/packages/auto-approve/src/process-checks/node/owlbot-template-changes.ts index 80ea9ab8f83..8ac04d7daca 100644 --- a/packages/auto-approve/src/process-checks/node/owlbot-template-changes.ts +++ b/packages/auto-approve/src/process-checks/node/owlbot-template-changes.ts @@ -17,7 +17,9 @@ import { checkAuthor, checkTitleOrBody, reportIndividualChecks, + getOpenPRsInRepoFromSameAuthor, } from '../../utils-for-pr-checking'; +import {listCommitsOnAPR} from '../../get-pr-info'; import {Octokit} from '@octokit/rest'; import {OwlBotTemplateChanges} from '../owl-bot-template-changes'; @@ -27,12 +29,14 @@ import {OwlBotTemplateChanges} from '../owl-bot-template-changes'; - has an author that is 'gcf-owl-bot[bot]' - has a title that does NOT include BREAKING, or ! - has a PR body that does not contain 'PiperOrigin-RevId' + - is the first owlbot template PR in a repo (so they are merged in order) + - has no other commit authors on the PR */ export class OwlBotTemplateChangesNode extends OwlBotTemplateChanges { classRule = { author: 'gcf-owl-bot[bot]', // For this particular rule, we want to check a pattern and an antipattern; - // we want it to start with regular commit convention, + // we want it to start with regular commit convention, // and it should not be breaking or fix or feat titleRegex: /$(chore|build|tests|refactor)/, titleRegexExclude: /(fix|feat|breaking|!)/, @@ -60,9 +64,51 @@ export class OwlBotTemplateChangesNode extends OwlBotTemplateChanges { bodyMatches = checkTitleOrBody(incomingPR.body, this.classRule.bodyRegex); } + const openOwlBotPRs = await getOpenPRsInRepoFromSameAuthor( + incomingPR.repoOwner, + incomingPR.repoName, + incomingPR.author, + this.octokit + ); + + let otherOwlBotPRs = true; + if ( + openOwlBotPRs.length > 0 && + incomingPR.prNumber === openOwlBotPRs[0].number + ) { + otherOwlBotPRs = false; + } + + const commitsOnPR = await listCommitsOnAPR( + incomingPR.repoOwner, + incomingPR.repoName, + incomingPR.prNumber, + this.octokit + ); + + const commitAuthors = commitsOnPR.filter( + x => x.author?.login !== this.classRule.author + ); + let otherCommitAuthors = false; + if (commitAuthors.length > 0) { + otherCommitAuthors = true; + } + reportIndividualChecks( - ['authorshipMatches', 'titleMatches', 'bodyMatches'], - [authorshipMatches, titleMatches, !bodyMatches], + [ + 'authorshipMatches', + 'titleMatches', + 'bodyMatches', + 'otherOwlBotPRs', + 'otherCommitAuthors', + ], + [ + authorshipMatches, + titleMatches, + !bodyMatches, + otherOwlBotPRs, + otherCommitAuthors, + ], incomingPR.repoOwner, incomingPR.repoName, incomingPR.prNumber diff --git a/packages/auto-approve/src/process-checks/owl-bot-api-changes.ts b/packages/auto-approve/src/process-checks/owl-bot-api-changes.ts index 267aefb1323..2cbd8f225fd 100644 --- a/packages/auto-approve/src/process-checks/owl-bot-api-changes.ts +++ b/packages/auto-approve/src/process-checks/owl-bot-api-changes.ts @@ -77,7 +77,7 @@ export class OwlBotAPIChanges extends BaseLanguageRule { ); let otherOwlBotPRs = false; - if (openOwlBotPRs > 1) { + if (openOwlBotPRs.length > 1) { otherOwlBotPRs = true; } diff --git a/packages/auto-approve/src/utils-for-pr-checking.ts b/packages/auto-approve/src/utils-for-pr-checking.ts index 28b2fe905da..6220c83b3ca 100644 --- a/packages/auto-approve/src/utils-for-pr-checking.ts +++ b/packages/auto-approve/src/utils-for-pr-checking.ts @@ -25,6 +25,7 @@ import timezone from 'dayjs/plugin/timezone'; import {logger} from 'gcf-utils'; import {Octokit} from '@octokit/rest'; import * as semver from 'semver'; +import {components} from '@octokit/openapi-types/types'; dayjs.extend(utc); dayjs.extend(timezone); @@ -655,14 +656,15 @@ export async function getOpenPRsInRepoFromSameAuthor( repo: string, author: string, octokit: Octokit -): Promise { +): Promise { const otherPRs = await octokit.paginate(octokit.rest.pulls.list, { owner, repo, state: 'open', + direction: 'asc', }); const matchingPRs = otherPRs.filter(x => x.user?.login === author); - return matchingPRs.length; + return matchingPRs; } diff --git a/packages/auto-approve/src/valid-pr-schema-v2.json b/packages/auto-approve/src/valid-pr-schema-v2.json index d3647061d3c..49aa1e9fb93 100644 --- a/packages/auto-approve/src/valid-pr-schema-v2.json +++ b/packages/auto-approve/src/valid-pr-schema-v2.json @@ -22,6 +22,7 @@ "JavaApiaryCodegen", "JavaDependency", "OwlBotTemplateChanges", + "OwlBotTemplateChangesNode", "OwlBotAPIChanges", "PHPApiaryCodegen", "PythonSampleAppDependency", diff --git a/packages/auto-approve/test/additional-validation-for-versioning.test.ts b/packages/auto-approve/test/additional-validation-for-versioning.test.ts index 6007d8e8376..e7319f723fa 100644 --- a/packages/auto-approve/test/additional-validation-for-versioning.test.ts +++ b/packages/auto-approve/test/additional-validation-for-versioning.test.ts @@ -1136,7 +1136,7 @@ describe('run additional versioning checks', () => { describe('getting all open PRs in a repo from an author', async () => { it('should return the number of PRs from anotherAuthor', async () => { const prRequest = nock('https://api.github.com') - .get('/repos/testRepoOwner/testRepoName/pulls?state=open') + .get('/repos/testRepoOwner/testRepoName/pulls?state=open&direction=asc') .reply(200, [{id: 1, user: {login: 'anotherAuthor'}}]); const prCount = await getOpenPRsInRepoFromSameAuthor( @@ -1147,12 +1147,12 @@ describe('run additional versioning checks', () => { ); prRequest.done(); - assert.deepStrictEqual(prCount, 1); + assert.deepStrictEqual(prCount.length, 1); }); it('should return the number of PRs from someOtherAuthor', async () => { const prRequest = nock('https://api.github.com') - .get('/repos/testRepoOwner/testRepoName/pulls?state=open') + .get('/repos/testRepoOwner/testRepoName/pulls?state=open&direction=asc') .reply(200, [{id: 1, user: {login: 'anotherAuthor'}}]); const prCount2 = await getOpenPRsInRepoFromSameAuthor( @@ -1163,7 +1163,7 @@ describe('run additional versioning checks', () => { ); prRequest.done(); - assert.deepStrictEqual(prCount2, 0); + assert.deepStrictEqual(prCount2.length, 0); }); it('should throw an error if prs are not received', async () => { diff --git a/packages/auto-approve/test/check-pr-V2.test.ts b/packages/auto-approve/test/check-pr-V2.test.ts index 0bb6615a653..d8bd9f0a515 100644 --- a/packages/auto-approve/test/check-pr-V2.test.ts +++ b/packages/auto-approve/test/check-pr-V2.test.ts @@ -36,7 +36,7 @@ function getPRsOnRepo( response: {id: number; user: {login: string}}[] ) { return nock('https://api.github.com') - .get(`/repos/${owner}/${repo}/pulls?state=open`) + .get(`/repos/${owner}/${repo}/pulls?state=open&direction=asc`) .reply(200, response); } @@ -109,6 +109,9 @@ describe('check pr against config', async () => { [{filename: 'requirements.txt', sha: '1234'}], 200 ), + getPRsOnRepo('GoogleCloudPlatform', 'python-docs-samples-1', [ + {id: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), listCommitsOnAPR('GoogleCloudPlatform', 'python-docs-samples-1', [ {author: {login: 'gcf-owl-bot[bot]'}}, ]), @@ -132,9 +135,6 @@ describe('check pr against config', async () => { 'ewogICAgIm5hbWUiOiAiZGxwIiwKICAgICJuYW1lX3ByZXR0eSI6ICJDbG91ZCBEYXRhIExvc3MgUHJldmVudGlvbiIsCiAgICAicHJvZHVjdF9kb2N1bWVudGF0aW9uIjogImh0dHBzOi8vY2xvdWQuZ29vZ2xlLmNvbS9kbHAvZG9jcy8iLAogICAgImNsaWVudF9kb2N1bWVudGF0aW9uIjogImh0dHBzOi8vY2xvdWQuZ29vZ2xlLmNvbS9weXRob24vZG9jcy9yZWZlcmVuY2UvZGxwL2xhdGVzdCIsCiAgICAiaXNzdWVfdHJhY2tlciI6ICIiLAogICAgInJlbGVhc2VfbGV2ZWwiOiAiZ2EiLAogICAgImxhbmd1YWdlIjogInB5dGhvbiIsCiAgICAibGlicmFyeV90eXBlIjogIkdBUElDX0FVVE8iLAogICAgInJlcG8iOiAiZ29vZ2xlYXBpcy9weXRob24tZGxwIiwKICAgICJkaXN0cmlidXRpb25fbmFtZSI6ICJnb29nbGUtY2xvdWQtZGxwIiwKICAgICJhcGlfaWQiOiAiZGxwLmdvb2dsZWFwaXMuY29tIiwKICAgICJyZXF1aXJlc19iaWxsaW5nIjogdHJ1ZSwKICAgICJkZWZhdWx0X3ZlcnNpb24iOiAidjIiLAogICAgImNvZGVvd25lcl90ZWFtIjogIiIKfQ==', } ), - getPRsOnRepo('GoogleCloudPlatform', 'python-docs-samples-1', [ - {id: 2, user: {login: 'gcf-owl-bot[bot]'}}, - ]), ]; const pr = require(resolve( diff --git a/packages/auto-approve/test/node-owlbot-template-changes.test.ts b/packages/auto-approve/test/node-owlbot-template-changes.test.ts new file mode 100644 index 00000000000..769b2247224 --- /dev/null +++ b/packages/auto-approve/test/node-owlbot-template-changes.test.ts @@ -0,0 +1,230 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import {OwlBotTemplateChangesNode} from '../src/process-checks/node/owlbot-template-changes'; +import {describe, it} from 'mocha'; +import assert from 'assert'; +import nock from 'nock'; + +const {Octokit} = require('@octokit/rest'); +nock.disableNetConnect(); + +const octokit = new Octokit({ + auth: 'mypersonalaccesstoken123', +}); + +function getPRsOnRepo( + owner: string, + repo: string, + response: {number: number; user: {login: string}}[] +) { + return nock('https://api.github.com') + .get(`/repos/${owner}/${repo}/pulls?state=open&direction=asc`) + .reply(200, response); +} + +function listCommitsOnAPR( + owner: string, + repo: string, + response: {author: {login: string}}[] +) { + return nock('https://api.github.com') + .get(`/repos/${owner}/${repo}/pulls/1/commits`) + .reply(200, response); +} + +describe('behavior of OwlBotTemplateChangesNode process', () => { + it('should return false in checkPR if incoming PR does not match classRules', async () => { + const incomingPR = { + author: 'testAuthor', + title: 'testTitle', + fileCount: 3, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + body: 'body', + }; + + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'testAuthor'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); + + it('should return false in checkPR if incoming PR includes breaking', async () => { + const incomingPR = { + author: 'gcf-owl-bot[bot]', + title: 'breaking: a new PR', + fileCount: 3, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + body: 'body', + }; + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'testAuthor'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); + + it('should return false in checkPR if incoming PR includes PiperOrigin-RevId', async () => { + const incomingPR = { + author: 'gcf-owl-bot[bot]', + title: 'chore: a new PR', + fileCount: 3, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + body: + '... signature to CreateFeatureStore, CreateEntityType, CreateFeature feat: add network and enable_private_service_connect to IndexEndpoint feat: add service_attachment to IndexPrivateEndpoints feat: add stratified_split field to training_pipeline InputDataConfig fix: remove invalid resource annotations in LineageSubgraph' + + 'Regenerate this pull request now.' + + 'PiperOrigin-RevId: 413686247' + + 'Source-Link: googleapis/googleapis@244a89d' + + 'Source-Link: googleapis/googleapis-gen@c485e44' + + 'Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYzQ4NWU0NGExYjJmZWY1MTZlOWJjYTM2NTE0ZDUwY2ViZDVlYTUxZiJ9', + }; + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'testAuthor'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); + + it('should return false in checkPR if incoming PR is not the first in ascending order', async () => { + const incomingPR = { + author: 'gcf-owl-bot[bot]', + title: 'chore: a new PR', + fileCount: 3, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + }; + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 2, user: {login: 'gcf-owl-bot[bot]'}}, + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'testAuthor'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); + + it('should return false in checkPR if incoming PR has commits from other authors', async () => { + const incomingPR = { + author: 'gcf-owl-bot[bot]', + title: 'chore: a new PR', + fileCount: 3, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + }; + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'testAuthor'}}, + {author: {login: 'gcf-owl-bot[bot]'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); + + it('should return true in checkPR if incoming PR does match classRules, is the first in the PRs', async () => { + const incomingPR = { + author: 'gcf-owl-bot[bot]', + title: 'chore: a fine title', + fileCount: 2, + changedFiles: [{filename: 'hello', sha: '2345'}], + repoName: 'testRepoName', + repoOwner: 'testRepoOwner', + prNumber: 1, + body: 'body', + }; + const owlBotTemplateChanges = new OwlBotTemplateChangesNode(octokit); + + const scopes = [ + getPRsOnRepo('testRepoOwner', 'testRepoName', [ + {number: 1, user: {login: 'gcf-owl-bot[bot]'}}, + {number: 2, user: {login: 'gcf-owl-bot[bot]'}}, + ]), + listCommitsOnAPR('testRepoOwner', 'testRepoName', [ + {author: {login: 'gcf-owl-bot[bot]'}}, + {author: {login: 'gcf-owl-bot[bot]'}}, + ]), + ]; + + assert.deepStrictEqual( + await owlBotTemplateChanges.checkPR(incomingPR), + false + ); + scopes.forEach(scope => scope.done()); + }); +}); diff --git a/packages/auto-approve/test/owlbot-api-changes.test.ts b/packages/auto-approve/test/owlbot-api-changes.test.ts index 8803d760b7b..fb121e9fd4b 100644 --- a/packages/auto-approve/test/owlbot-api-changes.test.ts +++ b/packages/auto-approve/test/owlbot-api-changes.test.ts @@ -30,7 +30,7 @@ function getPRsOnRepo( response: {id: number; user: {login: string}}[] ) { return nock('https://api.github.com') - .get(`/repos/${owner}/${repo}/pulls?state=open`) + .get(`/repos/${owner}/${repo}/pulls?state=open&direction=asc`) .reply(200, response); }