diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index dbb790808..7113586cf 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -29,8 +29,9 @@ Before merging, the reviewer should check the following items: - [ ] If this PR includes breaking changes, do we need to update any jobs in production? Is it safe to release? - [ ] Are there any unit tests? -- [ ] Is there a changeset associated with this PR? Should there be? Note that - dev only changes don't need a changeset. +- [ ] Does every changed adaptor have a corresponding changeset? If an adaptor + change does not affect a release, has a maintainer applied the + `no changeset required` label? Repository-only changes need neither. - [ ] If there is a changeset, was `pnpm run version` used to bump versions (not `pnpm changeset version` directly)? This ensures changelog dates are stamped correctly. - [ ] Have you ticked a box under AI Usage? diff --git a/.github/workflows/changeset.yaml b/.github/workflows/changeset.yaml new file mode 100644 index 000000000..ade9696fa --- /dev/null +++ b/.github/workflows/changeset.yaml @@ -0,0 +1,58 @@ +name: Changeset + +on: + pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] + +permissions: + contents: read + +jobs: + changeset: + name: Changeset required + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Check whether a changeset is required + id: requirements + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: node scripts/check-changesets.mjs detect --base "$BASE_SHA" + + - name: Record changeset exemption + if: >- + steps.requirements.outputs.required == 'true' && + contains(github.event.pull_request.labels.*.name, 'no changeset required') + run: | + echo "Changeset check skipped by the 'no changeset required' label." + { + echo "## Changeset check skipped" + echo + echo "A maintainer applied the \`no changeset required\` label." + } >> "$GITHUB_STEP_SUMMARY" + + - name: Setup pnpm + if: >- + steps.requirements.outputs.required == 'true' && + !contains(github.event.pull_request.labels.*.name, 'no changeset required') + uses: pnpm/action-setup@v4 + + - name: Install changeset checker dependencies + if: >- + steps.requirements.outputs.required == 'true' && + !contains(github.event.pull_request.labels.*.name, 'no changeset required') + run: pnpm install --filter adaptors --frozen-lockfile + + - name: Validate changeset coverage + if: >- + steps.requirements.outputs.required == 'true' && + !contains(github.event.pull_request.labels.*.name, 'no changeset required') + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: node scripts/check-changesets.mjs validate --base "$BASE_SHA" diff --git a/README.md b/README.md index e5ad2aac4..67a1ad3e4 100644 --- a/README.md +++ b/README.md @@ -222,8 +222,14 @@ in the wiki. ## Changesets -Any submitted PRs should have an accompanying -[changeset](https://github.com/changesets/changesets). +Every submitted PR that changes one or more adaptors under `packages/` must have +an accompanying [changeset](https://github.com/changesets/changesets) for each +changed adaptor. A single changeset may target multiple adaptors. Repository +tooling and top-level documentation changes do not require a changeset. + +The **Changeset required** GitHub check enforces this policy. It detects changed +adaptors and verifies that every corresponding package appears in an added or +modified changeset. - Create a new changeset with `pnpm changeset`. This will prompt you for the changes: @@ -233,9 +239,14 @@ pnpm changeset ``` > Follow the prompts to describe your changes. This will create a new changeset -> file in the `.changesets` folder. +> file in the `.changeset` folder. - Commit the changeset to the repo when you're ready. +- Run `pnpm changeset:check` to verify changeset coverage before pushing. +- Test, documentation, and development-only changes inside an adaptor directory + still count as adaptor changes. If such a change does not need a release, ask + a maintainer to apply the exact `no changeset required` label. Adding or + removing the label reruns the check. ## Versioning diff --git a/package.json b/package.json index b6c7c06f9..74a65c0f5 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ "build:tools": "pnpm --filter \"./tools/**\" build", "build": "pnpm build:tools && pnpm build:adaptors", "changelog": "pnpm -C tools/changelog update-latest-versions", + "changeset:check": "node scripts/check-changesets.mjs validate", "docs:build": "pnpm exec scripts/build-docs.sh", "docs:publish": "pnpm exec scripts/publish-docs.sh", "docs:watch": "chokidar packages/*/docs/*.md -c 'pnpm docs:build'", @@ -21,7 +22,8 @@ "setup": "pnpm --filter \"./tools/**\" install", "slack:notify": "cd tools/slack && pnpm notify", "test:imports": "cd tools/import-tests && pnpm test", - "test": "pnpm lint && pnpm --filter \"./packages/**\" test && pnpm test:imports", + "test": "pnpm lint && pnpm --filter \"./packages/**\" test && pnpm test:imports && pnpm test:changesets", + "test:changesets": "node --test scripts/check-changesets.test.mjs", "test:git": "pnpm exec scripts/status-check.sh", "validate:schemas": "node scripts/validate-configuration-schemas.mjs", "version": "pnpm changeset version && pnpm run changelog" @@ -30,6 +32,7 @@ "license": "ISC", "devDependencies": { "@changesets/cli": "^2.29.8", + "@changesets/parse": "^0.4.2", "@openfn/adaptor-apis": "workspace:*", "@openfn/buildtools": "workspace:^1.0.2", "@openfn/generate-fhir": "workspace:^", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7cf1be25c..c89895846 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -20,6 +20,9 @@ importers: '@changesets/cli': specifier: ^2.29.8 version: 2.29.8(@types/node@18.17.5) + '@changesets/parse': + specifier: ^0.4.2 + version: 0.4.2 '@openfn/adaptor-apis': specifier: workspace:* version: link:tools/adaptor-apis diff --git a/scripts/check-changesets.mjs b/scripts/check-changesets.mjs new file mode 100644 index 000000000..f01948497 --- /dev/null +++ b/scripts/check-changesets.mjs @@ -0,0 +1,314 @@ +#!/usr/bin/env node +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import process from 'node:process'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +const rootDir = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + '..' +); +const docsUrl = + 'https://github.com/OpenFn/adaptors/blob/main/README.md#changesets'; + +const git = (args, { cwd = rootDir, allowFailure = false } = {}) => { + try { + return execFileSync('git', args, { + cwd, + encoding: 'utf8', + stdio: ['ignore', 'pipe', allowFailure ? 'ignore' : 'pipe'], + }); + } catch (error) { + if (allowFailure) return null; + throw new Error(error.stderr?.trim() || error.message); + } +}; + +const splitFiles = output => output.split('\0').filter(Boolean); + +const resolveBase = cwd => { + for (const ref of ['origin/main', 'main']) { + if ( + git(['rev-parse', '--verify', '--quiet', `${ref}^{commit}`], { + cwd, + allowFailure: true, + }) + ) { + return ref; + } + } + throw new Error('Could not find origin/main or main. Pass --base .'); +}; + +const readPackage = async (directory, mergeBase, cwd) => { + const packageFile = `packages/${directory}/package.json`; + + try { + return JSON.parse(await fs.readFile(path.join(cwd, packageFile), 'utf8')); + } catch (error) { + if (error.code !== 'ENOENT') { + throw new Error(`Could not read ${packageFile}: ${error.message}`); + } + + const contents = git(['show', `${mergeBase}:${packageFile}`], { + cwd, + allowFailure: true, + }); + if (!contents) { + throw new Error(`Could not find ${packageFile}.`); + } + return JSON.parse(contents); + } +}; + +export const inspectRepository = async ({ + cwd = rootDir, + baseRef, +} = {}) => { + const base = baseRef ?? resolveBase(cwd); + const mergeBase = git(['merge-base', base, 'HEAD'], { cwd }).trim(); + const untrackedFiles = splitFiles( + git(['ls-files', '--others', '--exclude-standard', '-z'], { cwd }) + ); + const changedFiles = [ + ...splitFiles( + git(['diff', '--name-only', '--no-renames', '-z', mergeBase, '--'], { + cwd, + }) + ), + ...untrackedFiles, + ]; + + const pathsByDirectory = new Map(); + for (const file of new Set(changedFiles)) { + const match = /^packages\/([^/]+)\//.exec(file); + if (!match) continue; + + const paths = pathsByDirectory.get(match[1]) ?? []; + paths.push(file); + pathsByDirectory.set(match[1], paths); + } + + const packagesByName = new Map(); + for (const [directory, files] of pathsByDirectory) { + const packageJson = await readPackage(directory, mergeBase, cwd); + if (!packageJson.name?.startsWith('@openfn/language-')) { + throw new Error( + `packages/${directory}/package.json has no OpenFn adaptor package name.` + ); + } + + const paths = packagesByName.get(packageJson.name) ?? []; + paths.push(...files); + packagesByName.set(packageJson.name, paths); + } + const changedPackages = [...packagesByName] + .map(([name, files]) => ({ name, paths: [...new Set(files)].sort() })) + .sort((a, b) => a.name.localeCompare(b.name)); + + const changesetFiles = [ + ...splitFiles( + git( + [ + 'diff', + '--name-only', + '-z', + '--diff-filter=AM', + mergeBase, + '--', + '.changeset/*.md', + ], + { cwd } + ) + ), + ...untrackedFiles.filter(file => /^\.changeset\/[^/]+\.md$/.test(file)), + ] + .filter(file => file !== '.changeset/README.md') + .filter((file, index, files) => files.indexOf(file) === index) + .sort(); + + return { changedPackages, changesetFiles }; +}; + +export const evaluateCoverage = async ({ + cwd = rootDir, + baseRef, + parser, +} = {}) => { + const inspection = await inspectRepository({ cwd, baseRef }); + if (!inspection.changedPackages.length) { + return { ...inspection, covered: [], missing: [], errors: [] }; + } + + const parse = parser ?? (await import('@changesets/parse')).default; + const covered = new Set(); + const errors = []; + + for (const file of inspection.changesetFiles) { + try { + const changeset = parse(await fs.readFile(path.join(cwd, file), 'utf8')); + if (!changeset.releases.length) { + throw new Error('does not target any packages'); + } + for (const release of changeset.releases) covered.add(release.name); + } catch (error) { + errors.push(`${file}: ${error.message}`); + } + } + + return { + ...inspection, + covered: [...covered].sort(), + missing: inspection.changedPackages.filter( + pkg => !covered.has(pkg.name) + ), + errors, + }; +}; + +const appendEnvironmentFile = async (environment, name, contents) => { + if (environment[name]) { + await fs.appendFile(environment[name], `${contents.trimEnd()}\n`); + } +}; + +const packageList = packages => + packages.map(pkg => `- \`${pkg.name}\``).join('\n'); + +const reportNotRequired = async environment => { + console.log('No adaptor packages changed; a changeset is not required.'); + await appendEnvironmentFile( + environment, + 'GITHUB_STEP_SUMMARY', + `## Changeset not required + +No files under \`packages/*\` changed in this pull request.` + ); +}; + +const detect = async (baseRef, environment) => { + const { changedPackages } = await inspectRepository({ baseRef }); + const required = changedPackages.length > 0; + + await appendEnvironmentFile( + environment, + 'GITHUB_OUTPUT', + `required=${required} +changed-packages=${JSON.stringify(changedPackages.map(pkg => pkg.name))}` + ); + + if (!required) return reportNotRequired(environment); + + console.log('Changeset coverage is required for:'); + changedPackages.forEach(pkg => console.log(`- ${pkg.name}`)); + await appendEnvironmentFile( + environment, + 'GITHUB_STEP_SUMMARY', + `## Changeset required + +${packageList(changedPackages)}` + ); +}; + +const validate = async (baseRef, environment) => { + const result = await evaluateCoverage({ baseRef }); + if (!result.changedPackages.length) { + await reportNotRequired(environment); + return true; + } + + if (result.missing.length || result.errors.length) { + const lines = ['Changeset coverage check failed.']; + if (result.missing.length) { + lines.push('', 'Missing changesets for:'); + for (const pkg of result.missing) { + lines.push(`- ${pkg.name}`); + pkg.paths.forEach(file => lines.push(` - ${file}`)); + } + } + if (result.errors.length) { + lines.push('', 'Invalid changesets:'); + result.errors.forEach(error => lines.push(`- ${error}`)); + } + lines.push( + '', + "Run 'pnpm changeset' and commit the generated file.", + "If no release is needed, ask a maintainer to apply the exact 'no changeset required' label.", + `See ${docsUrl}.` + ); + + const message = lines.join('\n'); + console.error(message); + if (environment.GITHUB_ACTIONS === 'true') { + const annotation = message + .replace(/%/g, '%25') + .replace(/\r/g, '%0D') + .replace(/\n/g, '%0A'); + console.error(`::error title=Changeset required::${annotation}`); + } + await appendEnvironmentFile( + environment, + 'GITHUB_STEP_SUMMARY', + `## Changeset coverage failed + +\`\`\` +${message} +\`\`\`` + ); + return false; + } + + console.log('Changesets cover every changed adaptor package.'); + result.changedPackages.forEach(pkg => console.log(`- ${pkg.name}`)); + await appendEnvironmentFile( + environment, + 'GITHUB_STEP_SUMMARY', + `## Changeset coverage passed + +${packageList(result.changedPackages)}` + ); + return true; +}; + +const parseArguments = argv => { + const mode = argv[0]; + if (!['detect', 'validate'].includes(mode)) { + throw new Error( + 'Usage: node scripts/check-changesets.mjs [--base ]' + ); + } + + if (argv.length === 1) return { mode }; + if (argv.length === 3 && argv[1] === '--base') { + return { mode, baseRef: argv[2] }; + } + throw new Error('Expected an optional --base argument.'); +}; + +export const main = async ( + argv = process.argv.slice(2), + environment = process.env +) => { + const { mode, baseRef } = parseArguments(argv); + if (mode === 'detect') { + await detect(baseRef, environment); + return true; + } + return validate(baseRef, environment); +}; + +const isMain = + process.argv[1] && + import.meta.url === pathToFileURL(path.resolve(process.argv[1])).href; + +if (isMain) { + main() + .then(success => { + if (!success) process.exitCode = 1; + }) + .catch(error => { + console.error(error.message); + process.exitCode = 1; + }); +} diff --git a/scripts/check-changesets.test.mjs b/scripts/check-changesets.test.mjs new file mode 100644 index 000000000..9ffb439c8 --- /dev/null +++ b/scripts/check-changesets.test.mjs @@ -0,0 +1,218 @@ +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; +import test from 'node:test'; + +import { + evaluateCoverage, + inspectRepository, +} from './check-changesets.mjs'; + +const git = (cwd, ...args) => + execFileSync('git', args, { + cwd, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + +const write = async (cwd, file, contents) => { + const target = path.join(cwd, file); + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.writeFile(target, contents); +}; + +const packageJson = name => JSON.stringify({ name }, null, 2); + +const createRepository = async () => { + const cwd = await fs.mkdtemp(path.join(os.tmpdir(), 'changeset-check-')); + git(cwd, 'init', '--initial-branch=main'); + git(cwd, 'config', 'user.name', 'Changeset Test'); + git(cwd, 'config', 'user.email', 'changeset@example.com'); + + await write( + cwd, + 'packages/common/package.json', + packageJson('@openfn/language-common') + ); + await write(cwd, 'packages/common/index.js', 'export const common = 1;\n'); + await write( + cwd, + 'packages/gmail/package.json', + packageJson('@openfn/language-gmail') + ); + await write(cwd, 'packages/gmail/index.js', 'export const gmail = 1;\n'); + await write(cwd, 'README.md', '# Test repository\n'); + + git(cwd, 'add', '.'); + git(cwd, 'commit', '-m', 'Initial commit'); + git(cwd, 'switch', '-c', 'feature'); + return cwd; +}; + +const withRepository = async callback => { + const cwd = await createRepository(); + try { + await callback(cwd); + } finally { + await fs.rm(cwd, { recursive: true, force: true }); + } +}; + +const changeset = releases => { + const frontmatter = releases + .map(([name, type]) => `'${name}': ${type}`) + .join('\n'); + return `---\n${frontmatter}\n---\n\nTest change.\n`; +}; + +const packageNames = result => result.changedPackages.map(pkg => pkg.name); +const missingNames = result => result.missing.map(pkg => pkg.name); + +test('repository-only changes need no changeset', async () => + withRepository(async cwd => { + await write(cwd, 'README.md', '# Updated repository\n'); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(result.changedPackages, []); + assert.deepEqual(result.missing, []); + })); + +test('ignores the changeset README', async () => + withRepository(async cwd => { + await write(cwd, '.changeset/README.md', 'Documentation only.\n'); + const result = await inspectRepository({ cwd, baseRef: 'main' }); + + assert.deepEqual(result.changesetFiles, []); + })); + +test('reports a changed adaptor without a changeset', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(missingNames(result), ['@openfn/language-common']); + })); + +test('accepts a changeset targeting the changed adaptor', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write( + cwd, + '.changeset/common.md', + changeset([['@openfn/language-common', 'patch']]) + ); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(result.missing, []); + })); + +test('reports partial coverage across two adaptors', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write(cwd, 'packages/gmail/index.js', 'export const gmail = 2;\n'); + await write( + cwd, + '.changeset/gmail.md', + changeset([['@openfn/language-gmail', 'minor']]) + ); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(missingNames(result), ['@openfn/language-common']); + })); + +test('accepts one changeset targeting two adaptors', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write(cwd, 'packages/gmail/index.js', 'export const gmail = 2;\n'); + await write( + cwd, + '.changeset/both.md', + changeset([ + ['@openfn/language-common', 'patch'], + ['@openfn/language-gmail', 'minor'], + ]) + ); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(result.missing, []); + })); + +test('accepts separate changesets targeting two adaptors', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write(cwd, 'packages/gmail/index.js', 'export const gmail = 2;\n'); + await write( + cwd, + '.changeset/common.md', + changeset([['@openfn/language-common', 'patch']]) + ); + await write( + cwd, + '.changeset/gmail.md', + changeset([['@openfn/language-gmail', 'minor']]) + ); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(result.missing, []); + })); + +test('an unrelated changeset does not cover a changed adaptor', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write( + cwd, + '.changeset/gmail.md', + changeset([['@openfn/language-gmail', 'patch']]) + ); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.deepEqual(missingNames(result), ['@openfn/language-common']); + })); + +test('reports empty and malformed changesets', async () => + withRepository(async cwd => { + await write(cwd, 'packages/common/index.js', 'export const common = 2;\n'); + await write(cwd, '.changeset/empty.md', '---\n---\n'); + await write(cwd, '.changeset/malformed.md', '---\ninvalid: [\n---\n'); + const result = await evaluateCoverage({ cwd, baseRef: 'main' }); + + assert.equal(result.errors.length, 2); + })); + +test('detects both adaptors in a cross-adaptor rename', async () => + withRepository(async cwd => { + git( + cwd, + 'mv', + 'packages/common/index.js', + 'packages/gmail/from-common.js' + ); + const result = await inspectRepository({ cwd, baseRef: 'main' }); + + assert.deepEqual(packageNames(result), [ + '@openfn/language-common', + '@openfn/language-gmail', + ]); + })); + +test('resolves added and deleted package names', async () => + withRepository(async cwd => { + await fs.rm(path.join(cwd, 'packages/common'), { + recursive: true, + force: true, + }); + await write( + cwd, + 'packages/new/package.json', + packageJson('@openfn/language-new') + ); + await write(cwd, 'packages/new/index.js', 'export const value = 1;\n'); + const result = await inspectRepository({ cwd, baseRef: 'main' }); + + assert.deepEqual(packageNames(result), [ + '@openfn/language-common', + '@openfn/language-new', + ]); + }));