From 70782b3677e40472260853df92d3ca8b805af422 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Thu, 1 Sep 2022 13:43:44 -0700 Subject: [PATCH] fix: update a few release nits after trying it on the cli (#179) - Correctly link workspace dep changelog entry to release even when release component is different than the package name - Update changelog entry also when rewriting workspaces deps - Clone repo during release process with `fetch-depth: 0` so we can have full commit history to build `AUTHORS` files - Removes `preversion`, `postversion`, and `prepublishOnly` scripts now that all tagging happens via `release-please` - Correctly version root when going from prerelease -> release Closes #178 --- .github/workflows/release-please.yml | 1 + lib/content/pkg.json | 10 +- lib/content/release-please.yml | 2 +- lib/release-please/index.js | 6 +- lib/release-please/node-workspace.js | 11 ++ lib/release-please/workspace-deps.js | 103 +++++++++++------- package.json | 3 - .../test/apply/full-content.js.test.cjs | 21 +--- tap-snapshots/test/check/diffs.js.test.cjs | 2 +- tap-snapshots/test/check/index.js.test.cjs | 12 -- test/apply/index.js | 9 +- test/release-please/workspace-deps.js | 73 ++++++++++--- 12 files changed, 149 insertions(+), 104 deletions(-) create mode 100644 lib/release-please/node-workspace.js diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index 7da210b2..434bd7aa 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -49,6 +49,7 @@ jobs: run: echo "::set-output name=branch::${{ fromJSON(needs.release-please.outputs.pr).headBranchName }}" - uses: actions/checkout@v3 with: + fetch-depth: 0 ref: ${{ steps.ref.outputs.branch }} - name: Setup git user run: | diff --git a/lib/content/pkg.json b/lib/content/pkg.json index ce0d9571..d8775b23 100644 --- a/lib/content/pkg.json +++ b/lib/content/pkg.json @@ -6,13 +6,9 @@ "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "{{npmBin}} run lint -- --fix", - "preversion": "{{npmBin}} test", - {{#if pkgPrivate}} - "postversion": "git push origin --follow-tags", - {{else}} - "postversion": "{{npmBin}} publish", - "prepublishOnly": "git push origin --follow-tags", - {{/if}} + "preversion": {{{del}}}, + "postversion": {{{del}}}, + "prepublishOnly": {{{del}}}, "snap": "tap", "test": "tap", "posttest": "{{npmBin}} run lint", diff --git a/lib/content/release-please.yml b/lib/content/release-please.yml index 4f814b92..e80e6122 100644 --- a/lib/content/release-please.yml +++ b/lib/content/release-please.yml @@ -37,7 +37,7 @@ jobs: - name: Output ref id: ref run: echo "::set-output name=branch::$\{{ fromJSON(needs.release-please.outputs.pr).headBranchName }}" - {{> setupGit with=(obj ref="${{ steps.ref.outputs.branch }}")}} + {{> setupGit with=(obj ref="${{ steps.ref.outputs.branch }}" fetch-depth=0)}} {{> setupNode}} {{> setupDeps}} - name: Post pull request actions diff --git a/lib/release-please/index.js b/lib/release-please/index.js index 421370a6..19f38fa6 100644 --- a/lib/release-please/index.js +++ b/lib/release-please/index.js @@ -3,11 +3,15 @@ const logger = require('./logger.js') const ChangelogNotes = require('./changelog.js') const Version = require('./version.js') const WorkspaceDeps = require('./workspace-deps.js') +const NodeWorkspace = require('./node-workspace.js') RP.setLogger(logger) RP.registerChangelogNotes('default', (options) => new ChangelogNotes(options)) RP.registerVersioningStrategy('default', (options) => new Version(options)) -RP.registerPlugin('workspace-deps', (options) => new WorkspaceDeps(options)) +RP.registerPlugin('workspace-deps', (o) => + new WorkspaceDeps(o.github, o.targetBranch, o.repositoryConfig)) +RP.registerPlugin('node-workspace', (o) => + new NodeWorkspace(o.github, o.targetBranch, o.repositoryConfig)) const main = async ({ repo: fullRepo, token, dryRun, branch }) => { if (!token) { diff --git a/lib/release-please/node-workspace.js b/lib/release-please/node-workspace.js new file mode 100644 index 00000000..d06c7da1 --- /dev/null +++ b/lib/release-please/node-workspace.js @@ -0,0 +1,11 @@ +const Version = require('./version.js') +const RP = require('release-please/build/src/plugins/node-workspace') + +module.exports = class NodeWorkspace extends RP.NodeWorkspace { + bumpVersion (pkg) { + // The default release please node-workspace plugin forces a patch + // bump for the root if it only includes workspace dep updates. + // This does the same thing except it respects the prerelease config. + return new Version(pkg).bump(pkg.version, [{ type: 'fix' }]) + } +} diff --git a/lib/release-please/workspace-deps.js b/lib/release-please/workspace-deps.js index be1e74a0..fd33b64c 100644 --- a/lib/release-please/workspace-deps.js +++ b/lib/release-please/workspace-deps.js @@ -1,4 +1,6 @@ const { ManifestPlugin } = require('release-please/build/src/plugin') +const { Changelog } = require('release-please/build/src/updaters/changelog.js') +const { PackageJson } = require('release-please/build/src/updaters/node/package-json.js') const matchLine = (line, re) => { const trimmed = line.trim().replace(/^[*\s]+/, '') @@ -10,61 +12,86 @@ const matchLine = (line, re) => { module.exports = class WorkspaceDeps extends ManifestPlugin { run (pullRequests) { - for (const { pullRequest } of pullRequests) { - const depLinks = pullRequest.body.releaseData.reduce((acc, release) => { - if (release.component) { - const url = matchLine(release.notes.split('\n')[0], /\[[^\]]+\]\((.*?)\)/) - if (url) { - acc[release.component] = url[1] + try { + for (const { pullRequest } of pullRequests) { + const getChangelog = (release) => pullRequest.updates.find((u) => { + const isChangelog = u.updater instanceof Changelog + const isComponent = release.component && u.path.startsWith(release.component) + const isRoot = !release.component && !u.path.includes('/') + return isChangelog && (isComponent || isRoot) + }) + + const getComponent = (pkgName) => pullRequest.updates.find((u) => { + const isPkg = u.updater instanceof PackageJson + return isPkg && JSON.parse(u.updater.rawContent).name === pkgName + }).path.replace(/\/package\.json$/, '') + + const depLinksByComponent = pullRequest.body.releaseData.reduce((acc, release) => { + if (release.component) { + const path = [ + this.github.repository.owner, + this.github.repository.repo, + 'releases', + 'tag', + release.tag.toString(), + ] + acc[release.component] = `https://github.com/${path.join('/')}` } - } - return acc - }, {}) + return acc + }, {}) - for (const release of pullRequest.body.releaseData) { - const lines = release.notes.split('\n') - const newLines = [] + for (const release of pullRequest.body.releaseData) { + const lines = release.notes.split('\n') + const newLines = [] - let inWorkspaceDeps = false - let collectWorkspaceDeps = false + let inWorkspaceDeps = false + let collectWorkspaceDeps = false - for (const line of lines) { - if (matchLine(line, 'The following workspace dependencies were updated')) { + for (const line of lines) { + if (matchLine(line, 'The following workspace dependencies were updated')) { // We are in the section with our workspace deps // Set the flag and discard this line since we dont want it in the final output - inWorkspaceDeps = true - } else if (inWorkspaceDeps) { - if (collectWorkspaceDeps) { - const depMatch = matchLine(line, /^(\S+) bumped from \S+ to (\S+)$/) - if (depMatch) { + inWorkspaceDeps = true + } else if (inWorkspaceDeps) { + if (collectWorkspaceDeps) { + const depMatch = matchLine(line, /^(\S+) bumped from \S+ to (\S+)$/) + if (depMatch) { // If we have a line that is a workspace dep update, then reformat // it and save it to the new lines - const [, depName, newVersion] = depMatch - const depSpec = `\`${depName}@${newVersion}\`` - const url = depLinks[depName] - newLines.push(` * ${url ? `[${depSpec}](${url})` : depSpec}`) - } else { + const [, depName, newVersion] = depMatch + const depSpec = `\`${depName}@${newVersion}\`` + const url = depLinksByComponent[getComponent(depName)] + newLines.push(` * deps: [${depSpec}](${url})`) + } else { // Anything else means we are done with dependencies so ignore // this line and dont look for any more - collectWorkspaceDeps = false - } - } else if (matchLine(line, 'dependencies')) { + collectWorkspaceDeps = false + } + } else if (matchLine(line, 'dependencies')) { // Only collect dependencies discard dev deps and everything else - collectWorkspaceDeps = true - } else if (matchLine(line, '') || matchLine(line, /^#/)) { - inWorkspaceDeps = false + collectWorkspaceDeps = true + } else if (matchLine(line, '') || matchLine(line, /^#/)) { + inWorkspaceDeps = false + newLines.push(line) + } + } else { newLines.push(line) } - } else { - newLines.push(line) } - } - const newNotes = newLines.join('\n').trim() - const emptyDeps = newNotes.match(/### Dependencies[\n]+(### .*)/m) + let newNotes = newLines.join('\n').trim() + const emptyDeps = newNotes.match(/### Dependencies[\n]+(### .*)/m) + if (emptyDeps) { + newNotes = newNotes.replace(emptyDeps[0], emptyDeps[1]) + } - release.notes = emptyDeps ? newNotes.replace(emptyDeps[0], emptyDeps[1]) : newNotes + release.notes = newNotes + getChangelog(release).updater.changelogEntry = newNotes + } } + } catch { + // Always return pull requests even if we failed so + // we dont fail the release } return pullRequests diff --git a/package.json b/package.json index 8619f7a5..53ec1eb5 100644 --- a/package.json +++ b/package.json @@ -12,9 +12,6 @@ "lint": "eslint \"**/*.js\"", "lintfix": "npm run lint -- --fix", "posttest": "npm run lint", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", - "preversion": "npm test", "snap": "tap", "test": "tap", "template-oss-apply": "template-oss-apply --force", diff --git a/tap-snapshots/test/apply/full-content.js.test.cjs b/tap-snapshots/test/apply/full-content.js.test.cjs index cb19cd7b..6f30b7c2 100644 --- a/tap-snapshots/test/apply/full-content.js.test.cjs +++ b/tap-snapshots/test/apply/full-content.js.test.cjs @@ -470,6 +470,7 @@ jobs: run: echo "::set-output name=branch::\${{ fromJSON(needs.release-please.outputs.pr).headBranchName }}" - uses: actions/checkout@v3 with: + fetch-depth: 0 ref: \${{ steps.ref.outputs.branch }} - name: Setup git user run: | @@ -678,9 +679,6 @@ package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -1398,6 +1396,7 @@ jobs: run: echo "::set-output name=branch::\${{ fromJSON(needs.release-please.outputs.pr).headBranchName }}" - uses: actions/checkout@v3 with: + fetch-depth: 0 ref: \${{ steps.ref.outputs.branch }} - name: Setup git user run: | @@ -1613,9 +1612,6 @@ package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -1737,9 +1733,6 @@ workspaces/a/package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -1816,9 +1809,6 @@ workspaces/b/package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -2117,6 +2107,7 @@ jobs: run: echo "::set-output name=branch::\${{ fromJSON(needs.release-please.outputs.pr).headBranchName }}" - uses: actions/checkout@v3 with: + fetch-depth: 0 ref: \${{ steps.ref.outputs.branch }} - name: Setup git user run: | @@ -2375,9 +2366,6 @@ workspaces/a/package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -2450,9 +2438,6 @@ workspaces/b/package.json "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" diff --git a/tap-snapshots/test/check/diffs.js.test.cjs b/tap-snapshots/test/check/diffs.js.test.cjs index c6501a47..5da3c33e 100644 --- a/tap-snapshots/test/check/diffs.js.test.cjs +++ b/tap-snapshots/test/check/diffs.js.test.cjs @@ -434,7 +434,7 @@ The module file package.json needs to be updated: "author" is "heynow", expected "GitHub Inc." "files[1]" is "x", expected "lib/" "scripts.lint:fix" is "x", expected to be removed - "scripts.preversion" is "x", expected "npm test" + "scripts.preversion" is "x", expected to be removed "standard" is { "config": "x " }, expected to be removed diff --git a/tap-snapshots/test/check/index.js.test.cjs b/tap-snapshots/test/check/index.js.test.cjs index e508cb31..4bf7260c 100644 --- a/tap-snapshots/test/check/index.js.test.cjs +++ b/tap-snapshots/test/check/index.js.test.cjs @@ -61,9 +61,6 @@ The module file package.json needs to be updated: "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -144,9 +141,6 @@ The module file package.json needs to be updated: "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -209,9 +203,6 @@ The module file package.json needs to be updated: "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" @@ -274,9 +265,6 @@ The module file package.json needs to be updated: "postlint": "template-oss-check", "template-oss-apply": "template-oss-apply --force", "lintfix": "npm run lint -- --fix", - "preversion": "npm test", - "postversion": "npm publish", - "prepublishOnly": "git push origin --follow-tags", "snap": "tap", "test": "tap", "posttest": "npm run lint" diff --git a/test/apply/index.js b/test/apply/index.js index 40f80fff..442a1a7a 100644 --- a/test/apply/index.js +++ b/test/apply/index.js @@ -113,13 +113,10 @@ t.test('private workspace', async (t) => { const rpManifest = await s.readJson('.release-please-manifest.json') const rpConfig = await s.readJson('release-please-config.json') - t.ok(pkg.scripts.prepublishOnly) - t.ok(pkg.scripts.postversion) - + t.notOk(pkg.scripts.prepublishOnly) + t.notOk(pkg.scripts.postversion) t.notOk(privatePkg.scripts.prepublishOnly) - t.ok(privatePkg.scripts.postversion) - - t.equal(pkg.scripts.prepublishOnly, privatePkg.scripts.postversion) + t.notOk(privatePkg.scripts.postversion) t.equal(rpManifest['.'], '1.0.0') t.equal(rpManifest['workspaces/b'], '1.0.0') diff --git a/test/release-please/workspace-deps.js b/test/release-please/workspace-deps.js index 07a16a2f..8159bb1d 100644 --- a/test/release-please/workspace-deps.js +++ b/test/release-please/workspace-deps.js @@ -1,24 +1,57 @@ const t = require('tap') const WorkspaceDeps = require('../../lib/release-please/workspace-deps.js') +const { Changelog } = require('release-please/build/src/updaters/changelog.js') +const { PackageJson } = require('release-please/build/src/updaters/node/package-json.js') +const { TagName } = require('release-please/build/src/util/tag-name.js') +const { Version } = require('release-please/build/src/version.js') const mockWorkspaceDeps = (notes) => { - const pullRequests = new WorkspaceDeps().run([{ + const releases = [{ + component: '', + notes: notes.join('\n'), + }, { + component: 'pkg2', + notes: '### no link', + tag: new TagName(new Version(2, 0, 0), 'pkg2'), + }, { + component: 'pkg1', + notes: '### [sdsfsdf](http://url1)', + tag: new TagName(new Version(2, 0, 0), 'pkg1'), + }, { + component: 'pkg3', + notes: '### [sdsfsdf](http://url3)', + tag: new TagName(new Version(2, 0, 0), 'pkg3'), + }] + + const options = { github: { repository: { owner: 'npm', repo: 'cli' } } } + + const pullRequests = new WorkspaceDeps(options.github).run([{ pullRequest: { body: { - releaseData: [{ - component: '', - notes: notes.join('\n'), - }, { - component: 'pkg2', - notes: '### no link', - }, { - component: 'pkg1', - notes: '### [sdsfsdf](http://url1)', - }], + releaseData: releases, }, + updates: [ + ...releases.map(r => ({ + path: `${r.component ? `${r.component}/` : ''}CHANGELOG.md`, + updater: new Changelog({ changelogEntry: notes.join('\n') }), + })), + ...releases.map(r => { + const pkg = new PackageJson({ version: '1.0.0' }) + pkg.rawContent = JSON.stringify({ + name: r.component === 'pkg1' ? '@scope/pkg1' : (r.component || 'root'), + }) + return { + path: `${r.component ? `${r.component}/` : ''}package.json`, + updater: pkg, + } + }), + ], }, }]) - return pullRequests[0].pullRequest.body.releaseData[0].notes.split('\n') + return { + pr: pullRequests[0].pullRequest.body.releaseData[0].notes.split('\n'), + changelog: pullRequests[0].pullRequest.updates[0].updater.changelogEntry.split('\n'), + } } const fixtures = { @@ -34,8 +67,9 @@ const fixtures = { ' * pkgA bumped from ^1.0.0 to ^2.0.0', ' * pkgB bumped from ^1.0.0 to ^2.0.0', ' * dependencies', - ' * pkg1 bumped from ^1.0.0 to ^2.0.0', + ' * @scope/pkg1 bumped from ^1.0.0 to ^2.0.0', ' * pkg2 bumped from ^1.0.0 to ^2.0.0', + ' * pkg3 bumped from ^1.0.0 to ^2.0.0', ' * devDependencies', ' * pkgC bumped from ^1.0.0 to ^2.0.0', ' * pkgD bumped from ^1.0.0 to ^2.0.0', @@ -66,22 +100,26 @@ const fixtures = { } t.test('rewrite deps', async (t) => { - t.strictSame(mockWorkspaceDeps(fixtures.wsDeps), [ + const mockWs = mockWorkspaceDeps(fixtures.wsDeps) + t.strictSame(mockWs.pr, [ '### Feat', '', ' * xyz', '', '### Dependencies', '', - ' * [`pkg1@^2.0.0`](http://url1)', - ' * `pkg2@^2.0.0`', + ' * deps: [`@scope/pkg1@^2.0.0`](https://github.com/npm/cli/releases/tag/pkg1-v2.0.0)', + ' * deps: [`pkg2@^2.0.0`](https://github.com/npm/cli/releases/tag/pkg2-v2.0.0)', + ' * deps: [`pkg3@^2.0.0`](https://github.com/npm/cli/releases/tag/pkg3-v2.0.0)', '', '### Next', '', ' * xyz', ]) + t.strictSame(mockWs.pr, mockWs.changelog) - t.strictSame(mockWorkspaceDeps(fixtures.empty), [ + const mockEmpty = mockWorkspaceDeps(fixtures.empty) + t.strictSame(mockEmpty.pr, [ '### Feat', '', ' * xyz', @@ -90,4 +128,5 @@ t.test('rewrite deps', async (t) => { '', ' * xyz', ]) + t.strictSame(mockEmpty.pr, mockEmpty.changelog) })