[6.x] [precommit hook] add dev script (#14890) (#15065)

* [precommitHook] move to dev script

* [eslint] fix lint error

* [dev/eslint] do simply boolean checks first

* [dev] use shared REPO_ROOT constant

* [dev/File] precompute relative/ext of path

* [dev/eslint] fix relative import

* [dev/eslint] fix typos

* [grunt] remove unused run:eslintStaged config

* [dev/run] only create log if we are going to use it

* [dev/run/isFailError] ensure return value is Boolean

* [dev/precommitHook] use less intermediate vars

package.json

@@ -54,7 +54,7 @@
     "build": "grunt build",
     "release": "grunt release",
     "start": "sh ./bin/kibana --dev",
-    "precommit": "grunt precommit",
+    "precommit": "node scripts/precommit_hook",
     "karma": "karma start",
     "elasticsearch": "grunt esvm:dev:keepalive",
     "lint": "echo 'use `node scripts/eslint`' && false",
@@ -240,6 +240,7 @@
     "event-stream": "3.3.2",
     "expect.js": "0.3.1",
     "faker": "1.1.0",
+    "getopts": "2.0.0",
     "grunt": "1.0.1",
     "grunt-angular-translate": "0.3.0",
     "grunt-aws-s3": "0.14.5",

scripts/precommit_hook.js

@@ -0,0 +1,2 @@
+require('../src/babel-register');
+require('../src/dev/run_precommit_hook');

src/dev/constants.js

@@ -0,0 +1,3 @@
+import { dirname } from 'path';
+
+export const REPO_ROOT = dirname(require.resolve('../../package.json'));

src/dev/eslint/index.js

@@ -0,0 +1,3 @@
+export { DEFAULT_ESLINT_PATHS } from './default_eslint_paths';
+export { pickFilesToLint } from './pick_files_to_lint';
+export { lintFiles } from './lint_files';

src/dev/eslint/lint_files.js

@@ -0,0 +1,34 @@
+import { CLIEngine } from 'eslint';
+
+import { createFailError } from '../run';
+import { REPO_ROOT } from '../constants';
+
+/**
+ * Lints a list of files with eslint. eslint reports are written to the log
+ * and a FailError is thrown when linting errors occur.
+ *
+ * @param  {ToolingLog} log
+ * @param  {Array<File>} files
+ * @return {undefined}
+ */
+export function lintFiles(log, files) {
+  const cli = new CLIEngine({
+    cache: true,
+    cwd: REPO_ROOT,
+  });
+
+  const paths = files.map(file => file.getRelativePath());
+  const report = cli.executeOnFiles(paths);
+
+  const failTypes = [];
+  if (report.errorCount > 0) failTypes.push('errors');
+  if (report.warningCount > 0) failTypes.push('warning');
+
+  if (!failTypes.length) {
+    log.success('%d files linted successfully', files.length);
+    return;
+  }
+
+  log.error(cli.getFormatter()(report.results));
+  throw createFailError(`eslint ${failTypes.join(' & ')}`, 1);
+}

src/dev/eslint/pick_files_to_lint.js

@@ -0,0 +1,42 @@
+import { CLIEngine } from 'eslint';
+
+import { DEFAULT_ESLINT_PATHS } from './default_eslint_paths';
+
+/**
+ * Filters a list of files that should be linted. This is done by comparing the
+ * file name against the default eslint patterns (used when executing the eslint
+ * script without arguments) and then filtering those files by the eslint ignored
+ * paths in .eslintignore.
+ *
+ * When any JS file is ignored by either mechanism a warning message is logged.
+ *
+ * @param  {ToolingLog} log
+ * @param  {Array<File>} files
+ * @return {Array<File>}
+ */
+export function pickFilesToLint(log, files) {
+  const cli = new CLIEngine();
+  const sourcePathGlobs = cli.resolveFileGlobPatterns(DEFAULT_ESLINT_PATHS);
+
+  return files.filter(file => {
+    const isNormallyLinted = file.matchesAnyGlob(sourcePathGlobs);
+    const isExplicitlyIgnored = isNormallyLinted && cli.isPathIgnored(file.getRelativePath());
+
+    if (isNormallyLinted && !isExplicitlyIgnored) {
+      log.debug('linting %j', file);
+      return true;
+    }
+
+    // warn about modified JS files that are not being linted
+    if (!isNormallyLinted && file.isJs()) {
+      log.warning('%j not selected by src/dev/eslint/default_eslint_paths', file);
+    }
+
+    // warn about modified JS files that are explicitly excluded from linting
+    if (isExplicitlyIgnored && file.isJs()) {
+      log.warning(`%j ignored by .eslintignore`, file);
+    }
+
+    return false;
+  });
+}

src/dev/file.js

@@ -0,0 +1,39 @@
+import { resolve, relative, extname } from 'path';
+
+import minimatch from 'minimatch';
+
+import { REPO_ROOT } from './constants';
+
+export class File {
+  constructor(path) {
+    this._path = resolve(path);
+    this._relativePath = relative(REPO_ROOT, this._path);
+    this._ext = extname(this._path);
+  }
+
+  getRelativePath() {
+    return this._relativePath;
+  }
+
+  isJs() {
+    return this._ext === '.js';
+  }
+
+  matchesRegex(regex) {
+    return this._relativePath.match(regex);
+  }
+
+  matchesAnyGlob(globs) {
+    return globs.some(pattern => minimatch(this._relativePath, pattern, {
+      dot: true
+    }));
+  }
+
+  toString() {
+    return this._relativePath;
+  }
+
+  toJSON() {
+    return this._relativePath;
+  }
+}

src/dev/precommit_hook/check_file_casing.js

@@ -0,0 +1,54 @@
+import { createFailError } from '../run';
+
+const NON_SNAKE_CASE_RE = /[A-Z \-]/;
+const ALLOW_NON_SNAKE_CASE = [
+  'docs/**/*',
+  'packages/eslint-*/**/*',
+  'src/babel-*/**/*',
+  'tasks/config/**/*',
+];
+
+function listFileNames(files) {
+  return files
+    .map(file => ` - ${file.getRelativePath()}`)
+    .join('\n');
+}
+
+/**
+ * Check that all passed File objects are using valid casing. Every
+ * file SHOULD be using snake_case but some files are allowed to stray:
+ *
+ *  - grunt config: the file name needs to match the module name
+ *  - eslint/babel packages: the directory name matches the module
+ *    name, which uses kebab-case to mimic other babel/eslint plugins,
+ *    configs, and presets
+ *  - docs: unsure why, but all docs use kebab-case and that's fine
+ *
+ * @param {ToolingLog} log
+ * @param {Array<File>} files
+ * @return {Promise<undefined>}
+ */
+export async function checkFileCasing(log, files) {
+  const errors = [];
+  const warnings = [];
+
+  files.forEach(file => {
+    const invalid = file.matchesRegex(NON_SNAKE_CASE_RE);
+    const allowed = file.matchesAnyGlob(ALLOW_NON_SNAKE_CASE);
+    if (!invalid) {
+      log.debug('%j uses valid casing', file);
+    } else if (allowed) {
+      warnings.push(file);
+    } else {
+      errors.push(file);
+    }
+  });
+
+  if (warnings.length) {
+    log.warning(`Filenames SHOULD be snake_case.\n${listFileNames(warnings)}`);
+  }
+
+  if (errors.length) {
+    throw createFailError(`Filenames MUST use snake_case.\n${listFileNames(errors)}`);
+  }
+}

src/dev/precommit_hook/get_files_for_commit.js

@@ -0,0 +1,39 @@
+import SimpleGit from 'simple-git';
+import { fromNode as fcb } from 'bluebird';
+
+import { REPO_ROOT } from '../constants';
+import { File } from '../file';
+
+/**
+ * Get the files that are staged for commit (excluding deleted files)
+ * as `File` objects that are aware of their commit status.
+ *
+ * @param  {String} repoPath
+ * @return {Promise<Array<File>>}
+ */
+export async function getFilesForCommit() {
+  const simpleGit = new SimpleGit(REPO_ROOT);
+
+  const output = await fcb(cb => simpleGit.diff(['--name-status', '--cached'], cb));
+
+  return output
+    .split('\n')
+    // Ignore blank lines
+    .filter(line => line.trim().length > 0)
+    // git diff --name-status outputs lines with two OR three parts
+    // separated by a tab character
+    .map(line => line.trim().split('\t'))
+    .map(([status, ...paths]) => {
+      // ignore deleted files
+      if (status === 'D') {
+        return undefined;
+      }
+
+      // the status is always in the first column
+      // .. If the file is edited the line will only have two columns
+      // .. If the file is renamed it will have three columns
+      // .. In any case, the last column is the CURRENT path to the file
+      return new File(paths[paths.length - 1]);
+    })
+    .filter(Boolean);
+}

src/dev/precommit_hook/index.js

@@ -0,0 +1,2 @@
+export { checkFileCasing } from './check_file_casing';
+export { getFilesForCommit } from './get_files_for_commit';

src/dev/run/fail.js

@@ -0,0 +1,12 @@
+const FAIL_TAG = Symbol('fail error');
+
+export function createFailError(reason, exitCode = 1) {
+  const error = new Error(reason);
+  error.exitCode = exitCode;
+  error[FAIL_TAG] = true;
+  return error;
+}
+
+export function isFailError(error) {
+  return Boolean(error && error[FAIL_TAG]);
+}

src/dev/run/flags.js

@@ -0,0 +1,36 @@
+import { relative } from 'path';
+
+import getopts from 'getopts';
+
+export function getFlags(argv) {
+  return getopts(argv, {
+    alias: {
+      v: 'verbose',
+    },
+    default: {
+      verbose: false,
+      quiet: false,
+      silent: false,
+      debug: false,
+      help: false,
+    }
+  });
+}
+
+export function getHelp() {
+  return (
+    `
+  node ${relative(process.cwd(), process.argv[1], '.js')}
+
+  Runs a dev task
+
+  Options:
+    --verbose, -v      Log verbosely
+    --debug            Log debug messages (less than verbose)
+    --quiet            Only log errors
+    --silent           Don't log anything
+    --help             Show this message
+
+`
+  );
+}

src/dev/run/index.js

@@ -0,0 +1,2 @@
+export { run } from './run';
+export { createFailError } from './fail';

src/dev/run/run.js

@@ -0,0 +1,28 @@
+import { createToolingLog, pickLevelFromFlags } from '../tooling_log';
+import { isFailError } from './fail';
+import { getFlags, getHelp } from './flags';
+
+export async function run(body) {
+  const flags = getFlags(process.argv.slice(2));
+
+  if (flags.help) {
+    process.stderr.write(getHelp());
+    process.exit(1);
+  }
+
+  const log = createToolingLog(pickLevelFromFlags(flags));
+  log.pipe(process.stdout);
+
+  try {
+    await body({ log, flags });
+  } catch (error) {
+    if (isFailError(error)) {
+      log.error(error.message);
+      process.exit(error.exitCode);
+    } else {
+      log.error('UNHANDLED ERROR');
+      log.error(error);
+      process.exit(1);
+    }
+  }
+}

src/dev/run_eslint.js

@@ -1,5 +1,5 @@
 import { parse } from 'eslint/lib/options';
-import { DEFAULT_ESLINT_PATHS } from './default_eslint_paths';
+import { DEFAULT_ESLINT_PATHS } from './eslint';
 
 const options = parse(process.argv);
 
@@ -11,5 +11,5 @@ if (!process.argv.includes('--no-cache')) {
   process.argv.push('--cache');
 }
 
-// common-js is requires to that logic before this executes before loading eslint
+// common-js is required so that logic before this executes before loading eslint
 require('eslint/bin/eslint');

src/dev/run_precommit_hook.js

@@ -0,0 +1,10 @@
+import { run } from './run';
+
+import { lintFiles, pickFilesToLint } from './eslint';
+import { getFilesForCommit, checkFileCasing } from './precommit_hook';
+
+run(async ({ log }) => {
+  const files = await getFilesForCommit();
+  await checkFileCasing(log, files);
+  await lintFiles(log, pickFilesToLint(log, files));
+});

src/dev/tooling_log/__tests__/log.js

@@ -4,9 +4,9 @@ import Chance from 'chance';
 import {
   createConcatStream,
   createPromiseFromStreams
-} from '../../streams';
+} from '../../../utils';
 
