diff --git a/.github/workflows/changelog-preview.yml b/.github/workflows/changelog-preview.yml index 95d6bdcb..7bc6fb8a 100644 --- a/.github/workflows/changelog-preview.yml +++ b/.github/workflows/changelog-preview.yml @@ -8,7 +8,6 @@ on: description: 'Version of Craft to use (tag or "latest")' required: false type: string - default: 'latest' # Also run on PRs in this repository (for dogfooding) # Includes 'edited' and 'labeled' to update when PR title/description/labels change @@ -27,9 +26,8 @@ jobs: fetch-depth: 0 # Install Craft using the shared install action - # TODO: Change to @v2 or @master after this PR is merged - name: Install Craft - uses: getsentry/craft/install@pull/669/head + uses: getsentry/craft/install@master with: craft-version: ${{ inputs.craft-version || 'latest' }} @@ -49,8 +47,11 @@ jobs: # Extract fields from JSON CHANGELOG=$(echo "$RESULT" | jq -r '.changelog // ""') BUMP_TYPE=$(echo "$RESULT" | jq -r '.bumpType // "none"') + PR_SKIPPED=$(echo "$RESULT" | jq -r '.prSkipped // false') - if [[ -z "$CHANGELOG" ]]; then + if [[ "$PR_SKIPPED" == "true" ]]; then + CHANGELOG="_This PR will not appear in the changelog._" + elif [[ -z "$CHANGELOG" ]]; then CHANGELOG="_No changelog entries will be generated from this PR._" fi diff --git a/src/commands/changelog.ts b/src/commands/changelog.ts index 53cfed7b..f4cc99c0 100644 --- a/src/commands/changelog.ts +++ b/src/commands/changelog.ts @@ -74,6 +74,7 @@ export async function changelogMain(argv: ChangelogOptions): Promise { bumpType: result.bumpType, totalCommits: result.totalCommits, matchedCommitsWithSemver: result.matchedCommitsWithSemver, + prSkipped: result.prSkipped ?? false, }; console.log(JSON.stringify(output, null, 2)); } else { diff --git a/src/utils/__tests__/changelog.test.ts b/src/utils/__tests__/changelog.test.ts index 92b7c005..e2946329 100644 --- a/src/utils/__tests__/changelog.test.ts +++ b/src/utils/__tests__/changelog.test.ts @@ -26,8 +26,12 @@ import { extractScope, formatScopeTitle, clearChangesetCache, + shouldExcludePR, + shouldSkipCurrentPR, + getBumpTypeForPR, SKIP_CHANGELOG_MAGIC_WORD, BODY_IN_CHANGELOG_MAGIC_WORD, + CurrentPRInfo, } from '../changelog'; const getConfigFileDirMock = config.getConfigFileDir as jest.MockedFunction; @@ -2766,3 +2770,346 @@ describe('formatScopeTitle', () => { expect(formatScopeTitle(scope)).toBe(expected); }); }); + +describe('shouldExcludePR', () => { + it('should return true when body contains #skip-changelog', () => { + const labels = new Set(); + expect(shouldExcludePR(labels, 'user', null, 'Some text #skip-changelog here')).toBe(true); + }); + + it('should return false when body does not contain magic word', () => { + const labels = new Set(); + expect(shouldExcludePR(labels, 'user', null, 'Normal body text')).toBe(false); + }); + + it('should return false when body is undefined', () => { + const labels = new Set(); + expect(shouldExcludePR(labels, 'user', null, undefined)).toBe(false); + }); + + it('should return false when body is empty', () => { + const labels = new Set(); + expect(shouldExcludePR(labels, 'user', null, '')).toBe(false); + }); + + it('should check body before config (early exit)', () => { + // Even with no config, magic word should cause exclusion + const labels = new Set(['feature']); + expect(shouldExcludePR(labels, 'user', null, '#skip-changelog')).toBe(true); + }); +}); + +describe('shouldSkipCurrentPR', () => { + const basePRInfo: CurrentPRInfo = { + number: 123, + title: 'Test PR', + body: '', + author: 'testuser', + labels: [], + baseRef: 'main', + }; + + beforeEach(() => { + clearChangesetCache(); + getConfigFileDirMock.mockReturnValue('/test'); + }); + + it('should return true when PR body contains #skip-changelog', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'This is a PR description\n\n#skip-changelog', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + }); + + it('should return true when PR body contains #skip-changelog inline', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'This is internal work #skip-changelog for now', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + }); + + it('should return false when PR body does not contain skip marker', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'This is a regular PR description', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(false); + }); + + it('should return true when PR has an excluded label from config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'Normal description', + labels: ['skip-changelog'], + }; + readFileSyncMock.mockImplementation((path: any) => { + if (typeof path === 'string' && path.includes('release.yml')) { + return `changelog: + exclude: + labels: + - skip-changelog + categories: + - title: Features + labels: + - feature`; + } + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + }); + + it('should return true when PR author is excluded in config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'Normal description', + author: 'dependabot[bot]', + }; + readFileSyncMock.mockImplementation((path: any) => { + if (typeof path === 'string' && path.includes('release.yml')) { + return `changelog: + exclude: + authors: + - dependabot[bot] + categories: + - title: Features + labels: + - feature`; + } + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + }); + + it('should return false when PR does not match any exclusion criteria', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'Normal description', + labels: ['feature'], + author: 'regularuser', + }; + readFileSyncMock.mockImplementation((path: any) => { + if (typeof path === 'string' && path.includes('release.yml')) { + return `changelog: + exclude: + labels: + - skip-changelog + authors: + - dependabot[bot] + categories: + - title: Features + labels: + - feature`; + } + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(false); + }); + + it('should prioritize magic word over config (skip even without config)', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + body: 'Description with #skip-changelog', + labels: ['feature'], + }; + // No release config + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + }); +}); + +describe('getBumpTypeForPR', () => { + const basePRInfo: CurrentPRInfo = { + number: 123, + title: 'Test PR', + body: '', + author: 'testuser', + labels: [], + baseRef: 'main', + }; + + beforeEach(() => { + clearChangesetCache(); + getConfigFileDirMock.mockReturnValue('/test'); + }); + + it('should return minor for feat: prefix with default config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'feat: Add new feature', + }; + // No release config - uses default conventional commits + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('minor'); + }); + + it('should return patch for fix: prefix with default config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'fix: Fix a bug', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('patch'); + }); + + it('should return major for breaking change with default config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'feat!: Breaking change', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('major'); + }); + + it('should return null for unmatched title', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'Random commit message', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBeNull(); + }); + + it('should match by label when config has label-based categories', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'Some random title', + labels: ['feature'], + }; + readFileSyncMock.mockImplementation((path: any) => { + if (typeof path === 'string' && path.includes('release.yml')) { + return `changelog: + categories: + - title: Features + labels: + - feature + semver: minor`; + } + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('minor'); + }); + + it('should work for skipped PRs (still determines bump type)', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'feat: New feature', + body: '#skip-changelog', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + // PR is skipped but should still have a bump type + expect(shouldSkipCurrentPR(prInfo)).toBe(true); + expect(getBumpTypeForPR(prInfo)).toBe('minor'); + }); + + it('should return minor for feat with scope', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'feat(api): Add new endpoint', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('minor'); + }); + + it('should return patch for fix with scope', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'fix(core): Fix memory leak', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + expect(getBumpTypeForPR(prInfo)).toBe('patch'); + }); + + it('should return patch for docs: prefix in default config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'docs: Update README', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + // docs category in default config has patch semver + expect(getBumpTypeForPR(prInfo)).toBe('patch'); + }); + + it('should return patch for chore: prefix in default config', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'chore: Update dependencies', + }; + readFileSyncMock.mockImplementation(() => { + throw { code: 'ENOENT' }; + }); + + // chore is in the build/internal category with patch semver + expect(getBumpTypeForPR(prInfo)).toBe('patch'); + }); + + it('should prefer label over title pattern when both match', () => { + const prInfo: CurrentPRInfo = { + ...basePRInfo, + title: 'feat: This looks like a feature', + labels: ['bug'], // Label says bug, title says feat + }; + readFileSyncMock.mockImplementation((path: any) => { + if (typeof path === 'string' && path.includes('release.yml')) { + return `changelog: + categories: + - title: Bug Fixes + labels: + - bug + semver: patch + - title: Features + labels: + - feature + semver: minor`; + } + throw { code: 'ENOENT' }; + }); + + // Label takes precedence, so should be patch (bug) not minor (feat) + expect(getBumpTypeForPR(prInfo)).toBe('patch'); + }); +}); diff --git a/src/utils/changelog.ts b/src/utils/changelog.ts index 55348cb3..78f398b4 100644 --- a/src/utils/changelog.ts +++ b/src/utils/changelog.ts @@ -571,11 +571,29 @@ export function normalizeReleaseConfig( /** * Checks if a PR should be excluded globally based on release config */ +/** + * Checks if a PR should be excluded globally based on: + * 1. The #skip-changelog magic word in the body (commit body or PR body) + * 2. Excluded labels from release config + * 3. Excluded authors from release config + * + * @param labels Set of labels on the PR + * @param author Author of the PR + * @param config Normalized release config + * @param body Optional body text to check for magic word + * @returns true if the PR should be excluded + */ export function shouldExcludePR( labels: Set, author: string | undefined, - config: NormalizedReleaseConfig | null + config: NormalizedReleaseConfig | null, + body?: string ): boolean { + // Check for magic word in body + if (body?.includes(SKIP_CHANGELOG_MAGIC_WORD)) { + return true; + } + if (!config?.changelog) { return false; } @@ -595,6 +613,44 @@ export function shouldExcludePR( return false; } +/** + * Checks if the current PR should be skipped from the changelog entirely. + * Convenience wrapper around shouldExcludePR that loads config automatically. + * + * @param prInfo The current PR info + * @returns true if the PR should be skipped + */ +export function shouldSkipCurrentPR(prInfo: CurrentPRInfo): boolean { + const rawConfig = readReleaseConfig(); + const releaseConfig = normalizeReleaseConfig(rawConfig); + const labels = new Set(prInfo.labels); + + return shouldExcludePR(labels, prInfo.author, releaseConfig, prInfo.body); +} + +/** + * Determines the version bump type for a PR based on its labels and title. + * This is used to determine the release version even for PRs that are + * excluded from the changelog (e.g., via #skip-changelog). + * + * @param prInfo The current PR info + * @returns The bump type (major, minor, patch) or null if no match + */ +export function getBumpTypeForPR(prInfo: CurrentPRInfo): BumpType | null { + const rawConfig = readReleaseConfig(); + const releaseConfig = normalizeReleaseConfig(rawConfig); + const labels = new Set(prInfo.labels); + + const matchedCategory = matchCommitToCategory( + labels, + prInfo.author, + prInfo.title.trim(), + releaseConfig + ); + + return matchedCategory?.semver ?? null; +} + /** * Checks if a category excludes the given PR based on labels and author */ @@ -771,6 +827,8 @@ export interface ChangelogResult { totalCommits: number; /** Number of commits that matched a category with a semver field */ matchedCommitsWithSemver: number; + /** Whether the current PR was skipped (only set when using --pr flag) */ + prSkipped?: boolean; } /** @@ -865,15 +923,29 @@ export async function generateChangelogWithHighlight( // Step 1: Fetch PR info from GitHub const prInfo = await fetchPRInfo(currentPRNumber); - // Step 2: Fetch the base branch to get current state + // Step 2: Check if PR should be skipped - bypass changelog generation but still determine bump type + if (shouldSkipCurrentPR(prInfo)) { + // Even skipped PRs contribute to version bumping based on their title + const bumpType = getBumpTypeForPR(prInfo); + + return { + changelog: '', + bumpType, + totalCommits: 1, + matchedCommitsWithSemver: bumpType ? 1 : 0, + prSkipped: true, + }; + } + + // Step 3: Fetch the base branch to get current state await git.fetch('origin', prInfo.baseRef); const baseRef = `origin/${prInfo.baseRef}`; logger.debug(`Using PR base branch "${prInfo.baseRef}" for changelog`); - // Step 3: Fetch raw commit info up to base branch + // Step 4: Fetch raw commit info up to base branch const rawCommits = await fetchRawCommitInfo(git, rev, baseRef); - // Step 4: Add current PR to the list with highlight flag (at the beginning) + // Step 5: Add current PR to the list with highlight flag (at the beginning) const currentPRCommit: RawCommitInfo = { hash: '', title: prInfo.title.trim(), @@ -887,14 +959,15 @@ export async function generateChangelogWithHighlight( }; const allCommits = [currentPRCommit, ...rawCommits]; - // Step 5: Run categorization on combined list + // Step 6: Run categorization on combined list const { data: rawData, stats } = categorizeCommits(allCommits); - // Step 6: Serialize to markdown + // Step 7: Serialize to markdown const changelog = await serializeChangelog(rawData, MAX_LEFTOVERS); return { changelog, + prSkipped: false, ...stats, }; } @@ -913,6 +986,7 @@ async function fetchRawCommitInfo( rev: string, until?: string ): Promise { + // Early filter: skip commits with magic word in commit body (optimization to avoid GitHub API calls) const gitCommits = (await getChangesSince(git, rev, until)).filter( ({ body }) => !body.includes(SKIP_CHANGELOG_MAGIC_WORD) ); @@ -921,17 +995,10 @@ async function fetchRawCommitInfo( gitCommits.map(({ hash }) => hash) ); - const result: RawCommitInfo[] = []; - - for (const gitCommit of gitCommits) { + // Note: PR body magic word check is handled by shouldExcludePR in categorizeCommits + return gitCommits.map(gitCommit => { const githubCommit = githubCommits[gitCommit.hash]; - - // Skip if PR body has skip marker - if (githubCommit?.prBody?.includes(SKIP_CHANGELOG_MAGIC_WORD)) { - continue; - } - - result.push({ + return { hash: gitCommit.hash, title: gitCommit.title, body: gitCommit.body, @@ -940,10 +1007,8 @@ async function fetchRawCommitInfo( prTitle: githubCommit?.prTitle ?? undefined, prBody: githubCommit?.prBody ?? undefined, labels: githubCommit?.labels ?? [], - }); - } - - return result; + }; + }); } /** @@ -967,8 +1032,10 @@ function categorizeCommits(rawCommits: RawCommitInfo[]): RawChangelogResult { for (const raw of rawCommits) { const labels = new Set(raw.labels); + // Use PR body if available, otherwise use commit body for skip-changelog check + const bodyToCheck = raw.prBody ?? raw.body; - if (shouldExcludePR(labels, raw.author, releaseConfig)) { + if (shouldExcludePR(labels, raw.author, releaseConfig, bodyToCheck)) { continue; }