mirrors/electron
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions 1

chore: add trop annotations to release notes. (#24672)

Browse source 
Trop annotations are in the form of "(Also in 7.3, 8, 9)" with links to
the sibling branches.

Previously seen in b43e601b83 but is now
free of optional chaining and nullish coalescing, to run on Node < 14 :)
This commit is contained in:
Charles Kerr 2020-07-27 10:01:41 -05:00 • committed by GitHub
parent 91f5837344
commit b39a5b71fe
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 130 additions and 179 deletions
Download patch file Download diff file Expand all files Collapse all files

12
script/release/notes/index.js
View file

@ -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 rangeList = range.split('..') || ['HEAD'];
const to = rangeList.pop(); const to = rangeList.pop();
const from = rangeList.pop() || (await getPreviousPoint(to)); const from = rangeList.pop() || (await getPreviousPoint(to));
@ -129,10 +129,9 @@ async function getReleaseNotes (range, newVersion, explicitLinks) {
newVersion = to; newVersion = to;
} }
console.log(`Generating release notes between ${from} and ${to} for version ${newVersion}`);
const notes = await notesGenerator.get(from, to, newVersion); const notes = await notesGenerator.get(from, to, newVersion);
const ret = { const ret = {
text: notesGenerator.render(notes, explicitLinks) text: notesGenerator.render(notes)
}; };
if (notes.unknown.length) { if (notes.unknown.length) {
@ -144,7 +143,7 @@ async function getReleaseNotes (range, newVersion, explicitLinks) {
async function main () { async function main () {
const opts = minimist(process.argv.slice(2), { const opts = minimist(process.argv.slice(2), {
boolean: ['explicit-links', 'help'], boolean: ['help'],
string: ['version'] string: ['version']
}); });
opts.range = opts._.shift(); opts.range = opts._.shift();
@ -153,14 +152,13 @@ async function main () {
console.log(` console.log(`
easy usage: ${name} version 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 -- * 'begin' and 'end' are two git references -- tags, branches, etc --
from which the release notes are generated. from which the release notes are generated.
* if omitted, 'begin' defaults to the previous tag in end's branch. * if omitted, 'begin' defaults to the previous tag in end's branch.
* if omitted, 'version' defaults to 'end'. Specifying a version is * 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. 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: For example, these invocations are equivalent:
${process.argv[1]} v4.0.1 ${process.argv[1]} v4.0.1
@ -169,7 +167,7 @@ For example, these invocations are equivalent:
return 0; 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); console.log(notes.text);
if (notes.warning) { if (notes.warning) {
throw new Error(notes.warning); throw new Error(notes.warning);

267
script/release/notes/notes.js
View file

@ -2,24 +2,22 @@
'use strict'; 'use strict';
const childProcess = require('child_process');
const fs = require('fs'); const fs = require('fs');
const os = require('os');
const path = require('path'); const path = require('path');
const { GitProcess } = require('dugite'); const { GitProcess } = require('dugite');
const octokit = require('@octokit/rest')({ const octokit = require('@octokit/rest')({
auth: process.env.ELECTRON_GITHUB_TOKEN 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 MAX_FAIL_COUNT = 3;
const CHECK_INTERVAL = 5000; const CHECK_INTERVAL = 5000;
const TROP_LOGIN = 'trop[bot]';
const NO_NOTES = 'No notes'; const NO_NOTES = 'No notes';
const FOLLOW_REPOS = ['electron/electron', 'electron/node'];
const docTypes = new Set(['doc', 'docs']); const docTypes = new Set(['doc', 'docs']);
const featTypes = new Set(['feat', 'feature']); const featTypes = new Set(['feat', 'feature']);
@ -57,6 +55,11 @@ class Commit {
this.isBreakingChange = false; this.isBreakingChange = false;
this.note = null; // string 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<string,GHKey>
this.prKeys = new Set(); // GHKey this.prKeys = new Set(); // GHKey
this.revertHash = null; // string this.revertHash = null; // string
this.semanticType = null; // string this.semanticType = null; // string
@ -107,7 +110,7 @@ const getNoteFromClerk = async (ghKey) => {
if (comment.body.startsWith(PERSIST_LEAD)) { if (comment.body.startsWith(PERSIST_LEAD)) {
return comment.body return comment.body
.slice(PERSIST_LEAD.length).trim() // remove PERSIST_LEAD .slice(PERSIST_LEAD.length).trim() // remove PERSIST_LEAD
.split('\r?\n') // break into lines .split(/\r?\n/) // split into lines
.map(line => line.trim()) .map(line => line.trim())
.filter(line => line.startsWith(QUOTE_LEAD)) // notes are quoted .filter(line => line.startsWith(QUOTE_LEAD)) // notes are quoted
.map(line => line.slice(QUOTE_LEAD.length)) // unquote the lines .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: * 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 getLocalCommitHashes = async (dir, ref) => {
const args = ['log', '--format=%H', 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 // return an array of Commits
@ -216,7 +209,9 @@ const getLocalCommits = async (module, point1, point2) => {
const fieldSep = ','; const fieldSep = ',';
const format = ['%H', '%s'].join(fieldSep); const format = ['%H', '%s'].join(fieldSep);
const args = ['log', '--cherry-pick', '--right-only', '--first-parent', `--format=${format}`, `${point1}..${point2}`]; 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 = []; const commits = [];
for (const log of logs) { for (const log of logs) {
@ -332,92 +327,52 @@ const addRepoToPool = async (pool, repo, from, to) => {
} }
}; };
/*** // @return Map<string,GHKey>
**** Other Repos // 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 getMergedTrops (commit, pool) {
const branches = new Map();
// other repos - gn for (const prKey of commit.prKeys.values()) {
const pull = pool.pulls[prKey.number];
const getDepsVariable = async (ref, key) => { const mergedBranches = new Set(
// get a copy of that reference point's DEPS file ((pull && pull.data && pull.data.labels) ? pull.data.labels : [])
const deps = await runGit(ELECTRON_VERSION, ['show', `${ref}:DEPS`]); .map(label => ((label && label.name) ? label.name : '').match(/merged\/([0-9]+-[x0-9]-[xy0-9])/))
const filename = path.resolve(os.tmpdir(), 'DEPS'); .filter(match => match)
fs.writeFileSync(filename, deps); .map(match => match[1])
// query the DEPS file
const response = childProcess.spawnSync(
'gclient',
['getdep', '--deps-file', filename, '--var', key],
{ encoding: 'utf8' }
); );
// cleanup if (mergedBranches.size > 0) {
fs.unlinkSync(filename); const isTropComment = (comment) => comment && comment.user && comment.user.login === TROP_LOGIN;
return response.stdout.trim();
};
const getDependencyCommitsGN = async (pool, fromRef, toRef) => { const ghKey = GHKey.NewFromPull(pull.data);
const repos = [{ // just node const backportRegex = /backported this PR to "(.*)",\s+please check out #(\d+)/;
owner: 'electron', const getBranchNameAndPullKey = (comment) => {
repo: 'node', const match = ((comment && comment.body) ? comment.body : '').match(backportRegex);
dir: path.resolve(SRC_DIR, 'third_party', 'electron_node'), return match ? [match[1], new GHKey(ghKey.owner, ghKey.repo, parseInt(match[2]))] : null;
deps_variable_name: 'node_version' };
}];
for (const repo of repos) { const comments = await getComments(ghKey);
// the 'DEPS' file holds the dependency reference point ((comments && comments.data) ? comments.data : [])
const key = repo.deps_variable_name; .filter(isTropComment)
const from = await getDepsVariable(fromRef, key); .map(getBranchNameAndPullKey)
const to = await getDepsVariable(toRef, key); .filter(pair => pair)
await addRepoToPool(pool, repo, from, to); .filter(([branch]) => mergedBranches.has(branch))
.forEach(([branch, key]) => branches.set(branch, key));
} }
};
// 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;
} }
return show; return branches;
};
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();
} }
function getOldestMajorBranchOfCommit (commit, pool) { // @return the shorthand name of the branch that `ref` is on,
return [...commit.prKeys.values()] // e.g. a ref of '10.0.0-beta.1' will return '10-x-y'
.map(prKey => pool.pulls[prKey.number]) async function getBranchNameOfRef (ref, dir) {
.filter(pull => !!pull) return (await runGit(dir, ['branch', '--all', '--contains', ref, '--sort', 'version:refname']))
.map(pull => getOldestMajorBranchOfPull(pull)) .split(/\r?\n/) // split into lines
.filter(major => !!major) .shift() // we sorted by refname and want the first result
.sort() .match(/(?:.*\/)?(.*)/)[1] // 'remote/origins/10-x-y' -> '10-x-y'
.shift(); .trim();
}
function commitExistsBeforeMajor (commit, pool, major) {
const firstAppearance = getOldestMajorBranchOfCommit(commit, pool);
return firstAppearance && (firstAppearance < major);
} }
/*** /***
@ -431,21 +386,15 @@ const getNotes = async (fromRef, toRef, newVersion) => {
} }
const pool = new Pool(); 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 // 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); 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 // remove any old commits
pool.commits = pool.commits.filter(commit => !pool.processedHashes.has(commit.hash)); pool.commits = pool.commits.filter(commit => !pool.processedHashes.has(commit.hash));
@ -479,12 +428,12 @@ const getNotes = async (fromRef, toRef, newVersion) => {
// remove non-user-facing commits // remove non-user-facing commits
pool.commits = pool.commits pool.commits = pool.commits
.filter(commit => commit.note && (commit.note !== NO_NOTES)) .filter(commit => commit && commit.note)
.filter(commit => !((commit.note || commit.subject).match(/^[Bb]ump v\d+\.\d+\.\d+/))); .filter(commit => commit.note !== NO_NOTES)
.filter(commit => commit.note.match(/^[Bb]ump v\d+\.\d+\.\d+/) === null);
if (!shouldIncludeMultibranchChanges(newVersion)) { for (const commit of pool.commits) {
const { major } = semver.parse(newVersion); commit.trops = await getMergedTrops(commit, pool);
pool.commits = pool.commits.filter(commit => !commitExistsBeforeMajor(commit, pool, major));
} }
pool.commits = removeSupercededStackUpdates(pool.commits); pool.commits = removeSupercededStackUpdates(pool.commits);
@ -496,7 +445,8 @@ const getNotes = async (fromRef, toRef, newVersion) => {
fix: [], fix: [],
other: [], other: [],
unknown: [], unknown: [],
name: newVersion name: newVersion,
toBranch
}; };
pool.commits.forEach(commit => { pool.commits.forEach(commit => {
@ -545,31 +495,42 @@ const removeSupercededStackUpdates = (commits) => {
**** Render **** Render
***/ ***/
const renderLink = (commit, explicitLinks) => { // @return the pull request's GitHub URL
let link; const buildPullURL = ghKey => `https://github.com/${ghKey.owner}/${ghKey.repo}/pull/${ghKey.number}`;
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;
};
const renderCommit = (commit, explicitLinks) => { const renderPull = ghKey => `[#${ghKey.number}](${buildPullURL(ghKey)})`;
// clean up the note
let note = commit.note || commit.subject; // @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 ? `<span style="font-size:small;">(Also in ${body})</span>` : body;
}
// @return a slightly cleaned-up human-readable change description
function renderDescription (commit) {
let note = commit.note || commit.subject || '';
note = note.trim(); note = note.trim();
if (note.length !== 0) { if (note.length !== 0) {
note = note[0].toUpperCase() + note.substr(1); note = note[0].toUpperCase() + note.substr(1);
@ -606,30 +567,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 rendered = [`# Release Notes for ${notes.name}\n\n`];
const renderSection = (title, commits) => { const renderSection = (title, commits) => {
if (commits.length === 0) { if (commits.length > 0) {
return; 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); renderSection('Breaking Changes', notes.breaking);
@ -638,7 +593,7 @@ const renderNotes = (notes, explicitLinks) => {
renderSection('Other Changes', notes.other); renderSection('Other Changes', notes.other);
if (notes.docs.length) { 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'); rendered.push('## Documentation\n\n', ` * Documentation changes: ${docs.join(', ')}\n`, '\n');
} }

26
spec-main/release-notes-spec.ts
View file

@ -67,6 +67,14 @@ class GitFake {
} }
} }
stdout = lines.join('\n'); 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 { } else {
console.error('unhandled GitProcess.exec():', args); console.error('unhandled GitProcess.exec():', args);
} }
@ -115,25 +123,15 @@ describe('release notes', () => {
sandbox.restore(); sandbox.restore();
}); });
describe('changes that exist in older branches', () => { describe('trop annotations', () => {
// use case: this fix is NOT news because it was already fixed it('shows sibling branches', async function () {
// while oldBranch was the latest stable release
it('are skipped if the target version is a new major line (x.0.0)', async function () {
const version = 'v9.0.0'; const version = 'v9.0.0';
gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]); gitFake.setBranch(oldBranch, [...sharedHistory, oldTropFix]);
gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]); gitFake.setBranch(newBranch, [...sharedHistory, newTropFix]);
const results: any = await notes.get(oldBranch, newBranch, version); 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); 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');
}); });
}); });