From b43e601b83a7b65b674665575bf960c9e4409687 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Tue, 21 Jul 2020 10:32:36 -0500 Subject: [PATCH] chore: add Trop annotations to release notes. (#24644) * chore: add Trop annotations to release notes. Trop annotations are in the form of "(Also in 7.3, 8, 9)" with links to the sibling branches. --- script/release/notes/index.js | 12 +- script/release/notes/notes.js | 264 +++++++++++++------------------- spec-main/release-notes-spec.ts | 26 ++-- 3 files changed, 125 insertions(+), 177 deletions(-) diff --git a/script/release/notes/index.js b/script/release/notes/index.js index 7f8f6b2e15fa..40e4cb4ab017 100755 --- a/script/release/notes/index.js +++ b/script/release/notes/index.js @@ -120,7 +120,7 @@ const getPreviousPoint = async (point) => { } }; -async function getReleaseNotes (range, newVersion, explicitLinks) { +async function getReleaseNotes (range, newVersion) { const rangeList = range.split('..') || ['HEAD']; const to = rangeList.pop(); const from = rangeList.pop() || (await getPreviousPoint(to)); @@ -129,10 +129,9 @@ async function getReleaseNotes (range, newVersion, explicitLinks) { newVersion = to; } - console.log(`Generating release notes between ${from} and ${to} for version ${newVersion}`); const notes = await notesGenerator.get(from, to, newVersion); const ret = { - text: notesGenerator.render(notes, explicitLinks) + text: notesGenerator.render(notes) }; if (notes.unknown.length) { @@ -144,7 +143,7 @@ async function getReleaseNotes (range, newVersion, explicitLinks) { async function main () { const opts = minimist(process.argv.slice(2), { - boolean: ['explicit-links', 'help'], + boolean: ['help'], string: ['version'] }); opts.range = opts._.shift(); @@ -153,14 +152,13 @@ async function main () { console.log(` easy usage: ${name} version -full usage: ${name} [begin..]end [--version version] [--explicit-links] +full usage: ${name} [begin..]end [--version version] * 'begin' and 'end' are two git references -- tags, branches, etc -- from which the release notes are generated. * if omitted, 'begin' defaults to the previous tag in end's branch. * if omitted, 'version' defaults to 'end'. Specifying a version is useful if you're making notes on a new version that isn't tagged yet. - * 'explicit-links' makes every note's issue, commit, or pull an MD link For example, these invocations are equivalent: ${process.argv[1]} v4.0.1 @@ -169,7 +167,7 @@ For example, these invocations are equivalent: return 0; } - const notes = await getReleaseNotes(opts.range, opts.version, opts['explicit-links']); + const notes = await getReleaseNotes(opts.range, opts.version); console.log(notes.text); if (notes.warning) { throw new Error(notes.warning); diff --git a/script/release/notes/notes.js b/script/release/notes/notes.js index 6008c3d4f25f..0f488448e4e1 100644 --- a/script/release/notes/notes.js +++ b/script/release/notes/notes.js @@ -2,24 +2,22 @@ 'use strict'; -const childProcess = require('child_process'); const fs = require('fs'); -const os = require('os'); const path = require('path'); const { GitProcess } = require('dugite'); const octokit = require('@octokit/rest')({ auth: process.env.ELECTRON_GITHUB_TOKEN }); -const semver = require('semver'); -const { ELECTRON_VERSION, SRC_DIR } = require('../../lib/utils'); +const { SRC_DIR } = require('../../lib/utils'); const MAX_FAIL_COUNT = 3; const CHECK_INTERVAL = 5000; +const TROP_LOGIN = 'trop[bot]'; + const NO_NOTES = 'No notes'; -const FOLLOW_REPOS = ['electron/electron', 'electron/node']; const docTypes = new Set(['doc', 'docs']); const featTypes = new Set(['feat', 'feature']); @@ -57,6 +55,11 @@ class Commit { this.isBreakingChange = false; this.note = null; // string + + // A set of branches to which this change has been merged. + // '8-x-y' => GHKey { owner: 'electron', repo: 'electron', number: 23714 } + this.trops = new Map(); // Map + this.prKeys = new Set(); // GHKey this.revertHash = null; // string this.semanticType = null; // string @@ -107,7 +110,7 @@ const getNoteFromClerk = async (ghKey) => { if (comment.body.startsWith(PERSIST_LEAD)) { return comment.body .slice(PERSIST_LEAD.length).trim() // remove PERSIST_LEAD - .split('\r?\n') // break into lines + .split(/\r?\n/) // split into lines .map(line => line.trim()) .filter(line => line.startsWith(QUOTE_LEAD)) // notes are quoted .map(line => line.slice(QUOTE_LEAD.length)) // unquote the lines @@ -117,18 +120,6 @@ const getNoteFromClerk = async (ghKey) => { } }; -// copied from https://github.com/electron/clerk/blob/master/src/index.ts#L4-L13 -const OMIT_FROM_RELEASE_NOTES_KEYS = [ - 'no-notes', - 'no notes', - 'no_notes', - 'none', - 'no', - 'nothing', - 'empty', - 'blank' -]; - /** * Looks for our project's conventions in the commit message: * @@ -206,7 +197,9 @@ const parsePullText = (pull, commit) => parseCommitMessage(`${pull.data.title}\n const getLocalCommitHashes = async (dir, ref) => { const args = ['log', '--format=%H', ref]; - return (await runGit(dir, args)).split(/[\r\n]+/).map(hash => hash.trim()); + return (await runGit(dir, args)) + .split(/\r?\n/) // split into lines + .map(hash => hash.trim()); }; // return an array of Commits @@ -216,7 +209,9 @@ const getLocalCommits = async (module, point1, point2) => { const fieldSep = ','; const format = ['%H', '%s'].join(fieldSep); const args = ['log', '--cherry-pick', '--right-only', '--first-parent', `--format=${format}`, `${point1}..${point2}`]; - const logs = (await runGit(dir, args)).split(/[\r\n]+/).map(field => field.trim()); + const logs = (await runGit(dir, args)) + .split(/\r?\n/) // split into lines + .map(field => field.trim()); const commits = []; for (const log of logs) { @@ -332,92 +327,50 @@ const addRepoToPool = async (pool, repo, from, to) => { } }; -/*** -**** Other Repos -***/ +// returns Map +// where the key is a branch name (e.g. '7-1-x' or '8-x-y') +// and the value is a GHKey to the PR +async function getBranchesOfCommit (commit, pool) { + const branches = new Map(); -// other repos - gn + for (const prKey of commit.prKeys.values()) { + const pull = pool.pulls[prKey.number]; + const mergedBranches = new Set((pull?.data?.labels || []) + .map(label => (label?.name ?? '').match(/merged\/([0-9]+-[x0-9]-[xy0-9])/)) + .filter(match => match) + .map(match => match[1]) + ); -const getDepsVariable = async (ref, key) => { - // get a copy of that reference point's DEPS file - const deps = await runGit(ELECTRON_VERSION, ['show', `${ref}:DEPS`]); - const filename = path.resolve(os.tmpdir(), 'DEPS'); - fs.writeFileSync(filename, deps); + if (mergedBranches.size > 0) { + const isTropComment = (comment) => (comment?.user?.login ?? '') === TROP_LOGIN; - // query the DEPS file - const response = childProcess.spawnSync( - 'gclient', - ['getdep', '--deps-file', filename, '--var', key], - { encoding: 'utf8' } - ); + const ghKey = GHKey.NewFromPull(pull.data); + const backportRegex = /backported this PR to "(.*)",\s+please check out #(\d+)/; + const getBranchNameAndPullKey = (comment) => { + const match = (comment?.body ?? '').match(backportRegex); + return match ? [match[1], new GHKey(ghKey.owner, ghKey.repo, parseInt(match[2]))] : null; + }; - // cleanup - fs.unlinkSync(filename); - return response.stdout.trim(); -}; - -const getDependencyCommitsGN = async (pool, fromRef, toRef) => { - const repos = [{ // just node - owner: 'electron', - repo: 'node', - dir: path.resolve(SRC_DIR, 'third_party', 'electron_node'), - deps_variable_name: 'node_version' - }]; - - for (const repo of repos) { - // the 'DEPS' file holds the dependency reference point - const key = repo.deps_variable_name; - const from = await getDepsVariable(fromRef, key); - const to = await getDepsVariable(toRef, key); - await addRepoToPool(pool, repo, from, to); - } -}; - -// Changes are interesting if they make a change relative to a previous -// release in the same series. For example if you fix a Y.0.0 bug, that -// should be included in the Y.0.1 notes even if it's also tropped back -// to X.0.1. -// -// The phrase 'previous release' is important: if this is the first -// prerelease or first stable release in a series, we omit previous -// branches' changes. Otherwise we will have an overwhelmingly long -// list of mostly-irrelevant changes. -const shouldIncludeMultibranchChanges = (version) => { - let show = true; - - if (semver.valid(version)) { - const prerelease = semver.prerelease(version); - show = prerelease - ? parseInt(prerelease.pop()) > 1 - : semver.patch(version) > 0; + ((await getComments(ghKey))?.data ?? []) + .filter(isTropComment) + .map(getBranchNameAndPullKey) + .filter(pair => pair) + .filter(([branch]) => mergedBranches.has(branch)) + .forEach(([branch, key]) => branches.set(branch, key)); + } } - return show; -}; - -function getOldestMajorBranchOfPull (pull) { - return pull.data.labels - .map(label => label.name.match(/merged\/(\d+)-(\d+)-x/) || label.name.match(/merged\/(\d+)-x-y/)) - .filter(label => !!label) - .map(label => parseInt(label[1])) - .filter(major => !!major) - .sort() - .shift(); + return branches; } -function getOldestMajorBranchOfCommit (commit, pool) { - return [...commit.prKeys.values()] - .map(prKey => pool.pulls[prKey.number]) - .filter(pull => !!pull) - .map(pull => getOldestMajorBranchOfPull(pull)) - .filter(major => !!major) - .sort() - .shift(); -} - -function commitExistsBeforeMajor (commit, pool, major) { - const firstAppearance = getOldestMajorBranchOfCommit(commit, pool); - return firstAppearance && (firstAppearance < major); +// @return the shorthand name of the branch that `ref` is on. +// e.g. a ref of '10.0.0-beta.1' will return '10-x-y' +async function getBranchNameOfRef (ref, dir) { + return (await runGit(dir, ['branch', '--all', '--contains', ref, '--sort', 'version:refname'])) + .split(/\r?\n/) // split into lines + .shift() // we sorted by refname and want the first result + .match(/(?:.*\/)?(.*)/)[1] // 'remote/origins/10-x-y' -> '10-x-y' + .trim(); } /*** @@ -431,21 +384,15 @@ const getNotes = async (fromRef, toRef, newVersion) => { } const pool = new Pool(); + const electronDir = path.resolve(SRC_DIR, 'electron'); + const toBranch = await getBranchNameOfRef(toRef, electronDir); + + console.log(`Generating release notes between ${fromRef} and ${toRef} for version ${newVersion} in branch ${toBranch}`); // get the electron/electron commits - const electron = { owner: 'electron', repo: 'electron', dir: path.resolve(SRC_DIR, 'electron') }; + const electron = { owner: 'electron', repo: 'electron', dir: electronDir }; await addRepoToPool(pool, electron, fromRef, toRef); - // Don't include submodules if comparing across major versions; - // there's just too much churn otherwise. - const includeDeps = semver.valid(fromRef) && - semver.valid(toRef) && - semver.major(fromRef) === semver.major(toRef); - - if (includeDeps) { - await getDependencyCommitsGN(pool, fromRef, toRef); - } - // remove any old commits pool.commits = pool.commits.filter(commit => !pool.processedHashes.has(commit.hash)); @@ -482,9 +429,8 @@ const getNotes = async (fromRef, toRef, newVersion) => { .filter(commit => commit.note && (commit.note !== NO_NOTES)) .filter(commit => !((commit.note || commit.subject).match(/^[Bb]ump v\d+\.\d+\.\d+/))); - if (!shouldIncludeMultibranchChanges(newVersion)) { - const { major } = semver.parse(newVersion); - pool.commits = pool.commits.filter(commit => !commitExistsBeforeMajor(commit, pool, major)); + for (const commit of pool.commits) { + commit.trops = await getBranchesOfCommit(commit, pool); } pool.commits = removeSupercededStackUpdates(pool.commits); @@ -496,7 +442,8 @@ const getNotes = async (fromRef, toRef, newVersion) => { fix: [], other: [], unknown: [], - name: newVersion + name: newVersion, + toBranch }; pool.commits.forEach(commit => { @@ -545,31 +492,42 @@ const removeSupercededStackUpdates = (commits) => { **** Render ***/ -const renderLink = (commit, explicitLinks) => { - let link; - const { owner, repo } = commit; - const keyIt = commit.prKeys.values().next(); - if (keyIt.done) /* no PRs */ { - const { hash } = commit; - const url = `https://github.com/${owner}/${repo}/commit/${hash}`; - const text = owner === 'electron' && repo === 'electron' - ? `${hash.slice(0, 8)}` - : `${owner}/${repo}@${hash.slice(0, 8)}`; - link = explicitLinks ? `[${text}](${url})` : text; - } else { - const { number } = keyIt.value; - const url = `https://github.com/${owner}/${repo}/pull/${number}`; - const text = owner === 'electron' && repo === 'electron' - ? `#${number}` - : `${owner}/${repo}#${number}`; - link = explicitLinks ? `[${text}](${url})` : text; - } - return link; -}; +// @return the pull request's GitHub URL +const buildPullURL = ghKey => `https://github.com/${ghKey.owner}/${ghKey.repo}/pull/${ghKey.number}`; -const renderCommit = (commit, explicitLinks) => { - // clean up the note - let note = commit.note || commit.subject; +const renderPull = ghKey => `[#${ghKey.number}](${buildPullURL(ghKey)})`; + +// @return the commit's GitHub URL +const buildCommitURL = commit => `https://github.com/${commit.owner}/${commit.repo}/commit/${commit.hash}`; + +const renderCommit = commit => `[${commit.hash.slice(0, 8)}](${buildCommitURL(commit)})`; + +// @return a markdown link to the PR if available; otherwise, the git commit +function renderLink (commit) { + const maybePull = commit.prKeys.values().next(); + return maybePull.value ? renderPull(maybePull.value) : renderCommit(commit); +} + +// @return a terser branch name, +// e.g. '7-2-x' -> '7.2' and '8-x-y' -> '8' +const renderBranchName = name => name.replace(/-[a-zA-Z]/g, '').replace('-', '.'); + +const renderTrop = (branch, ghKey) => `[${renderBranchName(branch)}](${buildPullURL(ghKey)})`; + +// @return markdown-formatted links to other branches' trops, +// e.g. "(Also in 7.2, 8, 9)" +function renderTrops (commit, excludeBranch) { + const body = [...commit.trops.entries()] + .filter(([branch]) => branch !== excludeBranch) + .sort(([branchA], [branchB]) => parseInt(branchA) - parseInt(branchB)) // sort by semver major + .map(([branch, key]) => renderTrop(branch, key)) + .join(', '); + return body ? `(Also in ${body})` : body; +} + +// @return a slightly cleaned-up human-readable change description +function renderDescription (commit) { + let note = commit.note || commit.subject || ''; note = note.trim(); if (note.length !== 0) { note = note[0].toUpperCase() + note.substr(1); @@ -606,30 +564,24 @@ const renderCommit = (commit, explicitLinks) => { } } - const link = renderLink(commit, explicitLinks); + return note; +} - return { note, link }; -}; +// @return markdown-formatted release note line item, +// e.g. '* Fixed a foo. #12345 (Also in 7.2, 8, 9)' +const renderNote = (commit, excludeBranch) => + `* ${renderDescription(commit)} ${renderLink(commit)} ${renderTrops(commit, excludeBranch)}\n`; -const renderNotes = (notes, explicitLinks) => { +const renderNotes = (notes) => { const rendered = [`# Release Notes for ${notes.name}\n\n`]; const renderSection = (title, commits) => { - if (commits.length === 0) { - return; + if (commits.length > 0) { + rendered.push( + `## ${title}\n\n`, + ...(commits.map(commit => renderNote(commit, notes.toBranch)).sort()) + ); } - const notes = new Map(); - for (const note of commits.map(commit => renderCommit(commit, explicitLinks))) { - if (!notes.has(note.note)) { - notes.set(note.note, [note.link]); - } else { - notes.get(note.note).push(note.link); - } - } - rendered.push(`## ${title}\n\n`); - const lines = []; - notes.forEach((links, key) => lines.push(` * ${key} ${links.map(link => link.toString()).sort().join(', ')}\n`)); - rendered.push(...lines.sort(), '\n'); }; renderSection('Breaking Changes', notes.breaking); @@ -638,7 +590,7 @@ const renderNotes = (notes, explicitLinks) => { renderSection('Other Changes', notes.other); if (notes.docs.length) { - const docs = notes.docs.map(commit => renderLink(commit, explicitLinks)).sort(); + const docs = notes.docs.map(commit => renderLink(commit)).sort(); rendered.push('## Documentation\n\n', ` * Documentation changes: ${docs.join(', ')}\n`, '\n'); } diff --git a/spec-main/release-notes-spec.ts b/spec-main/release-notes-spec.ts index 82c15f88e154..e88acabfadce 100644 --- a/spec-main/release-notes-spec.ts +++ b/spec-main/release-notes-spec.ts @@ -67,6 +67,14 @@ class GitFake { } } stdout = lines.join('\n'); + } else if (args.length === 6 && + args[0] === 'branch' && + args[1] === '--all' && + args[2] === '--contains' && + args[3].endsWith('-x-y')) { + // "what branch is this tag in?" + // git branch --all --contains ${ref} --sort version:refname + stdout = args[3]; } else { console.error('unhandled GitProcess.exec():', args); } @@ -115,25 +123,15 @@ describe('release notes', () => { sandbox.restore(); }); - describe('changes that exist in older branches', () => { - // use case: this fix is NOT news because it was already fixed - // while oldBranch was the latest stable release - it('are skipped if the target version is a new major line (x.0.0)', async function () { + describe('trop annotations', () => { + it('shows sibling branches', async function () { const version = 'v9.0.0'; gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]); gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]); const results: any = await notes.get(oldBranch, newBranch, version); - expect(results.fix).to.have.lengthOf(0); - }); - - // use case: this fix IS news because it's being fixed in - // multiple stable branches at once, including newBranch. - it('are included if the target version is a minor or patch bump', async function () { - const version = 'v9.0.1'; - gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]); - gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]); - const results: any = await notes.get(oldBranch, newBranch, version); expect(results.fix).to.have.lengthOf(1); + console.log(results.fix); + expect(results.fix[0].trops).to.have.keys('8-x-y', '9-x-y'); }); });