From 15c75a92761f844dfeb8dec36efbb15af6848e27 Mon Sep 17 00:00:00 2001 From: Hajime-san <41257923+Hajime-san@users.noreply.github.com> Date: Wed, 2 Oct 2024 07:16:57 +0900 Subject: [PATCH] fix: make workspace plugin with `separate-pull-request` fine (#2310) * fix merging behaviour in plugin * add test * remove unused test snapshot * update licnese header * considering plugin schema carefully * add unit test * remove `determineMerge` and set default value directly * set default value for `separatePullRequests` in `buildPlugin` --------- Co-authored-by: Jeff Ching --- __snapshots__/php-manifest.js | 59 ------ .../separate-pull-requests-workspace.js | 51 +++++ src/factories/plugin-factory.ts | 15 ++ src/manifest.ts | 1 + test/factories/plugin-factory.ts | 6 + .../separate-pull-requests-workspace.ts | 198 ++++++++++++++++++ 6 files changed, 271 insertions(+), 59 deletions(-) delete mode 100644 __snapshots__/php-manifest.js create mode 100644 __snapshots__/separate-pull-requests-workspace.js create mode 100644 test/plugins/compatibility/separate-pull-requests-workspace.ts diff --git a/__snapshots__/php-manifest.js b/__snapshots__/php-manifest.js deleted file mode 100644 index 38e541e2a..000000000 --- a/__snapshots__/php-manifest.js +++ /dev/null @@ -1,59 +0,0 @@ -exports['PHPManifest updateContent update version in docs manifest 1'] = ` -{ - "lang": "php", - "friendlyLang": "PHP", - "libraryTitle": "Google Cloud Client Library", - "moduleName": "google-cloud-php", - "markdown": "php", - "defaultModule": "google-cloud", - "modules": [ - { - "id": "google-cloud", - "name": "google/cloud", - "defaultService": "servicebuilder", - "versions": [ - "v0.8.0", - "v0.7.1", - "v0.7.0", - "v0.6.0", - "v0.5.1", - "v0.5.0", - "v0.4.1", - "v0.4.0", - "v0.3.0", - "master" - ] - }, - { - "id": "access-context-manager", - "name": "google/access-context-manager", - "defaultService": "accesscontextmanager/readme", - "versions": [ - "v0.2.0", - "v0.1.1", - "v0.1.0", - "master" - ] - }, - { - "id": "analytics-admin", - "name": "google/analytics-admin", - "defaultService": "analyticsadmin/readme", - "versions": [ - "v0.4.1", - "v0.4.0", - "v0.3.0", - "v0.2.0", - "v0.1.0", - "master" - ] - } - ], - "content": "json", - "home": "home.html", - "package": { - "title": "Packagist", - "href": "https://packagist.org/packages/google/cloud" - } -} -` diff --git a/__snapshots__/separate-pull-requests-workspace.js b/__snapshots__/separate-pull-requests-workspace.js new file mode 100644 index 000000000..79dd3f998 --- /dev/null +++ b/__snapshots__/separate-pull-requests-workspace.js @@ -0,0 +1,51 @@ +exports['Plugin compatibility separate-pull-requests and workspace plugin should version bump dependencies together 1'] = ` +:robot: I have created a release *beep* *boop* +--- + + +## [1.0.1](https://github.com/fake-owner/fake-repo/compare/pkgA-v1.0.0...pkgA-v1.0.1) (1983-10-10) + + +### Dependencies + +* The following workspace dependencies were updated + * dependencies + * pkgC bumped to 1.1.0 + +--- +This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). +` + +exports['Plugin compatibility separate-pull-requests and workspace plugin should version bump dependencies together 2'] = ` +:robot: I have created a release *beep* *boop* +--- + + +## [1.0.1](https://github.com/fake-owner/fake-repo/compare/pkgB-v1.0.0...pkgB-v1.0.1) (1983-10-10) + + +### Dependencies + +* The following workspace dependencies were updated + * dependencies + * pkgC bumped to 1.1.0 + +--- +This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). +` + +exports['Plugin compatibility separate-pull-requests and workspace plugin should version bump dependencies together 3'] = ` +:robot: I have created a release *beep* *boop* +--- + + +## [1.1.0](https://github.com/fake-owner/fake-repo/compare/pkgC-v1.0.0...pkgC-v1.1.0) (1983-10-10) + + +### Features + +* some feature ([aaaaaa](https://github.com/fake-owner/fake-repo/commit/aaaaaa)) + +--- +This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). +` diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index 242346853..5f2756e51 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -38,6 +38,7 @@ export interface PluginFactoryOptions { targetBranch: string; repositoryConfig: RepositoryConfig; manifestPath: string; + separatePullRequests?: boolean; // node options alwaysLinkLocal?: boolean; @@ -54,6 +55,8 @@ export type PluginBuilder = (options: PluginFactoryOptions) => ManifestPlugin; const pluginFactories: Record = { 'linked-versions': options => + // NOTE: linked-versions had already have a different behavior about merging + // see test/plugins/compatibility/linked-versions-workspace.ts new LinkedVersions( options.github, options.targetBranch, @@ -73,6 +76,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'node-workspace': options => @@ -83,6 +89,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'maven-workspace': options => @@ -93,6 +102,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'sentence-case': options => @@ -112,6 +124,9 @@ const pluginFactories: Record = { }; export function buildPlugin(options: PluginFactoryOptions): ManifestPlugin { + if (!options.separatePullRequests) { + options.separatePullRequests = false; + } if (typeof options.type === 'object') { const builder = pluginFactories[options.type.type]; if (builder) { diff --git a/src/manifest.ts b/src/manifest.ts index f152cd1c5..13e590704 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -388,6 +388,7 @@ export class Manifest { targetBranch: this.targetBranch, repositoryConfig: this.repositoryConfig, manifestPath: this.manifestPath, + separatePullRequests: this.separatePullRequests, }) ); this.pullRequestOverflowHandler = new FilePullRequestOverflowHandler( diff --git a/test/factories/plugin-factory.ts b/test/factories/plugin-factory.ts index ef78222a4..2b03db4dd 100644 --- a/test/factories/plugin-factory.ts +++ b/test/factories/plugin-factory.ts @@ -54,6 +54,7 @@ describe('PluginFactory', () => { targetBranch: 'target-branch', repositoryConfig, manifestPath: '.manifest.json', + separatePullRequests: false, }); expect(plugin).to.not.be.undefined; }); @@ -66,6 +67,7 @@ describe('PluginFactory', () => { targetBranch: 'target-branch', repositoryConfig, manifestPath: '.manifest.json', + separatePullRequests: false, }) ).to.throw(); }); @@ -80,6 +82,7 @@ describe('PluginFactory', () => { targetBranch: 'target-branch', repositoryConfig, manifestPath: '.manifest.json', + separatePullRequests: false, }); expect(plugin).to.not.be.undefined; expect(plugin).instanceof(LinkedVersions); @@ -94,6 +97,7 @@ describe('PluginFactory', () => { targetBranch: 'target-branch', repositoryConfig, manifestPath: '.manifest.json', + separatePullRequests: false, }); expect(plugin).to.not.be.undefined; expect(plugin).instanceof(GroupPriority); @@ -108,6 +112,7 @@ describe('PluginFactory', () => { targetBranch: 'target-branch', repositoryConfig, manifestPath: '.manifest.json', + separatePullRequests: false, }); expect(plugin).to.not.be.undefined; expect(plugin).instanceof(NodeWorkspace); @@ -151,6 +156,7 @@ describe('PluginFactory', () => { repositoryConfig: {}, targetBranch: 'main', manifestPath: '.manifest.json', + separatePullRequests: false, }; const strategy = await buildPlugin(pluginOptions); expect(strategy).to.be.instanceof(CustomTest); diff --git a/test/plugins/compatibility/separate-pull-requests-workspace.ts b/test/plugins/compatibility/separate-pull-requests-workspace.ts new file mode 100644 index 000000000..666fdbb5f --- /dev/null +++ b/test/plugins/compatibility/separate-pull-requests-workspace.ts @@ -0,0 +1,198 @@ +// Copyright 2024 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 +// +// http://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 {describe, it, afterEach, beforeEach} from 'mocha'; +import * as sinon from 'sinon'; +import {GitHub} from '../../../src/github'; +import {Manifest} from '../../../src/manifest'; +import {Update} from '../../../src/update'; +import { + buildGitHubFileContent, + mockReleases, + mockCommits, + safeSnapshot, + stubFilesFromFixtures, + assertHasUpdates, +} from '../../helpers'; +import {Version} from '../../../src/version'; +import {PackageJson} from '../../../src/updaters/node/package-json'; +import {expect} from 'chai'; +import {Changelog} from '../../../src/updaters/changelog'; + +const sandbox = sinon.createSandbox(); +const fixturesPath = './test/fixtures/plugins/node-workspace'; + +export function buildMockPackageUpdate( + path: string, + fixtureName: string +): Update { + const cachedFileContents = buildGitHubFileContent(fixturesPath, fixtureName); + return { + path, + createIfMissing: false, + cachedFileContents, + updater: new PackageJson({ + version: Version.parse( + JSON.parse(cachedFileContents.parsedContent).version + ), + }), + }; +} + +describe('Plugin compatibility', () => { + let github: GitHub; + beforeEach(async () => { + github = await GitHub.create({ + owner: 'fake-owner', + repo: 'fake-repo', + defaultBranch: 'main', + }); + }); + afterEach(() => { + sandbox.restore(); + }); + describe('separate-pull-requests and workspace plugin', () => { + it('should version bump dependencies together', async () => { + // Scenario: + // - package a,b depends on c + // - package c receives a new feature + // - package a,b version bumps its dependency on c + // - package a and b should both use a minor version bump + // - each package should have its own PR + mockReleases(sandbox, github, [ + { + id: 123456, + sha: 'abc123', + tagName: 'pkgA-v1.0.0', + url: 'https://github.com/fake-owner/fake-repo/releases/tag/pkgA-v1.0.0', + }, + { + id: 654321, + sha: 'abc123', + tagName: 'pkgB-v1.0.0', + url: 'https://github.com/fake-owner/fake-repo/releases/tag/pkgB-v1.0.0', + }, + { + id: 987654, + sha: 'abc123', + tagName: 'pkgC-v1.0.0', + url: 'https://github.com/fake-owner/fake-repo/releases/tag/pkgC-v1.0.0', + }, + ]); + mockCommits(sandbox, github, [ + { + sha: 'aaaaaa', + message: 'feat: some feature', + files: ['packages/node3/foo'], + }, + ]); + stubFilesFromFixtures({ + sandbox, + github, + fixturePath: fixturesPath, + files: [], + flatten: false, + targetBranch: 'main', + inlineFiles: [ + [ + 'package.json', + '{ "name": "root", "version": "2.0.0", "workspaces": ["packages/*"] }', + ], + [ + 'packages/node1/package.json', + '{ "name": "pkgA", "version": "1.0.0", "dependencies": { "pkgC": "workspace:*" } }', + ], + [ + 'packages/node2/package.json', + '{ "name": "pkgB", "version": "1.0.0", "dependencies": { "pkgC": "workspace:*" } }', + ], + [ + 'packages/node3/package.json', + '{ "name": "pkgC", "version": "1.0.0" }', + ], + ], + }); + sandbox + .stub(github, 'findFilesByGlobAndRef') + .withArgs('packages/node1', 'main') + .resolves(['packages/node1']) + .withArgs('packages/node2', 'main') + .resolves(['packages/node2']) + .withArgs('packages/node3', 'main') + .resolves(['packages/node3']); + const manifest = new Manifest( + github, + 'main', + { + 'packages/node1': { + releaseType: 'node', + component: 'pkgA', + }, + 'packages/node2': { + releaseType: 'node', + component: 'pkgB', + }, + 'packages/node3': { + releaseType: 'node', + component: 'pkgC', + }, + }, + { + 'packages/node1': Version.parse('1.0.0'), + 'packages/node2': Version.parse('1.0.0'), + 'packages/node3': Version.parse('1.0.0'), + }, + { + plugins: [{type: 'node-workspace'}], + separatePullRequests: true, + } + ); + const pullRequests = await manifest.buildPullRequests(); + expect(pullRequests).lengthOf(3); + + const pullRequest1 = pullRequests[0]; + safeSnapshot(pullRequest1.body.toString()); + const updaterA = ( + assertHasUpdates( + pullRequest1.updates, + 'packages/node1/CHANGELOG.md', + Changelog + ) as Update + ).updater as Changelog; + expect(updaterA.version.toString()).to.eql('1.0.1'); + + const pullRequest2 = pullRequests[1]; + safeSnapshot(pullRequest2.body.toString()); + const updaterB = ( + assertHasUpdates( + pullRequest2.updates, + 'packages/node2/CHANGELOG.md', + Changelog + ) as Update + ).updater as Changelog; + expect(updaterB.version.toString()).to.eql('1.0.1'); + + const pullRequest3 = pullRequests[2]; + safeSnapshot(pullRequest3.body.toString()); + const updaterC = ( + assertHasUpdates( + pullRequest3.updates, + 'packages/node3/CHANGELOG.md', + Changelog + ) as Update + ).updater as Changelog; + expect(updaterC.version.toString()).to.eql('1.1.0'); + }); + }); +});