File level owners

.eslintrc

@@ -28,11 +28,12 @@
     "context": false,
     "global": false,
     "describes": true,
+    "Owner": false,
     "OwnersMap": false,
     "GitHubStatusPost": false,
-    "FileOwner": false,
-    "FileOwners": false,
-    "PullRequestInfo": false
+    "OwnerTuples": false,
+    "PullRequestInfo": false,
+    "RepoFileOwner": false
   },
   "rules": {
     "array-bracket-spacing": [2, "never"],

config.js

@@ -63,6 +63,7 @@ nconf
     SECRET: 'keyboardcat',
     SECRET_TOKEN: process.env.SECRET_TOKEN,
 
+    NODE_ENV: process.env.NODE_ENV || 'development',
     GITHUB_ACCESS_TOKEN: process.env.GITHUB_ACCESS_TOKEN || 'default',
     GITHUB_REPO_DIR: process.env.GITHUB_REPO_DIR || 'default',
     GITHUB_BOT_USERNAME: process.env.GITHUB_BOT_USERNAME || 'default',

declarations/all.js

@@ -27,15 +27,6 @@ type OwnersMap = {
   [key: string]: Owner
 }
 
-type FileOwner = {
-  owner: Owner,
-  files: RepoFile[]
-}
-
-type FileOwners = {
-  [key: string]: FileOwner
-}
-
 type GitHubStatusPost = {
   state: 'success' | 'error' | 'failure',
   target_url: string,
@@ -45,7 +36,22 @@ type GitHubStatusPost = {
 
 type PullRequestInfo = {
   pr: PullRequest,
-  fileOwners: FileOwners,
+  repoFiles:RepoFile[],
   reviews: Review[],
-  approvalsMet: boolean
+  approvalsMet: boolean,
+  ownerTuples: OwnerTuples
 }
+
+type RepoFileOwner = {
+  id: string,
+  type: 'file' | 'dir',
+  usernames: string[]
+}
+
+type OwnerTuple = {
+  type: 'file' | 'dir',
+  owner: Owner | string[],
+  files: RepoFile[]
+}
+
+type OwnerTuples = OwnerTuple[];

gulpfile.js

@@ -32,7 +32,7 @@ gulp.task('babel', () => {
 });
 
 gulp.task('test', () => {
-  return gulp.src(tests).pipe($$.ava({verbose: true, timeout: '15s'}));
+  return gulp.src(tests).pipe($$.ava({verbose: true, timeout: '20s'}));
 });
 
 gulp.task('flow', () => {
@@ -66,7 +66,7 @@ gulp.task('watch', function() {
 });
 
 gulp.task('watch:test', function() {
-  return $$.watch(tests, {ignoreInitial: false},
+  return $$.watch(sources.concat(tests), {ignoreInitial: false},
       $$.batch(function(events, done) {
         gulp.start('test', done);
       }));

routes/owners/api.js

@@ -16,11 +16,11 @@
 
 /* @flow */
 
-import * as bb from 'bluebird';
 import * as _ from 'lodash';
 import {Git} from '../../src/git';
 import {PullRequest} from '../../src/github';
-import {findOwners} from '../../src/owner';
+import {createAggregatedOwnersTuple} from '../../src/owner';
+import {RepoFile} from '../../src/repo-file';
 import * as express from 'express';
 const config = require('../../config');
 const GITHUB_BOT_USERNAME = config.get('GITHUB_BOT_USERNAME');
@@ -41,17 +41,10 @@ const prActionTypes: string[] = [
   'synchronize',
 ];
 
-function getOwners(pr: PullRequest) : Promise<FileOwners> {
+function getOwners(pr: PullRequest) : Promise<RepoFile[]> {
   return git.pullLatestForRepo(GITHUB_REPO_DIR, 'origin', 'master').then(() => {
-    const promises = bb.all([
-      pr.getFiles(),
-      git.getOwnersFilesForBranch(pr.author, GITHUB_REPO_DIR, 'master'),
-    ]);
-    return promises.then(function(res) {
-      const files = res[0];
-      const ownersMap = res[1];
-      return findOwners(files, ownersMap);
-    });
+    return git.getOwnersFilesForBranch(pr.author, GITHUB_REPO_DIR, 'master')
+        .then(ownersMap => pr.getFiles(ownersMap));
   });
 }
 
@@ -112,30 +105,32 @@ export function index(req: Object, res: Object) {
         .then(processPullRequest.bind(null, body));
   }
 
-  return promise.then(ok).catch(() => {
-    res.status(500).send('Something went wrong!');
+  return promise.then(ok).catch(e => {
+    if (process.env.NODE_ENV == 'production') {
+      res.status(500).send('Something went wrong!');
+    } else {
+      console.log(e.stack);
+      res.status(500).send(e.stack);
+    }
   });
 }
 
 function maybePostComment(prInfo: PullRequestInfo): Promise<*> {
-  const {pr, fileOwners} = prInfo;
+  const {pr, ownerTuples} = prInfo;
   // If all approvals are still not met, do we need to submit a new post?
   return pr.getLastApproversList(GITHUB_BOT_USERNAME).then(reviewers => {
 
-    const allFileOwnersUsernames = Object.keys(fileOwners).sort()
-        .map(key => {
-          const fileOwner = fileOwners[key];
-          const owner = fileOwner.owner;
-          return owner.dirOwners;
-        });
+    const allFileOwnersUsernames = ownerTuples.map(fileOwner => {
+      return fileOwner.owner.dirOwners;
+    });
 
     // If the list of reviewers from the last bot's comment is different
     // from the current evaluation of required reviewers then we need to
     // post the new list of reviewers. (this might be more than the number
     // of previously required reviewers or less, but in any case we need to
     // post the list again)
     if (!_.isEqual(reviewers, allFileOwnersUsernames)) {
-      return pr.postIssuesComment(pr.composeBotComment(fileOwners))
+      return pr.postIssuesComment(pr.composeBotComment(ownerTuples))
           .then(() => {
             return pr.setFailureStatus();
           });
@@ -145,10 +140,11 @@ function maybePostComment(prInfo: PullRequestInfo): Promise<*> {
 }
 
 function getPullRequestInfo(pr: PullRequest): Promise<PullRequestInfo> {
-  return getOwners(pr).then(fileOwners => {
+  return getOwners(pr).then(repoFiles => {
     return pr.getUniqueReviews().then(reviews => {
-      const approvalsMet = pr.areAllApprovalsMet(fileOwners, reviews);
-      return {pr, fileOwners, reviews, approvalsMet};
+      const approvalsMet = pr.areAllApprovalsMet(repoFiles, reviews);
+      const ownerTuples = createAggregatedOwnersTuple(repoFiles);
+      return {pr, repoFiles, reviews, approvalsMet, ownerTuples};
     });
   });
 }
@@ -171,11 +167,11 @@ function processPullRequest(body: Object,
 }
 
 function openedAction(prInfo: PullRequestInfo): Promise<*> {
-  const {pr, fileOwners, approvalsMet} = prInfo;
+  const {pr, approvalsMet, ownerTuples} = prInfo;
   if (approvalsMet) {
     return prInfo.pr.setApprovedStatus();
   }
-  return pr.postIssuesComment(pr.composeBotComment(fileOwners))
+  return pr.postIssuesComment(pr.composeBotComment(ownerTuples))
     .then(() => {
       return pr.setFailureStatus();
     });

src/git.js

@@ -49,7 +49,7 @@ export class Git {
    * Retrieves all the OWNERS paths inside a repository.
    */
   getOwnersFilesForBranch(author: string, dirPath: string,
-      targetBranch: string): OwnersMap {
+      targetBranch: string): Promise<OwnersMap> {
     // NOTE: for some reason `git ls-tree --full-tree -r HEAD **/OWNERS*
     // doesn't work from here.
     return exec(`cd ${dirPath} && git checkout ${targetBranch} ` +

src/github.js

@@ -87,7 +87,7 @@ export class PullRequest {
    * and pulls out the files that have been changed in any way
    * and returns type RepoFile[].
    */
-  getFiles(): Promise<RepoFile[]> {
+  getFiles(ownersMap: OwnersMap): Promise<RepoFile[]> {
     return request({
       url: `https://api.github.com/repos/${this.project}/${this.repo}/pulls/` +
           `${this.id}/files`,
@@ -96,7 +96,7 @@ export class PullRequest {
       headers,
     }).then(function(res: any) {
       const body = JSON.parse(res.body);
-      return body.map(item => new RepoFile(item.filename));
+      return body.map(item => new RepoFile(item.filename, ownersMap));
     });
   }
 
@@ -139,6 +139,9 @@ export class PullRequest {
   }
 
   postIssuesComment(body: string): Promise<*> {
+    if (process.env.NODE_ENV != 'production') {
+      return Promise.resolve();
+    }
     return request({
       url: `https://api.github.com/repos/${this.project}/${this.repo}/issues/` +
           `${this.id}/comments`,
@@ -151,7 +154,7 @@ export class PullRequest {
 
   getCommentsByAuthor(author: string): Promise<PullRequestComment[]> {
     return this.getComments().then(comments => {
-      return comments.filter(x => x.author === author);
+      return comments.filter(x => x.author == author);
     });
   }
 
@@ -197,19 +200,17 @@ export class PullRequest {
     });
   }
 
-  areAllApprovalsMet(fileOwners: FileOwners, reviews: Review[]): boolean {
+  areAllApprovalsMet(repoFiles: RepoFile[], reviews: Review[]): boolean {
     const reviewersWhoApproved = reviews.filter(x => {
       return x.state == 'approved';
     }).map(x => x.username);
     // If you're the author, then you yourself are assume to approve your own
     // PR.
     reviewersWhoApproved.push(this.author);
 
-    return Object.keys(fileOwners).every(path => {
-      const fileOwner = fileOwners[path];
-      const owner = fileOwner.owner;
-      _.intersection(owner.dirOwners, reviewersWhoApproved);
-      return _.intersection(owner.dirOwners, reviewersWhoApproved).length > 0;
+    return repoFiles.every(repoFile => {
+      return _.intersection(repoFile.findRepoFileOwner().usernames,
+          reviewersWhoApproved).length > 0;
     });
   }
 
@@ -242,19 +243,19 @@ export class PullRequest {
     });
   }
 
-  composeBotComment(fileOwners: FileOwners) {
+  composeBotComment(aggregatedOwners: OwnerTuples): string {
     let comment = 'Hi, ampproject bot here! Here are a list of the owners ' +
         'that can approve your files.\n\nYou may leave an issue comment ' +
         `stating "@${GITHUB_BOT_USERNAME} retry!" to force me to re-evaluate ` +
         'this Pull Request\'s status\n\n';
-    Object.keys(fileOwners).sort().forEach(key => {
-      const fileOwner = fileOwners[key];
+    aggregatedOwners.forEach(fileOwner => {
       const owner = fileOwner.owner;
       // Slice from char 2 to remove the ./ prefix normalization
       const files = fileOwner.files.map(x => `- ${x.path.slice(2)}`).join('\n');
       const usernames = '/to ' + owner.dirOwners.join(' ') + '\n';
       comment += usernames + files + '\n\n';
     });
+
     comment += '\n\nFor any issues please file a bug at ' +
         'https://github.com/google/github-owners-bot/issues';
     return comment;

src/owner.js

@@ -34,7 +34,7 @@ export class Owner {
   fullpath: string;
   score: number;
   dirOwners: string[];
-  fileOwners: Object;
+  fileOwners: {[key: string]: Set<string>};
 
   constructor(config: any, pathToRepoDir: string, filePath: string) {
     // We want it have the leading ./ to evaluate `.` later on
@@ -52,52 +52,30 @@ export class Owner {
     config.forEach(entry => {
       if (typeof entry === 'string') {
         this.dirOwners.push(entry);
-      } else if (entry && entry['file-only']) {
-        // TODO(erwin): support file level entries. Finalize spec for it.
+      } else if (typeof entry === 'object') {
+        const username = Object.keys(entry)[0];
+        const files = entry[username];
+        files.forEach(filepath => {
+          const path = `${this.dirname}/${filepath}`;
+          const fileOwners = this.fileOwners[path];
+          if (!fileOwners) {
+            this.fileOwners[path] = new Set([username]);
+          } else {
+            fileOwners.add(username);
+          }
+        });
       }
     });
     this.dirOwners.sort();
   }
-}
 
-/**
- * Returns a list of github usernames that can be "approvers" for the set
- * of files. It first tries to find the interection across the files and if
- * there are none it will return the union across usernames.
- */
-export function findOwners(files: RepoFile[],
-    ownersMap: OwnersMap): FileOwners {
-  const fileOwners: FileOwners = Object.create(null);
-  files.forEach((file : RepoFile) => {
-    const owner = findClosestOwnersFile(file, ownersMap);
-    if (!fileOwners[owner.dirname]) {
-      fileOwners[owner.dirname] = ({
-        owner,
-        files: [file],
-      } : FileOwner);
-    } else {
-      fileOwners[owner.dirname].files.push(file);
+  getFileLevelOwners(repoFile: RepoFile): ?string[] {
+    const fileOwners = this.fileOwners[repoFile.path];
+    if (fileOwners) {
+      return Array.from(fileOwners).sort();
     }
-  });
-  return fileOwners;
-}
-
-/**
- * Using the `ownersMap` key which is the path to the actual OWNER file
- * in the repo, we simulate a folder traversal by splitting the path and
- * finding the closest OWNER file for a RepoFile.
- */
-export function findClosestOwnersFile(file: RepoFile,
-    ownersMap: OwnersMap): Owner {
-  let dirname = file.dirname;
-  let owner = ownersMap[dirname];
-  const dirs = dirname.split(path.sep);
-
-  while (!owner && dirs.pop() && dirs.length) {
-    dirname = dirs.join(path.sep);
-    owner = ownersMap[dirname];
+    return null;
   }
-  return owner;
 }
 
 export function createOwnersMap(owners: Owner[]): OwnersMap {
@@ -108,3 +86,26 @@ export function createOwnersMap(owners: Owner[]): OwnersMap {
     return ownersMap;
   }, Object.create(null));
 }
+
+export function createAggregatedOwnersTuple(
+    repoFiles: RepoFile[]): OwnerTuples {
+  const aggregatedOwners = Object.create(null);
+
+  repoFiles.forEach(repoFile => {
+    const repoFileOwner = repoFile.findRepoFileOwner();
+    const id = repoFileOwner.id;
+
+    if (!aggregatedOwners[id]) {
+      aggregatedOwners[id] = {
+        type: repoFileOwner.type,
+        owner: repoFile.ownersMap[id],
+        files: [repoFile],
+      };
+    } else {
+      aggregatedOwners[id].files.push(repoFile);
+    }
+  });
+  return Object.keys(aggregatedOwners).sort().map(key => {
+    return aggregatedOwners[key];
+  });;
+}