From a5209adbdd0a2a30f5705b42321d0c1d3da2e0b2 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 09:53:42 +0900 Subject: [PATCH 1/8] fix merging behaviour in plugin --- src/factories/plugin-factory.ts | 21 +++++++++++++++++++++ src/manifest.ts | 7 +++++++ test/factories/plugin-factory.ts | 6 ++++++ 3 files changed, 34 insertions(+) diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index 242346853..b42b80a5e 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -18,6 +18,7 @@ import { RepositoryConfig, SentenceCasePluginConfig, GroupPriorityPluginConfig, + hasMergeTypePlugin, } from '../manifest'; import {GitHub} from '../github'; import {ManifestPlugin} from '../plugin'; @@ -38,6 +39,7 @@ export interface PluginFactoryOptions { targetBranch: string; repositoryConfig: RepositoryConfig; manifestPath: string; + separatePullRequests: boolean; // node options alwaysLinkLocal?: boolean; @@ -52,6 +54,21 @@ export interface PluginFactoryOptions { export type PluginBuilder = (options: PluginFactoryOptions) => ManifestPlugin; +function merge(options: PluginFactoryOptions): boolean | undefined { + if (hasMergeTypePlugin(options.type)) { + if ( + typeof options.type.merge === 'undefined' && + // NOTE: linked-versions had already have a different behavior when this code wrote + options.type.type !== 'linked-versions' + ) { + return !options.separatePullRequests; + } + return options.type.merge; + } + // return undefined due to relying on the default behavior of the plugin constructor + return undefined; +} + const pluginFactories: Record = { 'linked-versions': options => new LinkedVersions( @@ -63,6 +80,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: merge(options), } ), 'cargo-workspace': options => @@ -73,6 +91,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: merge(options), } ), 'node-workspace': options => @@ -83,6 +102,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: merge(options), } ), 'maven-workspace': options => @@ -93,6 +113,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), + merge: merge(options), } ), 'sentence-case': options => diff --git a/src/manifest.ts b/src/manifest.ts index 10ac85748..3573e0a69 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -246,6 +246,12 @@ export type PluginType = | WorkspacePluginConfig | NodeWorkspacePluginConfig; +export function hasMergeTypePlugin( + type: PluginType +): type is LinkedVersionPluginConfig | WorkspacePluginConfig { + return typeof type === 'object'; +} + /** * This is the schema of the manifest config json */ @@ -387,6 +393,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); From f6082f3eb5ee818e3bf66a6b4dbec951b14b4ae4 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 09:53:56 +0900 Subject: [PATCH 2/8] add test --- .../separate-pull-requests-workspace.js | 51 +++++ .../separate-pull-requests-workspace.ts | 198 ++++++++++++++++++ 2 files changed, 249 insertions(+) create mode 100644 __snapshots__/separate-pull-requests-workspace.js create mode 100644 test/plugins/compatibility/separate-pull-requests-workspace.ts 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/test/plugins/compatibility/separate-pull-requests-workspace.ts b/test/plugins/compatibility/separate-pull-requests-workspace.ts new file mode 100644 index 000000000..4b2963087 --- /dev/null +++ b/test/plugins/compatibility/separate-pull-requests-workspace.ts @@ -0,0 +1,198 @@ +// Copyright 2022 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'); + }); + }); +}); From a944b75ccc487045440dca514200545a28415f06 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 09:54:03 +0900 Subject: [PATCH 3/8] remove unused test snapshot --- __snapshots__/php-manifest.js | 59 ----------------------------------- 1 file changed, 59 deletions(-) delete mode 100644 __snapshots__/php-manifest.js 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" - } -} -` From b2d4b10ceeafdb3d79cb66cc8cf986296b26b369 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 09:59:12 +0900 Subject: [PATCH 4/8] update licnese header --- test/plugins/compatibility/separate-pull-requests-workspace.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/plugins/compatibility/separate-pull-requests-workspace.ts b/test/plugins/compatibility/separate-pull-requests-workspace.ts index 4b2963087..666fdbb5f 100644 --- a/test/plugins/compatibility/separate-pull-requests-workspace.ts +++ b/test/plugins/compatibility/separate-pull-requests-workspace.ts @@ -1,4 +1,4 @@ -// Copyright 2022 Google LLC +// 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. From 8bfda1647845e97708cb0080bcf080ded9f08bb4 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 14:18:56 +0900 Subject: [PATCH 5/8] considering plugin schema carefully --- src/factories/plugin-factory.ts | 20 ++++++++++++-------- src/manifest.ts | 6 ------ 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index b42b80a5e..dceb74055 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -18,7 +18,7 @@ import { RepositoryConfig, SentenceCasePluginConfig, GroupPriorityPluginConfig, - hasMergeTypePlugin, + WorkspacePluginConfig, } from '../manifest'; import {GitHub} from '../github'; import {ManifestPlugin} from '../plugin'; @@ -55,15 +55,19 @@ export interface PluginFactoryOptions { export type PluginBuilder = (options: PluginFactoryOptions) => ManifestPlugin; function merge(options: PluginFactoryOptions): boolean | undefined { - if (hasMergeTypePlugin(options.type)) { - if ( - typeof options.type.merge === 'undefined' && - // NOTE: linked-versions had already have a different behavior when this code wrote - options.type.type !== 'linked-versions' - ) { + // NOTE: linked-versions had already have a different behavior when this code wrote + // see test/plugins/compatibility/linked-versions-workspace.ts + if (typeof options.type === 'string' && options.type !== 'linked-versions') { + return !options.separatePullRequests; + } + if (typeof options.type !== 'string') { + const type = options.type as + | LinkedVersionPluginConfig + | WorkspacePluginConfig; + if (typeof type.merge === 'undefined' && type.type !== 'linked-versions') { return !options.separatePullRequests; } - return options.type.merge; + return type.merge; } // return undefined due to relying on the default behavior of the plugin constructor return undefined; diff --git a/src/manifest.ts b/src/manifest.ts index 3573e0a69..95d067a91 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -246,12 +246,6 @@ export type PluginType = | WorkspacePluginConfig | NodeWorkspacePluginConfig; -export function hasMergeTypePlugin( - type: PluginType -): type is LinkedVersionPluginConfig | WorkspacePluginConfig { - return typeof type === 'object'; -} - /** * This is the schema of the manifest config json */ From f447a636098c794c97831f1f5384a6a749dbe0f3 Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Wed, 12 Jun 2024 18:47:55 +0900 Subject: [PATCH 6/8] add unit test --- src/factories/plugin-factory.ts | 48 +++++++++++----------- test/factories/plugin-factory.ts | 69 ++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 23 deletions(-) diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index dceb74055..0cc9076e7 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -54,25 +54,6 @@ export interface PluginFactoryOptions { export type PluginBuilder = (options: PluginFactoryOptions) => ManifestPlugin; -function merge(options: PluginFactoryOptions): boolean | undefined { - // NOTE: linked-versions had already have a different behavior when this code wrote - // see test/plugins/compatibility/linked-versions-workspace.ts - if (typeof options.type === 'string' && options.type !== 'linked-versions') { - return !options.separatePullRequests; - } - if (typeof options.type !== 'string') { - const type = options.type as - | LinkedVersionPluginConfig - | WorkspacePluginConfig; - if (typeof type.merge === 'undefined' && type.type !== 'linked-versions') { - return !options.separatePullRequests; - } - return type.merge; - } - // return undefined due to relying on the default behavior of the plugin constructor - return undefined; -} - const pluginFactories: Record = { 'linked-versions': options => new LinkedVersions( @@ -84,7 +65,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: merge(options), + merge: determineMerge(options), } ), 'cargo-workspace': options => @@ -95,7 +76,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: merge(options), + merge: determineMerge(options), } ), 'node-workspace': options => @@ -106,7 +87,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: merge(options), + merge: determineMerge(options), } ), 'maven-workspace': options => @@ -117,7 +98,7 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: merge(options), + merge: determineMerge(options), } ), 'sentence-case': options => @@ -174,3 +155,24 @@ export function unregisterPlugin(name: string) { export function getPluginTypes(): readonly VersioningStrategyType[] { return Object.keys(pluginFactories).sort(); } + +export function determineMerge( + options: PluginFactoryOptions +): boolean | undefined { + // NOTE: linked-versions had already have a different behavior when this code wrote + // see test/plugins/compatibility/linked-versions-workspace.ts + if (typeof options.type === 'string' && options.type !== 'linked-versions') { + return !options.separatePullRequests; + } + if (typeof options.type !== 'string') { + const type = options.type as + | LinkedVersionPluginConfig + | WorkspacePluginConfig; + if (typeof type.merge === 'undefined' && type.type !== 'linked-versions') { + return !options.separatePullRequests; + } + return type.merge; + } + // return undefined due to relying on the default behavior of the plugin constructor + return undefined; +} diff --git a/test/factories/plugin-factory.ts b/test/factories/plugin-factory.ts index 2b03db4dd..d104dea5e 100644 --- a/test/factories/plugin-factory.ts +++ b/test/factories/plugin-factory.ts @@ -19,6 +19,7 @@ import { getPluginTypes, registerPlugin, unregisterPlugin, + determineMerge, } from '../../src/factories/plugin-factory'; import {expect} from 'chai'; import {LinkedVersions} from '../../src/plugins/linked-versions'; @@ -176,4 +177,72 @@ describe('PluginFactory', () => { expect(allTypes).to.contain(pluginType); }); }); + describe('determineMerge', () => { + const pluginOptions = { + github, + repositoryConfig: {}, + targetBranch: 'main', + manifestPath: '.manifest.json', + }; + it('should return false', () => { + const separatePullRequests = true; + + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests, + type: 'node-workspace', + }) + ).to.be.false; + + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests, + type: { + type: 'maven-workspace', + }, + }) + ).to.be.false; + }); + it('should return true', () => { + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests: false, + type: 'cargo-workspace', + }) + ).to.be.true; + + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests: true, + type: { + type: 'node-workspace', + merge: true, + }, + }) + ).to.be.true; + }); + it('should return undefined', () => { + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests: true, + type: 'linked-versions', + }) + ).to.be.undefined; + + expect( + determineMerge({ + ...pluginOptions, + separatePullRequests: true, + type: { + type: 'linked-versions', + }, + }) + ).to.be.undefined; + }); + }); }); From eb0b59c3b41709fc8a6d36b56299b86863cbec7b Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Thu, 19 Sep 2024 12:37:06 +0900 Subject: [PATCH 7/8] remove `determineMerge` and set default value directly --- src/factories/plugin-factory.ts | 37 +++++------------ test/factories/plugin-factory.ts | 69 -------------------------------- 2 files changed, 11 insertions(+), 95 deletions(-) diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index 0cc9076e7..69b2d9c92 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -18,7 +18,6 @@ import { RepositoryConfig, SentenceCasePluginConfig, GroupPriorityPluginConfig, - WorkspacePluginConfig, } from '../manifest'; import {GitHub} from '../github'; import {ManifestPlugin} from '../plugin'; @@ -56,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, @@ -65,7 +66,6 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: determineMerge(options), } ), 'cargo-workspace': options => @@ -76,7 +76,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: determineMerge(options), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'node-workspace': options => @@ -87,7 +89,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: determineMerge(options), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'maven-workspace': options => @@ -98,7 +102,9 @@ const pluginFactories: Record = { { ...options, ...(options.type as WorkspacePluginOptions), - merge: determineMerge(options), + merge: + (options.type as WorkspacePluginOptions).merge ?? + !options.separatePullRequests, } ), 'sentence-case': options => @@ -155,24 +161,3 @@ export function unregisterPlugin(name: string) { export function getPluginTypes(): readonly VersioningStrategyType[] { return Object.keys(pluginFactories).sort(); } - -export function determineMerge( - options: PluginFactoryOptions -): boolean | undefined { - // NOTE: linked-versions had already have a different behavior when this code wrote - // see test/plugins/compatibility/linked-versions-workspace.ts - if (typeof options.type === 'string' && options.type !== 'linked-versions') { - return !options.separatePullRequests; - } - if (typeof options.type !== 'string') { - const type = options.type as - | LinkedVersionPluginConfig - | WorkspacePluginConfig; - if (typeof type.merge === 'undefined' && type.type !== 'linked-versions') { - return !options.separatePullRequests; - } - return type.merge; - } - // return undefined due to relying on the default behavior of the plugin constructor - return undefined; -} diff --git a/test/factories/plugin-factory.ts b/test/factories/plugin-factory.ts index d104dea5e..2b03db4dd 100644 --- a/test/factories/plugin-factory.ts +++ b/test/factories/plugin-factory.ts @@ -19,7 +19,6 @@ import { getPluginTypes, registerPlugin, unregisterPlugin, - determineMerge, } from '../../src/factories/plugin-factory'; import {expect} from 'chai'; import {LinkedVersions} from '../../src/plugins/linked-versions'; @@ -177,72 +176,4 @@ describe('PluginFactory', () => { expect(allTypes).to.contain(pluginType); }); }); - describe('determineMerge', () => { - const pluginOptions = { - github, - repositoryConfig: {}, - targetBranch: 'main', - manifestPath: '.manifest.json', - }; - it('should return false', () => { - const separatePullRequests = true; - - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests, - type: 'node-workspace', - }) - ).to.be.false; - - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests, - type: { - type: 'maven-workspace', - }, - }) - ).to.be.false; - }); - it('should return true', () => { - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests: false, - type: 'cargo-workspace', - }) - ).to.be.true; - - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests: true, - type: { - type: 'node-workspace', - merge: true, - }, - }) - ).to.be.true; - }); - it('should return undefined', () => { - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests: true, - type: 'linked-versions', - }) - ).to.be.undefined; - - expect( - determineMerge({ - ...pluginOptions, - separatePullRequests: true, - type: { - type: 'linked-versions', - }, - }) - ).to.be.undefined; - }); - }); }); From 90e796f8abbae28de5ee308bc09899ee3d517a4a Mon Sep 17 00:00:00 2001 From: Hajime-san Date: Thu, 19 Sep 2024 12:47:40 +0900 Subject: [PATCH 8/8] set default value for `separatePullRequests` in `buildPlugin` --- src/factories/plugin-factory.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/factories/plugin-factory.ts b/src/factories/plugin-factory.ts index 69b2d9c92..5f2756e51 100644 --- a/src/factories/plugin-factory.ts +++ b/src/factories/plugin-factory.ts @@ -38,7 +38,7 @@ export interface PluginFactoryOptions { targetBranch: string; repositoryConfig: RepositoryConfig; manifestPath: string; - separatePullRequests: boolean; + separatePullRequests?: boolean; // node options alwaysLinkLocal?: boolean; @@ -124,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) {