From 46f5cf709a01780c60fbf672567025161f84ef06 Mon Sep 17 00:00:00 2001 From: Jack Schofield Date: Mon, 3 Aug 2020 22:25:16 +0100 Subject: [PATCH] add validate command (#31) * adding a validate command to help validate CODEOWNERS file * fixing vuln in dev dependency --- README.md | 39 +++++++++++--- package-lock.json | 40 ++++++++------ package.json | 2 +- src/cli.ts | 24 +++++++++ src/commands/__fixtures__/validate/files.json | 6 +++ src/commands/__fixtures__/validate/index.ts | 16 ++++++ src/commands/__fixtures__/validate/owners | 4 ++ .../validate/owners-invalid-format | 1 + .../__snapshots__/validate.test.int.ts.snap | 9 ++++ src/commands/validate.test.int.ts | 52 +++++++++++++++++++ src/commands/validate.ts | 21 ++++++++ src/lib/logger/logger.ts | 4 +- src/lib/ownership/index.ts | 1 + src/lib/ownership/lib/OwnershipEngine.test.ts | 19 +++++++ src/lib/ownership/lib/OwnershipEngine.ts | 20 +++++-- src/lib/ownership/types.ts | 2 + src/lib/ownership/validate.ts | 46 ++++++++++++++++ 17 files changed, 277 insertions(+), 29 deletions(-) create mode 100644 src/commands/__fixtures__/validate/files.json create mode 100644 src/commands/__fixtures__/validate/index.ts create mode 100644 src/commands/__fixtures__/validate/owners create mode 100644 src/commands/__fixtures__/validate/owners-invalid-format create mode 100644 src/commands/__snapshots__/validate.test.int.ts.snap create mode 100644 src/commands/validate.test.int.ts create mode 100644 src/commands/validate.ts create mode 100644 src/lib/ownership/validate.ts diff --git a/README.md b/README.md index a23db20..93f2fbc 100644 --- a/README.md +++ b/README.md @@ -6,13 +6,12 @@ A CLI tool for working with GitHub CODEOWNERS. Things it does: -* Output who owns each file in your repo (ignoring files listed in `.gitignore`) -* Output who owns a single file -* Output ownership information at a specific git commit -* Output ownership information of staged files -* Outputs lots of lovely stats -* Outputs handy formats for integrations (CSV and JSONL) -* Validates that the provided owners are in the correct format for github +* Calculate ownership stats +* Find out who owns each and every file (ignoring files listed in `.gitignore`) +* Find out who owns a single file +* Find out who owns your staged files +* Outputs in a bunch of script friendly handy formats for integrations (CSV and JSONL) +* Validates that your CODEOWNERS file is valid ## Installation Install via npm globally then run @@ -82,7 +81,7 @@ Usage: github-codeowners audit [options] list the owners for all files Options: - -d, --dir path to VCS directory (default: "/Users/jjmschofield/projects/github/github-codeowners") + -d, --dir path to VCS directory (default: "") -c, --codeowners path to codeowners file (default: "/.github/CODEOWNERS") -o, --output how to output format eg: simple, jsonl, csv (default: "simple") -u, --unloved unowned files only (default: false) @@ -152,6 +151,30 @@ Options: -h, --help output usage information ``` +### Validate +Validates your CODEOWNERS file to find common mistakes, will throw on errors (such as malformed owners). +```shell script +$ cd +$ github-codeowners validate +Found duplicate rules [ 'some/duplicate/rule @octocat' ] +Found rules which did not match any files [ 'some/non-existent/path @octocat' ] +... +``` + +Full usage information: +```shell script +$ github-codeowners validate --help +Usage: github-codeowners validate [options] + +Validates a CODOWNER file and files in dir + +Options: + -d, --dir path to VCS directory (default: "") + -c, --codeowners path to codeowners file (default: "/.github/CODEOWNERS") + -r, --root the root path to filter files by (default: "") + -h, --help output usage information +``` + ## Output Formats Check `github-codeowners --help` for support for a given command, however generally the following outputs are supported: * `simple` - tab delimited - terminal friendly output diff --git a/package-lock.json b/package-lock.json index df3db81..ad38cbc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "github-codeowners", - "version": "0.1.0", + "version": "0.1.1", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -1684,7 +1684,8 @@ "balanced-match": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-1.0.0.tgz", - "integrity": "sha1-ibTRmasr7kneFk6gK4nORi1xt2c=" + "integrity": "sha1-ibTRmasr7kneFk6gK4nORi1xt2c=", + "dev": true }, "base": { "version": "0.11.2", @@ -1754,6 +1755,7 @@ "version": "1.1.11", "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.11.tgz", "integrity": "sha512-iCuPHDFgrHX7H2vEI/5xpz07zSHB00TpugqhmYtVmMO6518mCuRMoOYFldEBl0g187ufozdaHgWKcYFb61qGiA==", + "dev": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -1965,7 +1967,8 @@ "concat-map": { "version": "0.0.1", "resolved": "https://registry.npmjs.org/concat-map/-/concat-map-0.0.1.tgz", - "integrity": "sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=" + "integrity": "sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=", + "dev": true }, "convert-source-map": { "version": "1.7.0", @@ -5483,14 +5486,15 @@ "version": "3.0.4", "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-3.0.4.tgz", "integrity": "sha512-yJHVQEhyqPLUTgt9B83PXu6W3rx4MvvHvSUvToogpwoGDOUQ+yDrR0HRot+yOCdCO7u4hX3pWft6kWBBcqh0UA==", + "dev": true, "requires": { "brace-expansion": "^1.1.7" } }, "minimist": { - "version": "0.0.8", - "resolved": "https://registry.npmjs.org/minimist/-/minimist-0.0.8.tgz", - "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0=", + "version": "1.2.5", + "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", + "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", "dev": true }, "mixin-deep": { @@ -5515,12 +5519,12 @@ } }, "mkdirp": { - "version": "0.5.1", - "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.1.tgz", - "integrity": "sha1-MAV0OOrGz3+MR2fzhkjWaX11yQM=", + "version": "0.5.5", + "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.5.tgz", + "integrity": "sha512-NKmAlESf6jMGym1++R0Ra7wvhV+wFW63FaSOFPwRahvea0gMUcGUhVeAg/0BC0wiv9ih5NYPB1Wn1UEI1/L+xQ==", "dev": true, "requires": { - "minimist": "0.0.8" + "minimist": "^1.2.5" } }, "ms": { @@ -6880,9 +6884,9 @@ "dev": true }, "tslint": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/tslint/-/tslint-6.0.0.tgz", - "integrity": "sha512-9nLya8GBtlFmmFMW7oXXwoXS1NkrccqTqAtwXzdPV9e2mqSEvCki6iHL/Fbzi5oqbugshzgGPk7KBb2qNP1DSA==", + "version": "6.1.3", + "resolved": "https://registry.npmjs.org/tslint/-/tslint-6.1.3.tgz", + "integrity": "sha512-IbR4nkT96EQOvKE2PW/djGz8iGNeJ4rF2mBfiYaR/nvUWYKJhLwimoJKgjIFEIDibBtOevj7BqCRL4oHeWWUCg==", "dev": true, "requires": { "@babel/code-frame": "^7.0.0", @@ -6893,10 +6897,10 @@ "glob": "^7.1.1", "js-yaml": "^3.13.1", "minimatch": "^3.0.4", - "mkdirp": "^0.5.1", + "mkdirp": "^0.5.3", "resolve": "^1.3.2", "semver": "^5.3.0", - "tslib": "^1.10.0", + "tslib": "^1.13.0", "tsutils": "^2.29.0" }, "dependencies": { @@ -6905,6 +6909,12 @@ "resolved": "https://registry.npmjs.org/commander/-/commander-2.20.3.tgz", "integrity": "sha512-GpVkmM8vF2vQUkj2LvZmD35JxeJOLCwJ9cUkugyk2nuhbv3+mJvpLYYt+0+USMxE+oj+ey/lJEnhZw75x/OMcQ==", "dev": true + }, + "tslib": { + "version": "1.13.0", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-1.13.0.tgz", + "integrity": "sha512-i/6DQjL8Xf3be4K/E6Wgpekn5Qasl1usyw++dAA35Ue5orEn65VIxOA+YvNNl9HV3qv70T7CNwjODHZrLwvd1Q==", + "dev": true } } }, diff --git a/package.json b/package.json index f3985bf..5af69f7 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "jest-junit": "^11.0.1", "ts-jest": "^26.1.3", "ts-node": "^8.6.2", - "tslint": "^6.0.0", + "tslint": "^6.1.3", "tslint-config-airbnb-base": "^0.3.0", "typescript": "^3.8.2", "uuid": "^8.2.0" diff --git a/src/cli.ts b/src/cli.ts index 21d28f3..92c8342 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -9,6 +9,7 @@ import { git } from './commands/git'; import { log } from './lib/logger'; import { OUTPUT_FORMAT } from './lib/writers/types'; +import { validate } from './commands/validate'; commander.command('audit') .description('list the owners for all files') @@ -35,6 +36,29 @@ commander.command('audit') } }); +commander.command('validate') + .description('Validates a CODOWNER file and files in dir') + .option('-d, --dir ', 'path to VCS directory', process.cwd()) + .option('-c, --codeowners ', 'path to codeowners file (default: "/.github/CODEOWNERS")') + .option('-r, --root ', 'the root path to filter files by', '') + .action(async (options) => { + try { + if (!options.codeowners) { + options.codeowners = path.resolve(options.dir, '.github/CODEOWNERS'); + } + + if (options.root) { + options.dir = path.resolve(options.dir, options.root); + } + + await validate(options); + } catch (error) { + log.error('failed to run validate command', error); + process.exit(1); + } + }); + + commander.command('who ') .description('lists owners of a specific file') .option('-d, --dir ', 'path to VCS directory', process.cwd()) diff --git a/src/commands/__fixtures__/validate/files.json b/src/commands/__fixtures__/validate/files.json new file mode 100644 index 0000000..460e1d8 --- /dev/null +++ b/src/commands/__fixtures__/validate/files.json @@ -0,0 +1,6 @@ +{ + "files" : [ + { "path": "valid.js" }, + { "path": "duplicate-rule.js" } + ] +} diff --git a/src/commands/__fixtures__/validate/index.ts b/src/commands/__fixtures__/validate/index.ts new file mode 100644 index 0000000..c1aa8f6 --- /dev/null +++ b/src/commands/__fixtures__/validate/index.ts @@ -0,0 +1,16 @@ +import path from 'path'; + +import { FixturePaths } from '../types'; + +const paths: FixturePaths = { + files: path.resolve(__dirname, 'files.json'), + codeowners: path.resolve(__dirname, 'owners'), + gitignores: [], +}; + +export const invalidOwnerFixtures: FixturePaths = { + ...paths, + codeowners: path.resolve(__dirname, 'owners-invalid-format'), +}; + +export default paths; diff --git a/src/commands/__fixtures__/validate/owners b/src/commands/__fixtures__/validate/owners new file mode 100644 index 0000000..cb48570 --- /dev/null +++ b/src/commands/__fixtures__/validate/owners @@ -0,0 +1,4 @@ +valid.js @some-owner +duplicate-rule.js @some-owner +file-does-not-exist.md @some-owner +duplicate-rule.js @some-owner diff --git a/src/commands/__fixtures__/validate/owners-invalid-format b/src/commands/__fixtures__/validate/owners-invalid-format new file mode 100644 index 0000000..e7ad4d5 --- /dev/null +++ b/src/commands/__fixtures__/validate/owners-invalid-format @@ -0,0 +1 @@ +file-exists-duplicate-rule.md @valid-owner not-a-valid-owner diff --git a/src/commands/__snapshots__/validate.test.int.ts.snap b/src/commands/__snapshots__/validate.test.int.ts.snap new file mode 100644 index 0000000..13b994d --- /dev/null +++ b/src/commands/__snapshots__/validate.test.int.ts.snap @@ -0,0 +1,9 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`validate when all owners are valid output the result of all validation checks: stderr 1`] = ` +"Found duplicate rules [ 'duplicate-rule.js @some-owner' ] +Found rules which did not match any files [ 'file-does-not-exist.md @some-owner' ] +" +`; + +exports[`validate when all owners are valid output the result of all validation checks: stdout 1`] = `""`; diff --git a/src/commands/validate.test.int.ts b/src/commands/validate.test.int.ts new file mode 100644 index 0000000..26325c1 --- /dev/null +++ b/src/commands/validate.test.int.ts @@ -0,0 +1,52 @@ +import { v4 as uuidv4 } from 'uuid'; +import fixtures, { invalidOwnerFixtures } from './__fixtures__/validate'; +import { generateProject } from './__fixtures__/project-builder.test.helper'; + +import util from 'util'; + +const exec = util.promisify(require('child_process').exec); + +describe('validate', () => { + describe('when all owners are valid', () => { + const testId = uuidv4(); + + let testDir = 'not set'; + + beforeAll(async () => { + testDir = await generateProject(testId, fixtures); + // tslint:disable-next-line:no-console + console.log(`test scratch dir: ${testDir}`); + }); + + const runCli = async (args: string) => { + return exec(`node ../../../dist/cli.js ${args}`, { cwd: testDir }); + }; + + it('output the result of all validation checks', async () => { + const { stdout, stderr } = await runCli('validate'); + expect(stdout).toMatchSnapshot('stdout'); + expect(stderr).toMatchSnapshot('stderr'); + }); + }); + + + describe('when owners are invalid', () => { + const testId = uuidv4(); + + let testDir = 'not set'; + + beforeAll(async () => { + testDir = await generateProject(testId, invalidOwnerFixtures); + // tslint:disable-next-line:no-console + console.log(`test scratch dir: ${testDir}`); + }); + + const runCli = async (args: string) => { + return exec(`node ../../../dist/cli.js ${args}`, { cwd: testDir }); + }; + + it('should throw on invalid users', async () => { + await expect(() => runCli('validate')).rejects.toThrow(); + }); + }); +}); diff --git a/src/commands/validate.ts b/src/commands/validate.ts new file mode 100644 index 0000000..ddaa361 --- /dev/null +++ b/src/commands/validate.ts @@ -0,0 +1,21 @@ +// import { writeOwnedFile, writeStats } from '../lib/writers'; +import { validate as assertValidRules } from '../lib/ownership'; +import { log } from '../lib/logger'; + +interface ValidateOptions { + codeowners: string; + dir: string; + root: string; +} + +export const validate = async (options: ValidateOptions) => { + const results = await assertValidRules(options); // will throw on errors such as badly formatted rules + + if(results.duplicated.size > 0){ + log.warn('Found duplicate rules', Array.from(results.duplicated.values())); + } + + if(results.unmatched.size > 0){ + log.warn('Found rules which did not match any files', Array.from(results.unmatched.values())); + } +}; diff --git a/src/lib/logger/logger.ts b/src/lib/logger/logger.ts index 6faaa54..d7c6fe7 100644 --- a/src/lib/logger/logger.ts +++ b/src/lib/logger/logger.ts @@ -4,9 +4,9 @@ class Logger { console.error(msg, error); } - public warn(msg: string, error?: Error): void { + public warn(msg: string, obj?: Object): void { // tslint:disable-next-line:no-console - console.warn(msg, error); + console.warn(msg, obj); } } diff --git a/src/lib/ownership/index.ts b/src/lib/ownership/index.ts index 8cad2fb..ffc2460 100644 --- a/src/lib/ownership/index.ts +++ b/src/lib/ownership/index.ts @@ -1,4 +1,5 @@ export { OwnedFile } from './lib/OwnedFile'; export { OwnershipEngine } from './lib/OwnershipEngine'; export { getFileOwnership } from './file'; +export { validate } from './validate'; export { Matcher, FileOwnershipMatcher } from './types'; diff --git a/src/lib/ownership/lib/OwnershipEngine.test.ts b/src/lib/ownership/lib/OwnershipEngine.test.ts index bf76c1b..bc1d59e 100644 --- a/src/lib/ownership/lib/OwnershipEngine.test.ts +++ b/src/lib/ownership/lib/OwnershipEngine.test.ts @@ -14,11 +14,13 @@ describe('OwnershipEngine', () => { describe('calcFileOwnership', () => { const createFileOwnershipMatcher = (path: string, owners: string[]): FileOwnershipMatcher => { return { + rule: `${path} ${owners.join(' ')}`, path, owners, match: (testPath: string) => { return testPath === path; }, + matched: 0, }; }; @@ -40,6 +42,23 @@ describe('OwnershipEngine', () => { expect(result).toEqual(expectedOwners); }); + it('should count the number of times a rule is matched to a path', () => { + // Arrange + const owners = ['@owner-1', '@owner-2']; + const path = 'my/awesome/file.ts'; + const matcher = createFileOwnershipMatcher(path, owners); + + expect(matcher.matched).toEqual(0); + + const underTest = new OwnershipEngine([matcher]); + + // Act + underTest.calcFileOwnership(path); + + // Assert + expect(underTest.getRules()[0].matched).toEqual(1); + }); + it('should should take precedence from the last matching rule', () => { // Arrange const expectedOwner = '@owner-2'; diff --git a/src/lib/ownership/lib/OwnershipEngine.ts b/src/lib/ownership/lib/OwnershipEngine.ts index 58b67d5..d08342e 100644 --- a/src/lib/ownership/lib/OwnershipEngine.ts +++ b/src/lib/ownership/lib/OwnershipEngine.ts @@ -20,6 +20,7 @@ export class OwnershipEngine { for (const matcher of matchers) { if (matcher.match(filePath)) { + matcher.matched++; return matcher.owners; } } @@ -27,6 +28,17 @@ export class OwnershipEngine { return []; } + public getRules(): { rule: string, matched: number }[] { + const status: { rule: string, matched: number }[] = []; + + for (const matcher of this.matchers) { + status.push({ rule: matcher.rule, matched: matcher.matched }); + } + + return status; + } + + public static FromCodeownersFile(filePath: string) { try { const lines = fs.readFileSync(filePath).toString().split('\n'); @@ -49,7 +61,7 @@ export class OwnershipEngine { } } -const createMatcherCodeownersRule = (rule: string) => { +const createMatcherCodeownersRule = (rule: string): FileOwnershipMatcher => { // Split apart on spaces const parts = rule.split(/\s+/); @@ -61,8 +73,8 @@ const createMatcherCodeownersRule = (rule: string) => { // Remaining parts are expected to be team names (if any) if (parts.length > 1) { teamNames = parts.slice(1, parts.length); - for(const name of teamNames){ - if(!codeOwnerRegex.test(name)){ + for (const name of teamNames) { + if (!codeOwnerRegex.test(name)) { throw new Error(`${name} is not a valid owner name in rule ${rule}`); } } @@ -73,9 +85,11 @@ const createMatcherCodeownersRule = (rule: string) => { // Return our complete matcher return { + rule, path, owners: teamNames, match: match.ignores.bind(match), + matched: 0, }; }; diff --git a/src/lib/ownership/types.ts b/src/lib/ownership/types.ts index f4a1d71..8021b05 100644 --- a/src/lib/ownership/types.ts +++ b/src/lib/ownership/types.ts @@ -1,7 +1,9 @@ export type Matcher = (path: string) => boolean; export interface FileOwnershipMatcher { + rule: string; path: string; owners: string[]; match: Matcher; + matched: number; } diff --git a/src/lib/ownership/validate.ts b/src/lib/ownership/validate.ts new file mode 100644 index 0000000..9bc0373 --- /dev/null +++ b/src/lib/ownership/validate.ts @@ -0,0 +1,46 @@ +import { OwnershipEngine } from './lib/OwnershipEngine'; +import { readDirRecursively } from './lib/readDirRecursively'; + +interface ValidationResults { + duplicated: Set; + unmatched: Set; +} + +export const validate = async (options: { codeowners: string, dir: string, root?: string }): Promise => { + const engine = OwnershipEngine.FromCodeownersFile(options.codeowners); // Validates code owner file + + const filePaths = await readDirRecursively(options.dir, ['.git']); + + for (const file of filePaths) { + engine.calcFileOwnership(file); // Test each file against rule set + } + + const rules = engine.getRules(); + + const unique: Set = new Set(); + const duplicated: Set = new Set(); + const hasMatches: Set = new Set(); + const unmatched: Set = new Set(); + + for (const { rule, matched } of rules) { + if (!unique.has(rule)) { + unique.add(rule); + } else { + duplicated.add(rule); + } + + if (matched > 0) { + hasMatches.add(rule); + } else { + unmatched.add(rule); + } + } + + for (const rule of unmatched) { // Where we have duplicates we get an edge condition where one version of the matcher doesn't get hit - TODO - there is no doubt a nicer way to express this + if (hasMatches.has(rule)) { + unmatched.delete(rule); + } + } + + return { duplicated, unmatched }; +};