-import { createToolingLog } from '../';
+import { createToolingLog } from '../tooling_log';
 
 const chance = new Chance();
 const capture = (level, block) => {

src/dev/tooling_log/index.js

@@ -0,0 +1,2 @@
+export { createToolingLog } from './tooling_log';
+export { pickLevelFromFlags } from './log_levels';

src/dev/tooling_log/log_levels.js

@@ -8,6 +8,14 @@ const LEVELS = [
   'verbose',
 ];
 
+export function pickLevelFromFlags(flags) {
+  if (flags.verbose) return 'verbose';
+  if (flags.debug) return 'debug';
+  if (flags.quiet) return 'error';
+  if (flags.silent) return 'silent';
+  return 'info';
+}
+
 export function parseLogLevel(name) {
   const i = LEVELS.indexOf(name);
 

src/dev/tooling_log/tooling_log.js

@@ -1,7 +1,7 @@
 import { format } from 'util';
 import { PassThrough } from 'stream';
 
-import { magenta, yellow, red, blue, brightBlack } from 'ansicolors';
+import { magenta, yellow, red, blue, green, brightBlack } from 'ansicolors';
 
 import { parseLogLevel } from './log_levels';
 
@@ -33,6 +33,11 @@ export function createToolingLog(initialLogLevelName = 'silent') {
       this.write(' %s ', blue('info'), format(...args));
     }
 
+    success(...args) {
+      if (!logLevel.flags.info) return;
+      this.write(' %s ', green('succ'), format(...args));
+    }
+
     warning(...args) {
       if (!logLevel.flags.warning) return;
       this.write(' %s ', yellow('warn'), format(...args));

src/es_archiver/cli.js

@@ -12,7 +12,7 @@ import { Command } from 'commander';
 import elasticsearch from 'elasticsearch';
 
 import { EsArchiver } from './es_archiver';
-import { createToolingLog } from '../utils';
+import { createToolingLog } from '../dev';
 import { readConfigFile } from '../functional_test_runner';
 
 const cmd = new Command('node scripts/es_archiver');

src/es_archiver/lib/__tests__/stats.js

@@ -3,9 +3,8 @@ import { uniq } from 'lodash';
 import sinon from 'sinon';
 
 import { createStats } from '../';
-
+import { createToolingLog } from '../../../dev';
 import {
-  createToolingLog,
   createConcatStream,
   createPromiseFromStreams
 } from '../../../utils';

src/functional_test_runner/__tests__/integration/with_es_archiver.js

@@ -3,7 +3,8 @@ import { resolve } from 'path';
 import { format as formatUrl } from 'url';
 
 import { readConfigFile } from '../../lib';
-import { createToolingLog, createReduceStream } from '../../../utils';
+import { createToolingLog } from '../../../dev';
+import { createReduceStream } from '../../../utils';
 import { createEsTestCluster } from '../../../test_utils/es';
 import { startupKibana } from '../lib';
 

src/functional_test_runner/cli.js

@@ -2,7 +2,7 @@ import { resolve } from 'path';
 
 import { Command } from 'commander';
 
-import { createToolingLog } from '../utils';
+import { createToolingLog } from '../dev';
 import { createFunctionalTestRunner } from './functional_test_runner';
 
 const cmd = new Command('node scripts/functional_test_runner');

src/functional_test_runner/lib/config/__tests__/read_config_file.js

@@ -1,6 +1,6 @@
 import expect from 'expect.js';
 
-import { createToolingLog } from '../../../../utils';
+import { createToolingLog } from '../../../../dev';
 import { readConfigFile } from '../read_config_file';
 import { Config } from '../config';
 

src/ui/ui_settings/create_or_upgrade_saved_config/__tests__/create_or_upgrade_integration.js

@@ -3,7 +3,7 @@ import expect from 'expect.js';
 
 import { createEsTestCluster } from '../../../../test_utils/es';
 import { createServerWithCorePlugins } from '../../../../test_utils/kbn_server';
-import { createToolingLog } from '../../../../utils';
+import { createToolingLog } from '../../../../dev';
 import { createOrUpgradeSavedConfig } from '../create_or_upgrade_saved_config';
 
 describe('createOrUpgradeSavedConfig()', () => {

src/utils/index.js

@@ -6,7 +6,6 @@ export { pkg } from './package_json';
 export { unset } from './unset';
 export { encodeQueryComponent } from './encode_query_component';
 export { modifyUrl } from './modify_url';
-export { createToolingLog } from './tooling_log';
 export { getFlattenedObject } from './get_flattened_object';
 
 export {

tasks/check_added_filenames.js

@@ -1,19 +0,0 @@
-import { isAdded, getFilename } from './utils/files_to_commit';
-
-export default function registerCheckAddedFilenames(grunt) {
-  grunt.registerTask('checkAddedFilenames', function () {
-    grunt.task.requires('collectFilesToCommit');
-
-    const invalid = grunt.config
-      .get('filesToCommit')
-      .map(getFilename)
-      .filter(isAdded)
-      .filter(name => !name.match(/([\/\\]|^)(docs|tasks[\/\\]config|webpackShims)([\/\\]|$)/))
-      .filter(name => name.match(/[A-Z \-]/))
-      .reduce((all, name) => `${all}  ${name}\n`, '');
-
-    if (invalid) {
-      grunt.fail.fatal(`Filenames must use snake_case.\n${invalid}`);
-    }
-  });
-}

tasks/collect_files_to_commit.js

@@ -1,14 +0,0 @@
-import filesToCommit from './utils/files_to_commit';
-
-export default function registerCollectFilesToCommit(grunt) {
-  const root = grunt.config.get('root');
-
-  grunt.registerTask('collectFilesToCommit', function () {
-    filesToCommit(root)
-      .then(files => {
-        grunt.log.ok(`${files.length} files with changes to commit`);
-        grunt.config.set('filesToCommit', files);
-      })
-      .nodeify(this.async());
-  });
-}

tasks/config/copy.js

@@ -14,6 +14,7 @@ module.exports = function () {
         '!src/ui_framework/doc_site/**',
         '!src/es_archiver/**',
         '!src/functional_test_runner/**',
+        '!src/dev/**',
         'bin/**',
         'ui_framework/components/**',
         'ui_framework/services/**',

tasks/config/run.js

@@ -41,14 +41,6 @@ module.exports = function (grunt) {
       ]
     },
 
-    eslintStaged: {
-      cmd: process.execPath,
-      args: [
-        require.resolve('../../scripts/eslint'),
-        // staged paths are written here by lintStagedFiles task
-      ]
-    },
-
     testServer: {
       options: {
         wait: false,

tasks/functional_test_runner.js

@@ -1,5 +1,5 @@
 import { createFunctionalTestRunner } from '../src/functional_test_runner';
-import { createToolingLog } from '../src/utils';
+import { createToolingLog } from '../src/dev';
 
 export default function (grunt) {
   grunt.registerMultiTask('functional_test_runner', 'run tests with the functional test runner', function () {

tasks/lint_staged_files.js

@@ -1,53 +0,0 @@
-
-import { extname, resolve, relative } from 'path';
-import { isStaged, getFilename } from './utils/files_to_commit';
-import { CLIEngine } from 'eslint';
-import { red, blue } from 'ansicolors';
-import minimatch from 'minimatch';
-
-import { DEFAULT_ESLINT_PATHS } from '../src/dev/default_eslint_paths';
-
-const root = resolve(__dirname, '..');
-
-export default function (grunt) {
-  grunt.registerTask('lintStagedFiles', function () {
-    grunt.task.requires('collectFilesToCommit');
-
-    // convert eslint paths to globs
-    const cli = new CLIEngine();
-    const sourcePathGlobs = cli.resolveFileGlobPatterns(DEFAULT_ESLINT_PATHS);
-
-    const files = grunt.config
-      .get('filesToCommit')
-      .filter(isStaged)
-      .map(getFilename)
-      .map(file => relative(root, resolve(file))) // resolve to pwd, then get relative from the root
-      .filter(file => {
-        if (!sourcePathGlobs.some(glob => minimatch(file, glob))) {
-          if (extname(file) === '.js') {
-            grunt.log.writeln(`${red('WARNING:')} ${file} not selected by src/eslint/default_eslint_paths`);
-          }
-          return false;
-        }
-
-        if (cli.isPathIgnored(file)) {
-          if (extname(file) === '.js') {
-            grunt.log.writeln(`${blue('DEBUG:')} ${file} ignored by .eslintignore`);
-          }
-          return false;
-        }
-
-        return true;
-      });
-
-    if (files.length) {
-      const args = grunt.config.get('run.eslintStaged.args');
-      grunt.config.set('run.eslintStaged.args', [
-        ...args,
-        ...files
-      ]);
-
-      grunt.task.run(['run:eslintStaged']);
-    }
-  });
-}

tasks/precommit.js

@@ -1,7 +0,0 @@
-export default function (grunt) {
-  grunt.registerTask('precommit', [
-    'collectFilesToCommit',
-    'checkAddedFilenames',
-    'lintStagedFiles'
-  ]);
-}

tasks/utils/files_to_commit.js

@@ -1,43 +0,0 @@
-import SimpleGit from 'simple-git';
-import { promisify } from 'bluebird';
-
-export default function filesToCommit(path) {
-  const simpleGit = new SimpleGit(path);
-  const gitDiff = promisify(simpleGit.diff, simpleGit);
-
-  return gitDiff(['--name-status', '--cached'])
-    .then(output => {
-      return output
-        .split('\n')
-        .filter(line => line.trim().length > 0) // Ignore blank lines
-        .map(line => line.trim().split('\t'))
-        .map(parts => {
-          const status = parts[0];
-          // If a file's been edited, it will only have 2 elements. If it's been renamed it will have
-          // 3 elements. But in both cases, the last element is the current name of the file.
-          const name = parts[parts.length - 1];
-          return { status, name };
-        })
-        .filter(file => file.status !== 'D'); // Ignore deleted files
-    });
-}
-
-export function getFilename(file) {
-  return file.name;
-}
-
-export function isAdded(file) {
-  return file.status === 'A';
-}
-
-export function isDeleted(file) {
-  return file.status === 'D';
-}
-
-export function isUnmerged(file) {
-  return file.status === 'U';
-}
-
-export function isStaged(file) {
-  return !isDeleted(file) && !isUnmerged(file);
-}