Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make workspace plugin with separate-pull-request fine #2310

Merged
59 changes: 0 additions & 59 deletions __snapshots__/php-manifest.js

This file was deleted.

51 changes: 51 additions & 0 deletions __snapshots__/separate-pull-requests-workspace.js
Original file line number Diff line number Diff line change
@@ -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).
`
27 changes: 27 additions & 0 deletions src/factories/plugin-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
RepositoryConfig,
SentenceCasePluginConfig,
GroupPriorityPluginConfig,
WorkspacePluginConfig,
} from '../manifest';
import {GitHub} from '../github';
import {ManifestPlugin} from '../plugin';
Expand All @@ -38,6 +39,7 @@ export interface PluginFactoryOptions {
targetBranch: string;
repositoryConfig: RepositoryConfig;
manifestPath: string;
separatePullRequests: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be optional and have a default value. The ability to register plugins and pluginFactories is part of the public API (PluginFactoryOptions). It's unlikely that someone is providing their own PluginFactoryOptions instance, but a default wouldn't hurt.


// node options
alwaysLinkLocal?: boolean;
Expand All @@ -63,6 +65,7 @@ const pluginFactories: Record<string, PluginBuilder> = {
{
...options,
...(options.type as WorkspacePluginOptions),
merge: determineMerge(options),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will override the merge option provided by the user (WorkspacePluginOptions has an optional merge).

Also, it seems weird for determineMerge to be checking to see which type of plugin is being configured. determineMerge is already called in a block chosen based on the plugin type. We should just explicitly set the default in the plugin factory for each type.

}
),
'cargo-workspace': options =>
Expand All @@ -73,6 +76,7 @@ const pluginFactories: Record<string, PluginBuilder> = {
{
...options,
...(options.type as WorkspacePluginOptions),
merge: determineMerge(options),
}
),
'node-workspace': options =>
Expand All @@ -83,6 +87,7 @@ const pluginFactories: Record<string, PluginBuilder> = {
{
...options,
...(options.type as WorkspacePluginOptions),
merge: determineMerge(options),
}
),
'maven-workspace': options =>
Expand All @@ -93,6 +98,7 @@ const pluginFactories: Record<string, PluginBuilder> = {
{
...options,
...(options.type as WorkspacePluginOptions),
merge: determineMerge(options),
}
),
'sentence-case': options =>
Expand Down Expand Up @@ -149,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;
}
1 change: 1 addition & 0 deletions src/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ export class Manifest {
targetBranch: this.targetBranch,
repositoryConfig: this.repositoryConfig,
manifestPath: this.manifestPath,
separatePullRequests: this.separatePullRequests,
})
);
this.pullRequestOverflowHandler = new FilePullRequestOverflowHandler(
Expand Down
75 changes: 75 additions & 0 deletions test/factories/plugin-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -54,6 +55,7 @@ describe('PluginFactory', () => {
targetBranch: 'target-branch',
repositoryConfig,
manifestPath: '.manifest.json',
separatePullRequests: false,
});
expect(plugin).to.not.be.undefined;
});
Expand All @@ -66,6 +68,7 @@ describe('PluginFactory', () => {
targetBranch: 'target-branch',
repositoryConfig,
manifestPath: '.manifest.json',
separatePullRequests: false,
})
).to.throw();
});
Expand All @@ -80,6 +83,7 @@ describe('PluginFactory', () => {
targetBranch: 'target-branch',
repositoryConfig,
manifestPath: '.manifest.json',
separatePullRequests: false,
});
expect(plugin).to.not.be.undefined;
expect(plugin).instanceof(LinkedVersions);
Expand All @@ -94,6 +98,7 @@ describe('PluginFactory', () => {
targetBranch: 'target-branch',
repositoryConfig,
manifestPath: '.manifest.json',
separatePullRequests: false,
});
expect(plugin).to.not.be.undefined;
expect(plugin).instanceof(GroupPriority);
Expand All @@ -108,6 +113,7 @@ describe('PluginFactory', () => {
targetBranch: 'target-branch',
repositoryConfig,
manifestPath: '.manifest.json',
separatePullRequests: false,
});
expect(plugin).to.not.be.undefined;
expect(plugin).instanceof(NodeWorkspace);
Expand Down Expand Up @@ -151,6 +157,7 @@ describe('PluginFactory', () => {
repositoryConfig: {},
targetBranch: 'main',
manifestPath: '.manifest.json',
separatePullRequests: false,
};
const strategy = await buildPlugin(pluginOptions);
expect(strategy).to.be.instanceof(CustomTest);
Expand All @@ -170,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;
});
});
});
Loading
Loading