From 868e57d1d022ce6f5933dc5aed5f4a0a2232ca57 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Wed, 6 May 2026 00:02:06 -0700 Subject: [PATCH 1/2] Fix behavior of group exclude option --- .vscode/launch.json | 1 + ...-9c18f781-7c73-4b14-adb7-a5fc6930628a.json | 7 + ...-085686e8-22a7-4276-be7f-2335a0f8f8e5.json | 11 ++ docs/concepts/groups.md | 40 +++--- docs/overview/v3-migration.md | 20 ++- .../__functional__/commands/migrate.test.ts | 124 +++++++++++++++++- .../monorepo/getPackageGroups.test.ts | 4 +- .../__tests__/monorepo/isPathIncluded.test.ts | 31 +++-- .../validate/isValidChangelogOptions.test.ts | 8 +- .../beachball/src/changelog/writeChangelog.ts | 5 +- packages/beachball/src/commands/migrate.ts | 53 +++++++- .../src/monorepo/getPackageGroups.ts | 4 +- .../src/monorepo/getScopedPackages.ts | 5 +- .../beachball/src/monorepo/isPathIncluded.ts | 30 ++--- .../beachball/src/types/BeachballOptions.ts | 4 +- .../beachball/src/types/ChangelogOptions.ts | 7 +- .../src/validation/isValidChangelogOptions.ts | 4 +- 17 files changed, 272 insertions(+), 86 deletions(-) create mode 100644 change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json create mode 100644 change/change-085686e8-22a7-4276-be7f-2335a0f8f8e5.json diff --git a/.vscode/launch.json b/.vscode/launch.json index 9580603b8..fddc916f5 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -15,6 +15,7 @@ "args": ["--", "--runInBand", "--watch", "--testTimeout=1000000", "${file}"], "sourceMaps": true, "outputCapture": "std", + // "runtimeVersion": "22", "console": "integratedTerminal" }, { diff --git a/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json b/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json new file mode 100644 index 000000000..2bc1c0e08 --- /dev/null +++ b/change/beachball-9c18f781-7c73-4b14-adb7-a5fc6930628a.json @@ -0,0 +1,7 @@ +{ + "type": "major", + "comment": "Fix behavior of `VersionGroupOptions.exclude` and `ChangelogGroupOptions.exclude`: if a package path **matches** any `exclude` pattern, it's excluded, rather than requiring negation. (To migrate, just remove the leading `!` from your `exclude` patterns.)", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/change-085686e8-22a7-4276-be7f-2335a0f8f8e5.json b/change/change-085686e8-22a7-4276-be7f-2335a0f8f8e5.json new file mode 100644 index 000000000..315315f3f --- /dev/null +++ b/change/change-085686e8-22a7-4276-be7f-2335a0f8f8e5.json @@ -0,0 +1,11 @@ +{ + "changes": [ + { + "type": "major", + "comment": "Remove deprecated `ChangelogGroupOptions.masterPackageName` (use `mainPackageName`)", + "packageName": "beachball", + "email": "elcraig@microsoft.com", + "dependentChangeType": "patch" + } + ] +} \ No newline at end of file diff --git a/docs/concepts/groups.md b/docs/concepts/groups.md index 8cf495362..c2269e074 100644 --- a/docs/concepts/groups.md +++ b/docs/concepts/groups.md @@ -28,25 +28,26 @@ For cases where it's necessary to bump packages together, `beachball` also provi Groups can be added to the [configuration file](../overview/configuration). See the [`VersionGroupOptions` source](https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts) for full details. -| Name | Type | Description | -| ----------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `name` | `string` | Name of the version group | -| `include` | `string \| string[] \| true` | glob pattern(s) for package paths to include (see [notes on globs][1]). If `true`, include all packages except those matching `exclude`. | -| `exclude` | `string \| string[]` | glob pattern(s) for package paths to exclude (see [notes on globs][1]). This currently must use **negated patterns only** (will be fixed in version 3). | -| `disallowedChangeTypes` | `ChangeType[] \| null` | Disallow these change types for the group. | + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `name` | `string` | Name of the version group | +| `include` | `string \| string[] \| true` | glob pattern(s) for package paths to include (see [notes on globs][1]). If `true`, include all packages except those matching `exclude`. | +| `exclude` | `string \| string[]` | glob pattern(s) for package paths to exclude (see [notes on globs][1]). | +| `disallowedChangeTypes` | `ChangeType[] \| null` | Disallow these change types for the group. | Example: -```jsonc +```json { "groups": [ { "name": "group name", "include": ["packages/groupfoo/*"], - "exclude": ["!packages/groupfoo/bar"], - "disallowedChangeTypes": ["major"], - }, - ], + "exclude": ["packages/groupfoo/bar"], + "disallowedChangeTypes": ["major"] + } + ] } ``` @@ -58,12 +59,13 @@ If you only want to publish or record changes for certain packages, you should u To show changes for multiple packages in one change file, use the `changelog.groups` option. See the [`ChangelogGroupOptions` source](https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts) for full details. -| Name | Type | Description | -| ------------------- | ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `masterPackageName` | `string` | The main package which a group of changes bubbles up to. | -| `include` | `string \| string[] \| true` | glob pattern(s) for package paths to include (see [notes on globs][1]). If `true`, include all packages except those matching `exclude`. | -| `exclude` | `string \| string[]` | glob pattern(s) for package paths to exclude (see [notes on globs][1]). This currently must use **negated patterns only** (will be fixed in version 3). | -| `changelogPath` | `string` | Put the grouped changelog file under this directory. Can be relative to the root, or absolute. | + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `mainPackageName` | `string` | The main package which a group of changes bubbles up to. | +| `include` | `string \| string[] \| true` | glob pattern(s) for package paths to include (see [notes on globs][1]). If `true`, include all packages except those matching `exclude`. | +| `exclude` | `string \| string[]` | glob pattern(s) for package paths to exclude (see [notes on globs][1]). | +| `changelogPath` | `string` | Put the grouped changelog file under this directory. Can be relative to the root, or absolute. | In this example, changelogs for all packages under `packages/*` (except `packages/baz`) are written to a `CHANGELOG.md` at the repo root (`.`), with `foo` as the master package. (To replace `foo`'s usual changelog with a grouped one, you'd specify `changelogPath` as the path to `foo` instead, e.g. `packages/foo`.) @@ -72,10 +74,10 @@ In this example, changelogs for all packages under `packages/*` (except `package "changelog": { "groups": [ { - "masterPackageName": "foo", + "mainPackageName": "foo", "changelogPath": ".", "include": ["packages/*"], - "exclude": ["!packages/baz"] + "exclude": ["packages/baz"] } ] } diff --git a/docs/overview/v3-migration.md b/docs/overview/v3-migration.md index a6b230f38..673320cf2 100644 --- a/docs/overview/v3-migration.md +++ b/docs/overview/v3-migration.md @@ -8,13 +8,9 @@ category: doc This page describes how to migrate from beachball v2 to v3. -## Changelog - -For the full list of changes between v2 and v3, see the [beachball CHANGELOG.md](https://github.com/microsoft/beachball/blob/main/packages/beachball/CHANGELOG.md). - ## Running the migrate command -beachball v3 includes a `migrate` command that checks your config and logs any updates needed for v3: +beachball v3 includes a `migrate` command that **checks your config and logs any updates needed** for v3: ```bash beachball migrate @@ -26,4 +22,16 @@ If your config is already compatible, you will see: No config updates are needed for v3. ``` -Otherwise, the command will list specific config updates that are needed. The command doesn't attempt to make updates directly due to the variety of locations and file types where the config can be specified. +Otherwise, the command will list specific config updates that are needed. The command does NOT attempt to make updates directly due to the variety of locations and file types where the config can be specified. + +## Breaking changes + +For the full list of changes between v2 and v3, see the [beachball CHANGELOG.md](https://github.com/microsoft/beachball/blob/main/packages/beachball/CHANGELOG.md). + + + +### Fix group `exclude` negation behavior + +Remove the requirement for `groups[*].exclude` and `changelog.groups[*].exclude` patterns to be negated (leading `!`). + +To migrate, simply remove the leading `!` from all `exclude` patterns. diff --git a/packages/beachball/src/__functional__/commands/migrate.test.ts b/packages/beachball/src/__functional__/commands/migrate.test.ts index 48b74febb..304032e22 100644 --- a/packages/beachball/src/__functional__/commands/migrate.test.ts +++ b/packages/beachball/src/__functional__/commands/migrate.test.ts @@ -2,12 +2,23 @@ import { describe, expect, it, beforeAll, afterAll } from '@jest/globals'; import { initMockLogs } from '../../__fixtures__/mockLogs'; import { RepositoryFactory } from '../../__fixtures__/repositoryFactory'; import { migrate } from '../../commands/migrate'; +import type { BeachballOptions } from '../../types/BeachballOptions'; +import { getDefaultOptions } from '../../options/getDefaultOptions'; +import { BeachballError } from '../../types/BeachballError'; +import type { ChangelogGroupOptions } from '../../types/ChangelogOptions'; describe('migrate command', () => { const logs = initMockLogs(); let repositoryFactory: RepositoryFactory; + function migrateWrapper(options: Partial) { + return migrate({ + ...getDefaultOptions(), + ...options, + }); + } + beforeAll(() => { repositoryFactory = new RepositoryFactory('single'); }); @@ -17,8 +28,115 @@ describe('migrate command', () => { }); it('logs a success message when no config updates are needed', () => { - const repo = repositoryFactory.cloneRepository(); - migrate({ path: repo.rootPath }); - expect(logs.getMockLines('log')).toMatchInlineSnapshot(`"No config updates are needed for v3."`); + migrateWrapper({ + groups: [{ name: 'test', include: 'packages/test', exclude: ['packages/foo'], disallowedChangeTypes: null }], + changelog: { + groups: [ + { + mainPackageName: 'test', + include: ['packages/test'], + exclude: ['packages/bar'], + changelogPath: 'packages/test', + }, + ], + }, + }); + expect(logs.getMockLines('log')).toEqual('No config updates are needed for v3.'); + }); + + it('logs an error for negated groups[*].exclude', () => { + const disallowedChangeTypes = null; + expect(() => + migrateWrapper({ + groups: [ + // the group globs here don't need to make sense; just verify it only checks ! at beginning + { name: 'ok', include: true, exclude: 'packages/!(bar)', disallowedChangeTypes }, + { name: 'badstring', include: true, exclude: '!packages/foo', disallowedChangeTypes }, + { + name: 'badarray', + include: true, + exclude: ['packages/bar', '!packages/foo', '!packages/baz'], + disallowedChangeTypes, + }, + ], + }) + ).toThrow(BeachballError); + + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "The following updates are needed for v3: + • \`groups\` + ▪ Group "badstring" + ◦ Remove the leading "!" from these \`exclude\` patterns: + ▫ !packages/foo + ▪ Group "badarray" + ◦ Remove the leading "!" from these \`exclude\` patterns: + ▫ !packages/foo + ▫ !packages/baz" + `); + }); + + it('logs an error for changelog.groups[*].masterPackageName', () => { + expect(() => + migrateWrapper({ + changelog: { + groups: [ + { masterPackageName: 'test1', changelogPath: '', include: true } as unknown as ChangelogGroupOptions, + { mainPackageName: 'test2', changelogPath: '', include: true }, + { masterPackageName: 'test3', changelogPath: '', include: true } as unknown as ChangelogGroupOptions, + ], + }, + }) + ).toThrow(BeachballError); + + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "The following updates are needed for v3: + • \`changelog.groups\` + ▪ Group for package "test1" + ◦ Rename \`masterPackageName\` to \`mainPackageName\` + ▪ Group for package "test3" + ◦ Rename \`masterPackageName\` to \`mainPackageName\`" + `); + }); + + it('logs an error for negated changelog.groups[*].exclude and masterPackageName', () => { + expect(() => + migrateWrapper({ + changelog: { + groups: [ + { + masterPackageName: 'test', + include: true, + exclude: ['!packages/bar', '!packages/baz'], + changelogPath: '', + } as Partial as ChangelogGroupOptions, + { + mainPackageName: 'test2', + include: true, + exclude: '!packages/foo', + changelogPath: '', + }, + { + mainPackageName: 'test3', + include: true, + exclude: 'packages/!(bar)', + changelogPath: '', + }, + ], + }, + }) + ).toThrow(BeachballError); + + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "The following updates are needed for v3: + • \`changelog.groups\` + ▪ Group for package "test" + ◦ Rename \`masterPackageName\` to \`mainPackageName\` + ◦ Remove the leading "!" from these \`exclude\` patterns: + ▫ !packages/bar + ▫ !packages/baz + ▪ Group for package "test2" + ◦ Remove the leading "!" from these \`exclude\` patterns: + ▫ !packages/foo" + `); }); }); diff --git a/packages/beachball/src/__tests__/monorepo/getPackageGroups.test.ts b/packages/beachball/src/__tests__/monorepo/getPackageGroups.test.ts index 0f4ee3cc9..a90607ed7 100644 --- a/packages/beachball/src/__tests__/monorepo/getPackageGroups.test.ts +++ b/packages/beachball/src/__tests__/monorepo/getPackageGroups.test.ts @@ -117,7 +117,7 @@ describe('getPackageGroups', () => { }, repoOptions: { groups: [ - { name: 'group', include: ['packages/*'], exclude: ['!packages/internal'], disallowedChangeTypes: null }, + { name: 'group', include: ['packages/*'], exclude: ['packages/internal'], disallowedChangeTypes: null }, ], }, }); @@ -139,7 +139,7 @@ describe('getPackageGroups', () => { { name: 'group1', include: ['packages/**/*'], - exclude: ['!packages/core/*'], + exclude: ['packages/core/*'], disallowedChangeTypes: null, }, ], diff --git a/packages/beachball/src/__tests__/monorepo/isPathIncluded.test.ts b/packages/beachball/src/__tests__/monorepo/isPathIncluded.test.ts index 4dd37fba4..1e504644e 100644 --- a/packages/beachball/src/__tests__/monorepo/isPathIncluded.test.ts +++ b/packages/beachball/src/__tests__/monorepo/isPathIncluded.test.ts @@ -3,34 +3,43 @@ import { isPathIncluded } from '../../monorepo/isPathIncluded'; describe('isPathIncluded', () => { it('returns true if path is included (single include path)', () => { - expect(isPathIncluded('packages/a', 'packages/*')).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*' })).toBeTruthy(); }); - it('returns false if path is excluded (single exclude path)', () => { - expect(isPathIncluded('packages/a', 'packages/*', '!packages/a')).toBeFalsy(); + it('returns false if path is not included, with single include path', () => { + expect(isPathIncluded({ relativePath: 'stuff/b', include: 'packages/*' })).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/b', include: 'packages/!(b)' })).toBeFalsy(); }); - it('returns true if path is included (multiple include paths)', () => { - expect(isPathIncluded('packages/a', ['packages/b', 'packages/a'], ['!packages/b'])).toBeTruthy(); + it('returns false if path is excluded, with single exclude path', () => { + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*', exclude: 'packages/a' })).toBeFalsy(); }); - it('returns false if path is excluded (multiple exclude paths)', () => { - expect(isPathIncluded('packages/a', ['packages/*'], ['!packages/a', '!packages/b'])).toBeFalsy(); + it('returns true if path is included, with multiple include paths', () => { + expect( + isPathIncluded({ relativePath: 'packages/a', include: ['packages/b', 'packages/a'], exclude: ['packages/b'] }) + ).toBeTruthy(); + }); + + it('returns false if path is excluded, with multiple exclude paths', () => { + expect( + isPathIncluded({ relativePath: 'packages/a', include: ['packages/*'], exclude: ['packages/a'] }) + ).toBeFalsy(); }); it('returns true if include is true (no exclude paths)', () => { - expect(isPathIncluded('packages/a', true)).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: true })).toBeTruthy(); }); it('returns false if include is true and path is excluded', () => { - expect(isPathIncluded('packages/a', true, '!packages/a')).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: true, exclude: 'packages/a' })).toBeFalsy(); }); it('returns false if include path is empty', () => { - expect(isPathIncluded('packages/a', '')).toBeFalsy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: '' })).toBeFalsy(); }); it('ignores empty exclude path array', () => { - expect(isPathIncluded('packages/a', 'packages/*', [])).toBeTruthy(); + expect(isPathIncluded({ relativePath: 'packages/a', include: 'packages/*', exclude: [] })).toBeTruthy(); }); }); diff --git a/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts b/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts index 2873ca445..f2b373ad1 100644 --- a/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts +++ b/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts @@ -12,7 +12,7 @@ describe('isValidChangelogOptions', () => { expect(logs.mocks.error).not.toHaveBeenCalled(); }); - it('returns true when groups are valid with masterPackageName', () => { + it('returns false for groups with masterPackageName', () => { const options = { groups: [ { @@ -21,9 +21,9 @@ describe('isValidChangelogOptions', () => { include: ['pkg1', 'pkg2'], }, ], - } as ChangelogOptions; - expect(isValidChangelogOptions(options)).toBe(true); - expect(logs.mocks.error).not.toHaveBeenCalled(); + } as unknown as ChangelogOptions; + expect(isValidChangelogOptions(options)).toBe(false); + expect(logs.mocks.error).toHaveBeenCalled(); }); it('returns true when groups are valid with mainPackageName', () => { diff --git a/packages/beachball/src/changelog/writeChangelog.ts b/packages/beachball/src/changelog/writeChangelog.ts index 018a16d81..31363cc9c 100644 --- a/packages/beachball/src/changelog/writeChangelog.ts +++ b/packages/beachball/src/changelog/writeChangelog.ts @@ -69,8 +69,7 @@ async function writeGroupedChangelog( // Validate groups and initialize groupedChangelogs for (const group of changelogGroups) { const { changelogAbsDir } = group; - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion, beachball/no-deprecated - const mainPackageName = group.mainPackageName ?? group.masterPackageName!; + const mainPackageName = group.mainPackageName; const mainPackage = packageInfos[mainPackageName]; if (!mainPackage) { console.warn(`main package ${mainPackageName} does not exist.`); @@ -89,7 +88,7 @@ async function writeGroupedChangelog( const relativePath = path.relative(options.path, packagePath); for (const group of changelogGroups) { - const isInGroup = isPathIncluded(relativePath, group.include, group.exclude); + const isInGroup = isPathIncluded({ relativePath, include: group.include, exclude: group.exclude }); if (isInGroup) { groupedChangelogs[group.changelogAbsDir].changelogs.push(changelogs[pkg]); } diff --git a/packages/beachball/src/commands/migrate.ts b/packages/beachball/src/commands/migrate.ts index 4745a6f77..a8a45406a 100644 --- a/packages/beachball/src/commands/migrate.ts +++ b/packages/beachball/src/commands/migrate.ts @@ -1,3 +1,5 @@ +import { bulletedList, type BulletList } from '../logging/bulletedList'; +import { BeachballError } from '../types/BeachballError'; import type { BeachballOptions } from '../types/BeachballOptions'; /** @@ -6,17 +8,54 @@ import type { BeachballOptions } from '../types/BeachballOptions'; * Checks the config for any settings that need to be updated for v3 and logs them to the console. * If no updates are needed, a success message is printed. */ -export function migrate(_options: Pick): void { - const updates: string[] = []; +export function migrate(options: BeachballOptions): void { + const updates: BulletList = []; - // (Future migration checks will be added here) + const groupUpdates: BulletList = []; + for (const group of options.groups ?? []) { + const exclude = typeof group.exclude === 'string' ? [group.exclude] : group.exclude || []; + const negatedExclude = exclude.filter(p => p.startsWith('!')); + if (negatedExclude.length) { + groupUpdates.push(`Group "${group.name}"`, [ + 'Remove the leading "!" from these `exclude` patterns:', + negatedExclude, + ]); + } + } + if (groupUpdates.length) { + updates.push('`groups`', groupUpdates); + } + + const changelogGroupUpdates: BulletList = []; + for (const group of options.changelog?.groups ?? []) { + const thisGroupUpdates: BulletList = []; + let mainPkg = group.mainPackageName as string | undefined; + if (!mainPkg) { + mainPkg = (group as { masterPackageName?: string }).masterPackageName; + if (mainPkg) { + thisGroupUpdates.push('Rename `masterPackageName` to `mainPackageName`'); + } + } + + const exclude = typeof group.exclude === 'string' ? [group.exclude] : group.exclude || []; + const negatedExclude = exclude.filter(p => p.startsWith('!')); + if (negatedExclude.length) { + thisGroupUpdates.push(`Remove the leading "!" from these \`exclude\` patterns:`, negatedExclude); + } + + if (thisGroupUpdates.length) { + changelogGroupUpdates.push(`Group for package "${mainPkg ?? '(missing)'}"`, thisGroupUpdates); + } + } + if (changelogGroupUpdates.length) { + updates.push('`changelog.groups`', changelogGroupUpdates); + } if (updates.length === 0) { console.log('No config updates are needed for v3.'); } else { - console.log('The following updates are needed for v3:'); - for (const update of updates) { - console.log(` - ${update}`); - } + console.error('The following updates are needed for v3:'); + console.error(bulletedList(updates)); + throw new BeachballError('Config updates are needed', { alreadyLogged: true }); } } diff --git a/packages/beachball/src/monorepo/getPackageGroups.ts b/packages/beachball/src/monorepo/getPackageGroups.ts index f146e35ed..dbeecc48a 100644 --- a/packages/beachball/src/monorepo/getPackageGroups.ts +++ b/packages/beachball/src/monorepo/getPackageGroups.ts @@ -25,7 +25,9 @@ export function getPackageGroups( const packagePath = path.dirname(info.packageJsonPath); const relativePath = path.relative(root, packagePath); - const groupsForPkg = groups.filter(group => isPathIncluded(relativePath, group.include, group.exclude)); + const groupsForPkg = groups.filter(group => + isPathIncluded({ relativePath, include: group.include, exclude: group.exclude }) + ); if (groupsForPkg.length > 1) { // Keep going after this error to ensure we report all errors errorPackages[pkgName] = groupsForPkg; diff --git a/packages/beachball/src/monorepo/getScopedPackages.ts b/packages/beachball/src/monorepo/getScopedPackages.ts index 75814779e..085a05e8b 100644 --- a/packages/beachball/src/monorepo/getScopedPackages.ts +++ b/packages/beachball/src/monorepo/getScopedPackages.ts @@ -20,13 +20,14 @@ export function getScopedPackages( // If there were no include scopes, include all paths by default includeScopes = includeScopes.length ? includeScopes : true; - const excludeScopes = scope.filter(s => s.startsWith('!')); + const excludeScopes = scope.filter(s => s.startsWith('!')).map(s => s.slice(1)); result = new Set( packageNames.filter(pkgName => { const packagePath = path.dirname(packageInfos[pkgName].packageJsonPath); + const relativePath = path.relative(cwd, packagePath); - return isPathIncluded(path.relative(cwd, packagePath), includeScopes, excludeScopes); + return isPathIncluded({ relativePath, include: includeScopes, exclude: excludeScopes }); }) ); } else { diff --git a/packages/beachball/src/monorepo/isPathIncluded.ts b/packages/beachball/src/monorepo/isPathIncluded.ts index c7e5a7c67..4a36569a1 100644 --- a/packages/beachball/src/monorepo/isPathIncluded.ts +++ b/packages/beachball/src/monorepo/isPathIncluded.ts @@ -1,18 +1,18 @@ import minimatch from 'minimatch'; /** - * Check if a relative path should be included given include and exclude patterns using minimatch. - * @param relativePath Relative path to check. - * @param include Include pattern(s). If `true`, include all paths except those excluded. - * @param exclude Exclude pattern(s). Currently these must be **negated** patterns: - * e.g. if you want to exclude `packages/foo`, you must specify `exclude` as `!packages/foo`. - * (This will be fixed in a future major version.) + * Check if a relative package path should be included given include and exclude patterns using minimatch. */ -export function isPathIncluded( - relativePath: string, - include: string | string[] | true, - exclude?: string | string[] -): boolean { +export function isPathIncluded(params: { + /** Relative path to the package from the repo root. */ + relativePath: string; + /** Include pattern(s). If `true`, include all paths except those excluded. */ + include: string | string[] | true; + /** Exclude pattern(s). As of v3, these are no longer negated patterns. */ + exclude?: string | string[]; +}): boolean { + const { relativePath, include, exclude } = params; + let shouldInclude: boolean; if (include === true) { shouldInclude = true; @@ -22,14 +22,8 @@ export function isPathIncluded( } if (exclude?.length && shouldInclude) { - // TODO: this is weird/buggy--it assumes that exclude patterns are always negated, - // which intuitively (or comparing to other tools) is not how it should work. - // If this is fixed, updates will be needed in: - // - getScopedPackages() - // - ChangelogGroupOptions - // - VersionGroupOptions const excludePatterns = typeof exclude === 'string' ? [exclude] : exclude; - shouldInclude = excludePatterns.every(pattern => minimatch(relativePath, pattern)); + shouldInclude = !excludePatterns.some(pattern => minimatch(relativePath, pattern)); } return shouldInclude; diff --git a/packages/beachball/src/types/BeachballOptions.ts b/packages/beachball/src/types/BeachballOptions.ts index 7f6f3597c..07506af45 100644 --- a/packages/beachball/src/types/BeachballOptions.ts +++ b/packages/beachball/src/types/BeachballOptions.ts @@ -321,8 +321,8 @@ export interface VersionGroupOptions { * minimatch pattern(s) for package paths to exclude from this group. * Patterns are relative to the repo root and must use forward slashes. * - * Currently this must use **negated patterns only**: e.g. if you want to exclude `packages/foo`, - * you must specify `exclude` as `!packages/foo`. (This will be fixed in a future major version.) + * NOTE: As of v3, you should use non-negated patterns here (the previous bug requiring + * negated patterns has been fixed). */ exclude?: string | string[]; diff --git a/packages/beachball/src/types/ChangelogOptions.ts b/packages/beachball/src/types/ChangelogOptions.ts index 6feafc40b..9ff8a3874 100644 --- a/packages/beachball/src/types/ChangelogOptions.ts +++ b/packages/beachball/src/types/ChangelogOptions.ts @@ -85,8 +85,8 @@ export interface ChangelogGroupOptions { * minimatch pattern(s) for package paths to exclude from this group. * Patterns are relative to the repo root and must use forward slashes. * - * Currently this must use **negated patterns only**: e.g. if you want to exclude `packages/foo`, - * you must specify `exclude` as `!packages/foo`. (This will be fixed in a future major version.) + * NOTE: As of v3, you must use non-negated patterns here (the previous bug requiring + * negated patterns has been fixed). */ exclude?: string | string[]; @@ -101,9 +101,6 @@ export interface ChangelogGroupOptions { * All changes within the group are used to describe changes for the main package. */ mainPackageName: string; - - /** @deprecated Use `mainPackageName` */ - masterPackageName?: string; } /** diff --git a/packages/beachball/src/validation/isValidChangelogOptions.ts b/packages/beachball/src/validation/isValidChangelogOptions.ts index 17373ccda..ff0fa1d62 100644 --- a/packages/beachball/src/validation/isValidChangelogOptions.ts +++ b/packages/beachball/src/validation/isValidChangelogOptions.ts @@ -7,9 +7,7 @@ export function isValidChangelogOptions(options: ChangelogOptions): boolean { return true; } - const badGroups = options.groups.filter( - group => !group.changelogPath || !('masterPackageName' in group || 'mainPackageName' in group) || !group.include - ); + const badGroups = options.groups.filter(group => !group.changelogPath || !group.mainPackageName || !group.include); if (badGroups.length) { console.error( From 77c025eab8325be587c8a84d30c92e5e8862cf28 Mon Sep 17 00:00:00 2001 From: Elizabeth Craig Date: Wed, 6 May 2026 23:32:36 -0700 Subject: [PATCH 2/2] feedback --- docs/cli/options.md | 2 +- docs/concepts/groups.md | 6 +- docs/overview/configuration.md | 8 +-- docs/overview/v3-migration.md | 4 ++ .../validate/isValidChangelogOptions.test.ts | 60 ++++++++++++++----- .../validate/isValidGroupOptions.test.ts | 15 +++++ .../src/validation/isValidChangelogOptions.ts | 38 ++++++++++-- .../src/validation/isValidGroupOptions.ts | 12 ++++ 8 files changed, 118 insertions(+), 27 deletions(-) diff --git a/docs/cli/options.md b/docs/cli/options.md index d48398325..6cb33e1d3 100644 --- a/docs/cli/options.md +++ b/docs/cli/options.md @@ -7,7 +7,7 @@ category: doc # Beachball CLI options -For the latest full list of supported options, see `CliOptions` [in this file](https://github.com/microsoft/beachball/blob/main/src/types/BeachballOptions.ts). +For the latest full list of supported options, see `CliOptions` [in this file](https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/BeachballOptions.ts). **Most options can also be specified in the [configuration file](../overview/configuration)**, which is generally preferable as it's easier to read and maintain. diff --git a/docs/concepts/groups.md b/docs/concepts/groups.md index c2269e074..c55c901d7 100644 --- a/docs/concepts/groups.md +++ b/docs/concepts/groups.md @@ -26,7 +26,7 @@ For cases where it's necessary to bump packages together, `beachball` also provi ### Configuring version groups -Groups can be added to the [configuration file](../overview/configuration). See the [`VersionGroupOptions` source](https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts) for full details. +Groups can be added to the [configuration file](../overview/configuration). See the [`VersionGroupOptions` source](https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/ChangelogOptions.ts) for full details. | Name | Type | Description | @@ -57,7 +57,7 @@ If you only want to publish or record changes for certain packages, you should u ## Grouped changelogs -To show changes for multiple packages in one change file, use the `changelog.groups` option. See the [`ChangelogGroupOptions` source](https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts) for full details. +To show changes for multiple packages in one change file, use the `changelog.groups` option. See the [`ChangelogGroupOptions` source](https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/ChangelogOptions.ts) for full details. | Name | Type | Description | @@ -67,7 +67,7 @@ To show changes for multiple packages in one change file, use the `changelog.gro | `exclude` | `string \| string[]` | glob pattern(s) for package paths to exclude (see [notes on globs][1]). | | `changelogPath` | `string` | Put the grouped changelog file under this directory. Can be relative to the root, or absolute. | -In this example, changelogs for all packages under `packages/*` (except `packages/baz`) are written to a `CHANGELOG.md` at the repo root (`.`), with `foo` as the master package. (To replace `foo`'s usual changelog with a grouped one, you'd specify `changelogPath` as the path to `foo` instead, e.g. `packages/foo`.) +In this example, changelogs for all packages under `packages/*` (except `packages/baz`) are written to a `CHANGELOG.md` at the repo root (`.`), with `foo` as the main package. (To replace `foo`'s usual changelog with a grouped one, you'd specify `changelogPath` as the path to `foo` instead, e.g. `packages/foo`.) ```json { diff --git a/docs/overview/configuration.md b/docs/overview/configuration.md index 71a346653..0eef39074 100644 --- a/docs/overview/configuration.md +++ b/docs/overview/configuration.md @@ -67,7 +67,7 @@ To change the `disallowedChangeTypes` for package `foo`, you could add the follo ## Options -For the latest full list of supported options, see `RepoOptions` [in this file](https://github.com/microsoft/beachball/blob/main/src/types/BeachballOptions.ts). +For the latest full list of supported options, see `RepoOptions` [in this file](https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/BeachballOptions.ts). "Applies to" indicates where the settings can be specified: repo-level config or package-level config. @@ -105,10 +105,10 @@ For the latest full list of supported options, see `RepoOptions` [in this file]( | `tag` | `string` | `'latest'` | repo, package | `dist-tag` for npm when published | | `transform` | [`TransformOptions`][4] | | repo | Transformations for change files | -[1]: https://github.com/microsoft/beachball/blob/main/src/types/ChangeFilePrompt.ts -[2]: https://github.com/microsoft/beachball/blob/main/src/types/ChangelogOptions.ts +[1]: https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/ChangeFilePrompt.ts +[2]: https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/ChangelogOptions.ts [3]: ../concepts/groups#version-groups -[4]: https://github.com/microsoft/beachball/blob/main/src/types/BeachballOptions.ts +[4]: https://github.com/microsoft/beachball/blob/main/packages/beachball/src/types/BeachballOptions.ts [5]: #specifying-the-target-branch-and-remote [6]: #glob-matching diff --git a/docs/overview/v3-migration.md b/docs/overview/v3-migration.md index 673320cf2..262be685c 100644 --- a/docs/overview/v3-migration.md +++ b/docs/overview/v3-migration.md @@ -35,3 +35,7 @@ For the full list of changes between v2 and v3, see the [beachball CHANGELOG.md] Remove the requirement for `groups[*].exclude` and `changelog.groups[*].exclude` patterns to be negated (leading `!`). To migrate, simply remove the leading `!` from all `exclude` patterns. + +### Rename `changelog.groups[*].masterPackageName` to `mainPackageName` + +To migrate, find and replace `masterPackageName` to `mainPackageName`. diff --git a/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts b/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts index f2b373ad1..c05615ae6 100644 --- a/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts +++ b/packages/beachball/src/__tests__/validate/isValidChangelogOptions.test.ts @@ -12,32 +12,35 @@ describe('isValidChangelogOptions', () => { expect(logs.mocks.error).not.toHaveBeenCalled(); }); - it('returns false for groups with masterPackageName', () => { - const options = { + it('returns true when groups are valid', () => { + const options: ChangelogOptions = { groups: [ { changelogPath: 'path/to/changelog', - masterPackageName: 'package-name', + mainPackageName: 'package-name', include: ['pkg1', 'pkg2'], }, ], - } as unknown as ChangelogOptions; - expect(isValidChangelogOptions(options)).toBe(false); - expect(logs.mocks.error).toHaveBeenCalled(); + }; + expect(isValidChangelogOptions(options)).toBe(true); + expect(logs.mocks.error).not.toHaveBeenCalled(); }); - it('returns true when groups are valid with mainPackageName', () => { - const options: ChangelogOptions = { + it('returns false for groups with masterPackageName', () => { + const options = { groups: [ { changelogPath: 'path/to/changelog', - mainPackageName: 'package-name', + masterPackageName: 'package-name', include: ['pkg1', 'pkg2'], }, ], - }; - expect(isValidChangelogOptions(options)).toBe(true); - expect(logs.mocks.error).not.toHaveBeenCalled(); + } as unknown as ChangelogOptions; + expect(isValidChangelogOptions(options)).toBe(false); + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "ERROR: "changelog.groups[*].masterPackageName" is renamed to "mainPackageName" in v3. Invalid groups: + • masterPackageName "package-name"" + `); }); it('returns false when group is missing changelogPath', () => { @@ -50,7 +53,10 @@ describe('isValidChangelogOptions', () => { ], } as ChangelogOptions; expect(isValidChangelogOptions(options)).toBe(false); - expect(logs.mocks.error).toHaveBeenCalled(); + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "ERROR: "changelog.groups" entries must define "changelogPath", "mainPackageName", and "include". Invalid groups: + • { "mainPackageName": "package-name", "include": ["pkg1"] }" + `); }); it('returns false when group is missing mainPackageName and masterPackageName', () => { @@ -91,7 +97,11 @@ describe('isValidChangelogOptions', () => { ], }; expect(isValidChangelogOptions(options)).toBe(false); - expect(logs.mocks.error).toHaveBeenCalled(); + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "ERROR: "changelog.groups" entries must define "changelogPath", "mainPackageName", and "include". Invalid groups: + • { "changelogPath": "path/to/changelog" } + • { "mainPackageName": "package-name" }" + `); }); it('returns false for a mix of valid and invalid groups', () => { @@ -108,6 +118,26 @@ describe('isValidChangelogOptions', () => { ], }; expect(isValidChangelogOptions(options)).toBe(false); - expect(logs.mocks.error).toHaveBeenCalled(); + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "ERROR: "changelog.groups" entries must define "changelogPath", "mainPackageName", and "include". Invalid groups: + • { "changelogPath": "path/to/changelog2" }" + `); + }); + + it('returns false when exclude patterns start with "!"', () => { + const options: ChangelogOptions = { + // these groups don't make sense in combination; just test that the ones with bad exclude patterns are caught + groups: [ + { changelogPath: 'path', mainPackageName: 'pkg', include: true, exclude: ['ok', '!invalid-array'] }, + { changelogPath: 'path2', mainPackageName: 'pkg2', include: true, exclude: '!invalid-string' }, + { changelogPath: 'path3', mainPackageName: 'pkg3', include: true, exclude: ['ok', 'also-ok'] }, + ], + }; + expect(isValidChangelogOptions(options)).toBe(false); + expect(logs.getMockLines('error')).toMatchInlineSnapshot(` + "ERROR: "changelog.groups[*].exclude" patterns must not start with "!" in v3. Found invalid groups: + • mainPackageName "pkg": [ "ok", "!invalid-array" ] + • mainPackageName "pkg2": "!invalid-string"" + `); }); }); diff --git a/packages/beachball/src/__tests__/validate/isValidGroupOptions.test.ts b/packages/beachball/src/__tests__/validate/isValidGroupOptions.test.ts index ae36b55cd..7f96bc0e0 100644 --- a/packages/beachball/src/__tests__/validate/isValidGroupOptions.test.ts +++ b/packages/beachball/src/__tests__/validate/isValidGroupOptions.test.ts @@ -59,6 +59,21 @@ describe('isValidGroupOptions', () => { • { "include": ["pkg1"] }" `); }); + + it('returns false when a group has an exclude pattern starting with "!"', () => { + const groups: VersionGroupOptions[] = [ + { name: 'group1', include: ['pkg1'], exclude: '!badpattern', disallowedChangeTypes: null }, + { name: 'group2', include: ['pkg2'], disallowedChangeTypes: null }, + { name: 'group3', include: ['pkg3'], exclude: ['goodpattern', '!badpattern2'], disallowedChangeTypes: null }, + ]; + expect(isValidGroupOptions(groups)).toBe(false); + expect(logs.mocks.error).toHaveBeenCalledTimes(1); + expect(logs.mocks.error.mock.calls[0].join(' ')).toMatchInlineSnapshot(` + "ERROR: "groups[*].exclude" patterns must not start with "!". Found invalid groups: + • group1: "!badpattern" + • group3: [ "goodpattern", "!badpattern2" ]" + `); + }); }); describe('isValidGroupedPackageOptions', () => { diff --git a/packages/beachball/src/validation/isValidChangelogOptions.ts b/packages/beachball/src/validation/isValidChangelogOptions.ts index ff0fa1d62..88a9dc3ca 100644 --- a/packages/beachball/src/validation/isValidChangelogOptions.ts +++ b/packages/beachball/src/validation/isValidChangelogOptions.ts @@ -7,16 +7,46 @@ export function isValidChangelogOptions(options: ChangelogOptions): boolean { return true; } - const badGroups = options.groups.filter(group => !group.changelogPath || !group.mainPackageName || !group.include); + let isValid = true; + const oldGroups = options.groups.filter(group => 'masterPackageName' in group); + if (oldGroups.length) { + isValid = false; + console.error( + 'ERROR: "changelog.groups[*].masterPackageName" is renamed to "mainPackageName" in v3. ' + + 'Invalid groups:\n' + + bulletedList(oldGroups.map(group => `masterPackageName "${group.masterPackageName as string}"`)) + ); + } + + const badGroups = options.groups.filter( + group => !('masterPackageName' in group) && (!group.changelogPath || !group.mainPackageName || !group.include) + ); if (badGroups.length) { + isValid = false; console.error( 'ERROR: "changelog.groups" entries must define "changelogPath", "mainPackageName", and "include". ' + - 'Found invalid groups:\n' + + 'Invalid groups:\n' + bulletedList(badGroups.map(group => singleLineStringify(group))) ); - return false; } - return true; + const badExcludeGroups = options.groups.filter(group => { + const exclude = typeof group.exclude === 'string' ? [group.exclude] : group.exclude || []; + return exclude.some((pattern: string) => pattern.startsWith('!')); + }); + if (badExcludeGroups.length) { + isValid = false; + console.error( + 'ERROR: "changelog.groups[*].exclude" patterns must not start with "!" in v3. ' + + 'Found invalid groups:\n' + + bulletedList( + badExcludeGroups.map( + group => `mainPackageName "${group.mainPackageName}": ${singleLineStringify(group.exclude)}` + ) + ) + ); + } + + return isValid; } diff --git a/packages/beachball/src/validation/isValidGroupOptions.ts b/packages/beachball/src/validation/isValidGroupOptions.ts index 9020b7e68..555ae18a1 100644 --- a/packages/beachball/src/validation/isValidGroupOptions.ts +++ b/packages/beachball/src/validation/isValidGroupOptions.ts @@ -21,6 +21,18 @@ export function isValidGroupOptions(groups: VersionGroupOptions[]): boolean { return false; } + const badExcludeGroups = groups.filter(group => { + const exclude = typeof group.exclude === 'string' ? [group.exclude] : group.exclude || []; + return exclude.some((pattern: string) => pattern.startsWith('!')); + }); + if (badExcludeGroups.length) { + console.error( + 'ERROR: "groups[*].exclude" patterns must not start with "!". Found invalid groups:\n' + + bulletedList(badExcludeGroups.map(group => `${group.name}: ${singleLineStringify(group.exclude)}`)) + ); + return false; + } + // TODO: validate disallowedChangeTypes? (they're not currently validated anywhere) return true;