diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index eb4fdbc5e7f..f5c6b4c6705 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,9 +1,9 @@ ### Submission Review Guidelines: -- I have followed the [WooCommerce Contributing Guidelines](https://github.com/woocommerce/woocommerce/blob/trunk/.github/CONTRIBUTING.md) and the [WordPress Coding Standards](https://make.wordpress.org/core/handbook/best-practices/coding-standards/). -- I have checked to ensure there aren't other open [Pull Requests](https://github.com/woocommerce/woocommerce/pulls) for the same update/change. -- I have reviewed my code for [security best practices](https://developer.wordpress.org/apis/security/). -- Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate. +- I have followed the [WooCommerce Contributing Guidelines](https://github.com/woocommerce/woocommerce/blob/trunk/.github/CONTRIBUTING.md) and the [WordPress Coding Standards](https://make.wordpress.org/core/handbook/best-practices/coding-standards/). +- I have checked to ensure there aren't other open [Pull Requests](https://github.com/woocommerce/woocommerce/pulls) for the same update/change. +- I have reviewed my code for [security best practices](https://developer.wordpress.org/apis/security/). +- Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate. @@ -28,3 +28,37 @@ Using the [WooCommerce Testing Instructions Guide](https://github.com/woocommerc 3. + +### Changelog entry + + + +- [ ] Automatically create a changelog entry from the details below. + +
+ +#### Significance + + + +- [ ] Patch +- [ ] Minor +- [ ] Major + +#### Type + + + +- [ ] Fix - Fixes an existing bug +- [ ] Add - Adds functionality +- [ ] Update - Update existing functionality +- [ ] Dev - Development related task +- [ ] Tweak - A minor adjustment to the codebase +- [ ] Performance - Address performance issues +- [ ] Enhancement + +#### Message + +#### Comment + +
diff --git a/.github/workflows/changelog-auto-add.yml b/.github/workflows/changelog-auto-add.yml index 1afd60511b3..967432f1c48 100644 --- a/.github/workflows/changelog-auto-add.yml +++ b/.github/workflows/changelog-auto-add.yml @@ -1,10 +1,46 @@ name: 'Changelog Auto Add' -on: workflow_dispatch +on: + pull_request: + types: [opened, synchronize, reopened, edited] + workflow_dispatch: + inputs: + prNumber: + description: Pull request number + required: true jobs: - hello-world: - name: 'Hello World' + add-changelog: + name: 'Add changelog to PR' + if: ${{ github.event.pull_request.user.login != 'github-actions[bot]' }} runs-on: ubuntu-20.04 + permissions: + contents: write + issues: write + pull-requests: write steps: - - name: Hello - run: echo "Hello World" + - name: Checkout code + uses: actions/checkout@v3 + + - name: Setup PNPM + uses: pnpm/action-setup@c3b53f6a16e57305370b4ae5a540c2077a1d50dd + with: + version: '8.3.1' + + - name: Setup Node + uses: actions/setup-node@64ed1c7eab4cce3362f8c340dee64e5eaeef8f7c + with: + node-version-file: .nvmrc + cache: pnpm + registry-url: 'https://registry.npmjs.org' + + - name: Install prerequisites + run: | + pnpm install --filter monorepo-utils --ignore-scripts + # ignore scripts speeds up setup signficantly, but we still need to build monorepo utils + pnpm build + working-directory: tools/monorepo-utils + + - name: Add changelog + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: pnpm utils changefile ${{github.event.number || inputs.prNumber}} -o ${{ github.repository_owner }} diff --git a/bin/pre-push.sh b/bin/pre-push.sh index bccdc2587e7..563b74c3f48 100755 --- a/bin/pre-push.sh +++ b/bin/pre-push.sh @@ -31,9 +31,3 @@ if [ $? -ne 0 ]; then exit 1 fi -# Ensure both branches are tracked or check-changelogger-use will fail. Note we pass hooksPath -# to avoid running the pre-commit hook. -git -c core.hooksPath=/dev/null checkout $PROTECTED_BRANCH --quiet -git -c core.hooksPath=/dev/null checkout $CURRENT_BRANCH --quiet - -php tools/monorepo/check-changelogger-use.php $PROTECTED_BRANCH $CURRENT_BRANCH diff --git a/tools/monorepo-utils/src/changefile/index.ts b/tools/monorepo-utils/src/changefile/index.ts index f9d71fff26b..e61507e18af 100644 --- a/tools/monorepo-utils/src/changefile/index.ts +++ b/tools/monorepo-utils/src/changefile/index.ts @@ -17,6 +17,7 @@ import { getPullRequestData, shouldAutomateChangelog, getChangelogDetails, + getChangelogDetailsError, } from './lib/github'; import { getAllProjectPaths, @@ -66,8 +67,13 @@ const program = new Command( 'changefile' ) process.exit( 0 ); } - const { significance, type, message, comment } = - getChangelogDetails( prBody ); + const details = getChangelogDetails( prBody ); + const { significance, type, message, comment } = details; + const changelogDetailsError = getChangelogDetailsError( details ); + + if ( changelogDetailsError ) { + Logger.error( changelogDetailsError ); + } Logger.startTask( `Making a temporary clone of '${ headOwner }/${ name }'` @@ -76,7 +82,7 @@ const program = new Command( 'changefile' ) ? devRepoPath : await cloneAuthenticatedRepo( { owner: headOwner, name }, - true + false ); Logger.endTask(); @@ -85,15 +91,6 @@ const program = new Command( 'changefile' ) `Temporary clone of '${ headOwner }/${ name }' created at ${ tmpRepoPath }` ); - // When a devRepoPath is provided, assume that the dependencies are already installed. - if ( ! devRepoPath ) { - Logger.notice( `Installing dependencies in ${ tmpRepoPath }` ); - execSync( 'pnpm install', { - cwd: tmpRepoPath, - stdio: 'inherit', - } ); - } - Logger.notice( `Checking out remote branch ${ branch }` ); await checkoutRemoteBranch( tmpRepoPath, branch ); @@ -106,13 +103,17 @@ const program = new Command( 'changefile' ) tmpRepoPath, base, head, - fileName + fileName, + owner, + name ); try { const allProjectPaths = await getAllProjectPaths( tmpRepoPath ); - // Remove any already existing changelog files in case a change is reverted and the entry is no longer needed. + Logger.notice( + 'Removing existing changelog files in case a change is reverted and the entry is no longer needed' + ); allProjectPaths.forEach( ( projectPath ) => { const path = nodePath.join( tmpRepoPath, @@ -133,6 +134,22 @@ const program = new Command( 'changefile' ) } } ); + if ( touchedProjectsRequiringChangelog.length === 0 ) { + Logger.notice( 'No projects require a changelog' ); + process.exit( 0 ); + } + + // When a devRepoPath is provided, assume that the dependencies are already installed. + if ( ! devRepoPath ) { + Logger.notice( + `Installing dependencies in ${ tmpRepoPath }` + ); + execSync( 'pnpm install', { + cwd: tmpRepoPath, + stdio: 'inherit', + } ); + } + touchedProjectsRequiringChangelog.forEach( ( project ) => { Logger.notice( `Running changelog command for ${ project }` @@ -150,10 +167,11 @@ const program = new Command( 'changefile' ) Logger.error( e ); } + const touchedProjectsString = + touchedProjectsRequiringChangelog.join( ', ' ); + Logger.notice( - `Changelogs created for ${ touchedProjectsRequiringChangelog.join( - ', ' - ) }` + `Changelogs created for ${ touchedProjectsString }` ); const git = simpleGit( { @@ -187,7 +205,9 @@ const program = new Command( 'changefile' ) Logger.notice( `Adding and committing changes` ); await git.add( '.' ); - await git.commit( 'Adding changelog from automation.' ); + await git.commit( + `Add changefile(s) from automation for the following project(s): ${ touchedProjectsString }` + ); await git.push( 'origin', branch ); Logger.notice( `Pushed changes to ${ branch }` ); } diff --git a/tools/monorepo-utils/src/changefile/lib/__tests__/github.ts b/tools/monorepo-utils/src/changefile/lib/__tests__/github.ts index ead6aaf408f..c5b171de348 100644 --- a/tools/monorepo-utils/src/changefile/lib/__tests__/github.ts +++ b/tools/monorepo-utils/src/changefile/lib/__tests__/github.ts @@ -10,6 +10,7 @@ import { getChangelogSignificance, getChangelogType, getChangelogDetails, + getChangelogDetailsError, } from '../github'; import { Logger } from '../../../core/logger'; @@ -48,11 +49,11 @@ describe( 'getChangelogSignificance', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -87,11 +88,11 @@ describe( 'getChangelogSignificance', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -129,11 +130,11 @@ describe( 'getChangelogSignificance', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -173,11 +174,11 @@ describe( 'getChangelogType', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -212,11 +213,11 @@ describe( 'getChangelogType', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -254,11 +255,11 @@ describe( 'getChangelogType', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -298,11 +299,11 @@ describe( 'getChangelogDetails', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + '\r\n' + ''; @@ -314,7 +315,7 @@ describe( 'getChangelogDetails', () => { expect( details.comment ).toEqual( '' ); } ); - it( 'should error if a comment and message are added', () => { + it( 'should provide comment and message when both are added', () => { const body = '### Changelog entry\r\n' + '\r\n' + @@ -340,24 +341,22 @@ describe( 'getChangelogDetails', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + 'This is a very useful fix.\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + 'This is a very useful comment.\r\n' + '\r\n' + ''; const details = getChangelogDetails( body ); - expect( details ).toBeUndefined(); - expect( Logger.error ).toHaveBeenCalledWith( - 'Both a message and comment were found. Only one can be entered' - ); + expect( details.message ).toEqual( 'This is a very useful fix.' ); + expect( details.comment ).toEqual( 'This is a very useful comment.' ); } ); - it( 'should error if a comment is entered with a significance other than patch', () => { + it( 'should return a comment even when it is entered with a significance other than patch', () => { const body = '### Changelog entry\r\n' + '\r\n' + @@ -383,19 +382,73 @@ describe( 'getChangelogDetails', () => { '- [ ] Performance - Address performance issues\r\n' + '- [ ] Enhancement\r\n' + '\r\n' + - '#### Message\r\n' + + '#### Message ' + '\r\n' + '\r\n' + - '#### Comment\r\n' + + '#### Comment ' + `\r\n` + 'This is a very useful comment.\r\n' + '\r\n' + ''; const details = getChangelogDetails( body ); - expect( details ).toBeUndefined(); - expect( Logger.error ).toHaveBeenCalledWith( + expect( details.comment ).toEqual( 'This is a very useful comment.' ); + expect( details.significance ).toEqual( 'minor' ); + } ); +} ); + +describe( 'getChangelogDetailsError', () => { + it( 'should return an error when both a message and comment provided', () => { + const error = getChangelogDetailsError( { + message: 'message', + comment: 'comment', + type: 'fix', + significance: 'minor', + } ); + expect( error ).toEqual( + 'Both a message and comment were found. Only one can be entered' + ); + } ); + + it( 'should return an error when a comment is provided with a significance other than patch', () => { + const error = getChangelogDetailsError( { + message: '', + comment: 'comment', + type: 'fix', + significance: 'minor', + } ); + expect( error ).toEqual( 'Only patch changes can have a comment. Please change the significance to patch or remove the comment' ); } ); + + it( 'should return an error when no significance found', () => { + const error = getChangelogDetailsError( { + message: 'message', + comment: '', + type: 'fix', + significance: '', + } ); + expect( error ).toEqual( 'No changelog significance found' ); + } ); + + it( 'should return an error when no type found', () => { + const error = getChangelogDetailsError( { + message: 'message', + comment: '', + type: '', + significance: 'minor', + } ); + expect( error ).toEqual( 'No changelog type found' ); + } ); + + it( 'should return an error when neither a comment or message is provided', () => { + const error = getChangelogDetailsError( { + message: '', + comment: '', + type: 'fix', + significance: 'minor', + } ); + expect( error ).toEqual( 'No changelog message or comment found' ); + } ); } ); diff --git a/tools/monorepo-utils/src/changefile/lib/github.ts b/tools/monorepo-utils/src/changefile/lib/github.ts index f418348bf48..5757393f33b 100644 --- a/tools/monorepo-utils/src/changefile/lib/github.ts +++ b/tools/monorepo-utils/src/changefile/lib/github.ts @@ -113,7 +113,7 @@ export const getChangelogType = ( body: string ) => { * @return {void|string} changelog message. */ export const getChangelogMessage = ( body: string ) => { - const messageRegex = /#### Message\r\n()?(.*)#### Comment/gms; + const messageRegex = /#### Message ?()?(.*)#### Comment/gms; const match = messageRegex.exec( body ); if ( ! match ) { @@ -130,38 +130,62 @@ export const getChangelogMessage = ( body: string ) => { * @return {void|string} changelog comment. */ export const getChangelogComment = ( body: string ) => { - const commentRegex = /#### Comment\r\n()?(.*)<\/details>/gms; + const commentRegex = /#### Comment ?()?(.*)<\/details>/gms; const match = commentRegex.exec( body ); return match ? match[ 3 ].trim() : ''; }; +/** + * Get the changelog details from a pull request description. + * + * @param {string} body Pull request description + * @return {Object} Changelog details + */ export const getChangelogDetails = ( body: string ) => { - const message = getChangelogMessage( body ); - const comment = getChangelogComment( body ); - - if ( comment && message ) { - Logger.error( - 'Both a message and comment were found. Only one can be entered' - ); - // Logger.error has a process.exit( 1 ) call, this return is purely for testing purposes. - return; - } - - const significance = getChangelogSignificance( body ); - - if ( comment && significance !== 'patch' ) { - Logger.error( - 'Only patch changes can have a comment. Please change the significance to patch or remove the comment' - ); - // Logger.error has a process.exit( 1 ) call, this return is purely for testing purposes. - return; - } - return { - significance, + significance: getChangelogSignificance( body ), type: getChangelogType( body ), - message, - comment, + message: getChangelogMessage( body ), + comment: getChangelogComment( body ), }; }; + +/** + * Determine if a pull request description contains changelog input errors. + * + * @param {Object} details changelog details. + * @param {string} details.significance changelog significance. + * @param {string} details.type changelog type. + * @param {string} details.message changelog message. + * @param {string} details.comment changelog comment. + * @return {string|null} error message, or null if none found + */ +export const getChangelogDetailsError = ( { + significance, + type, + message, + comment, +}: { + significance?: string; + type?: string; + message?: string; + comment?: string; +} ) => { + if ( comment && message ) { + return 'Both a message and comment were found. Only one can be entered'; + } + if ( comment && significance !== 'patch' ) { + return 'Only patch changes can have a comment. Please change the significance to patch or remove the comment'; + } + if ( ! significance ) { + return 'No changelog significance found'; + } + if ( ! type ) { + return 'No changelog type found'; + } + if ( ! comment && ! message ) { + return 'No changelog message or comment found'; + } + return null; +}; diff --git a/tools/monorepo-utils/src/changefile/lib/projects.ts b/tools/monorepo-utils/src/changefile/lib/projects.ts index b4500afc4da..cf194060151 100644 --- a/tools/monorepo-utils/src/changefile/lib/projects.ts +++ b/tools/monorepo-utils/src/changefile/lib/projects.ts @@ -7,6 +7,10 @@ import path from 'path'; import { glob } from 'glob'; import simpleGit from 'simple-git'; +/** + * Internal dependencies + */ +import { getAuthenticatedRemote } from '../../core/git'; /** * Get all projects listed in the workspace yaml file. * @@ -32,7 +36,8 @@ export const getAllProjectsPathsFromWorkspace = async ( return project; } ) ); - return globbedProjects.flat(); + const r = globbedProjects.flat(); + return r; }; /** @@ -72,13 +77,17 @@ export const getChangeloggerProjectPaths = async ( * @param {string} base base hash * @param {string} head head hash * @param {string} fileName changelog file name + * @param {string} baseOwner PR base owner + * @param {string} baseName PR base name * @return {Array} List of files changed in a PR. */ export const getTouchedFilePaths = async ( tmpRepoPath: string, base: string, head: string, - fileName: string + fileName: string, + baseOwner: string, + baseName: string ) => { const git = simpleGit( { baseDir: tmpRepoPath, @@ -86,13 +95,12 @@ export const getTouchedFilePaths = async ( } ); // make sure base sha is available. - await git.raw( [ - 'remote', - 'add', - 'woocommerce', - 'git@github.com:woocommerce/woocommerce.git', - ] ); - await git.raw( [ 'fetch', 'woocommerce', base ] ); + await git.addRemote( + baseOwner, + getAuthenticatedRemote( { owner: baseOwner, name: baseName } ) + ); + await git.fetch( baseOwner, base ); + const diff = await git.raw( [ 'diff', '--name-only', @@ -101,7 +109,7 @@ export const getTouchedFilePaths = async ( return ( diff .split( '\n' ) - .filter( ( item ) => item.trim() ) + .map( ( item ) => item.trim() ) // Don't count changelogs themselves as touched files. .filter( ( item ) => ! item.includes( `/changelog/${ fileName }` ) ) ); @@ -165,13 +173,17 @@ export const getAllProjectPaths = async ( tmpRepoPath: string ) => { * @param {string} base base hash * @param {string} head head hash * @param {string} fileName changelog file name + * @param {string} baseOwner PR base owner + * @param {string} baseName PR base name * @return {Array} List of projects that have Jetpack changelogger enabled and have files changed in a PR. */ export const getTouchedProjectsRequiringChangelog = async ( tmpRepoPath: string, base: string, head: string, - fileName: string + fileName: string, + baseOwner: string, + baseName: string ) => { const allProjectPaths = await getAllProjectPaths( tmpRepoPath ); const changeloggerProjectsPaths = await getChangeloggerProjectPaths( @@ -182,7 +194,9 @@ export const getTouchedProjectsRequiringChangelog = async ( tmpRepoPath, base, head, - fileName + fileName, + baseOwner, + baseName ); return getTouchedChangeloggerProjectsPathsMappedToProjects( diff --git a/tools/monorepo-utils/src/core/git.ts b/tools/monorepo-utils/src/core/git.ts index 19a46e64eff..43a4bdfa363 100644 --- a/tools/monorepo-utils/src/core/git.ts +++ b/tools/monorepo-utils/src/core/git.ts @@ -115,6 +115,24 @@ export const cloneRepoShallow = async ( repoPath: string ) => { return await cloneRepo( repoPath, { '--depth': 1 } ); }; +/** + * Add a remote using the authenticated token `GITHUB_TOKEN` + * + * @param {Object} options CLI options + * @param {string} options.owner repo owner + * @param {string} options.name repo name + * @return {string} remote + */ +export const getAuthenticatedRemote = ( options: { + owner: string; + name: string; +} ) => { + const { owner, name } = options; + const source = `github.com/${ owner }/${ name }`; + const token = getEnvVar( 'GITHUB_TOKEN', true ); + return `https://${ owner }:${ token }@${ source }`; +}; + /** * Clone a repo using the authenticated token `GITHUB_TOKEN`. This allows the script to push branches to origin. * @@ -128,10 +146,7 @@ export const cloneAuthenticatedRepo = async ( options: { owner: string; name: string }, isShallow = true ): Promise< string > => { - const { owner, name } = options; - const source = `github.com/${ owner }/${ name }`; - const token = getEnvVar( 'GITHUB_TOKEN' ); - const remote = `https://${ owner }:${ token }@${ source }`; + const remote = getAuthenticatedRemote( options ); return isShallow ? await cloneRepoShallow( remote ) diff --git a/tools/monorepo-utils/src/core/github/repo.ts b/tools/monorepo-utils/src/core/github/repo.ts index 4ebaee35bd7..2f9d27f485c 100644 --- a/tools/monorepo-utils/src/core/github/repo.ts +++ b/tools/monorepo-utils/src/core/github/repo.ts @@ -212,10 +212,6 @@ export const isCommunityPullRequest = ( owner: string, name: string ) => { - return ( - pullRequestData.author_association === 'CONTRIBUTOR' || - // It's possible a MEMBER can open a PR from their own fork. - ( pullRequestData.author_association === 'MEMBER' && - pullRequestData.head.repo.full_name !== `${ owner }/${ name }` ) - ); + // We can't use author_association here because it can be changed by PR authors. Instead check PR source. + return pullRequestData.head.repo.full_name !== `${ owner }/${ name }`; };