From cc8fe0ae41dc8ef98ef552260b3c0c2737d0c42f Mon Sep 17 00:00:00 2001 From: Tim Foster Date: Thu, 28 Jun 2018 10:49:29 +0100 Subject: [PATCH 1/3] Add -L option to list open CRs --- lib/cli.js | 13 ++++++++- lib/grr.js | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++- package.json | 1 + 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 7a58195..49b840e 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -5,7 +5,7 @@ */ /* - * Copyright 2016 Joyent, Inc. + * Copyright 2018 Joyent, Inc. */ /* @@ -83,6 +83,14 @@ var options = [ type: 'bool', help: 'Delete the local branch used for working with the CR. ' + 'Typically this is used when you are done with the CR.' + }, + { + group: 'Miscellaneous CR operations' + }, + { + names: ['list', 'L'], + type: 'bool', + help: 'List outstanding code reviews for this component.' } ]; @@ -101,6 +109,7 @@ function usage() { 'Usage:', ' grr [] [] # create or update a CR', ' grr -D [] # delete feature branch, switch to master', + ' grr -L # list outstanding CRs for this component.', '', 'Options:', help, @@ -179,6 +188,8 @@ function main(argv) { issueArg: issueArg, dryRun: opts.dry_run }, mainFinish); + } else if (opts['list']) { + grr.grrList({log: log,}, mainFinish); } else { grr.grrUpdateOrCreate({ log: log, diff --git a/lib/grr.js b/lib/grr.js index fd3d8f7..89b34af 100644 --- a/lib/grr.js +++ b/lib/grr.js @@ -5,7 +5,7 @@ */ /* - * Copyright 2017 Joyent, Inc. + * Copyright 2018 Joyent, Inc. */ /* @@ -19,6 +19,7 @@ var request = require('request'); var strsplit = require('strsplit'); var vasync = require('vasync'); var VError = require('verror').VError; +var extsprintf = require('extsprintf').sprintf; var common = require('./common'); var config = require('./config'); @@ -1159,6 +1160,58 @@ function stepCreateUpdateCr(arg, cb) { }); } +/* + * List at most 500 open code reviews for the current repository. + */ +function stepListCRs(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.string(arg.repoName, 'arg.repoName'); + assert.string(arg.gerritUsername, 'arg.gerritUsername'); + assert.func(cb, 'cb'); + + common.forkExecWaitAndLog({ + argv: ['ssh', '-q', '-o', 'StrictHostKeyChecking=no', + '-o', 'UserKnownHostsFile=/dev/null', '-p', '29418', + arg.gerritUsername + '@cr.joyent.us', + 'gerrit', 'query', '--format=JSON', + 'project:' + arg.repoName, 'status:open', + 'limit:500'], + log: arg.log + }, function (err, res) { + if (err) { + cb(err); + return; + } + var lines = res.stdout.trim().split(/\n/g); + if (lines.length == 1) { + console.log( + "No outstanding code reviews for " + arg.repoName + '.'); + return + } + + var fmt = '%-26s%-27s%-30s%s'; + console.log(extsprintf(fmt, 'CREATED', 'URL', 'AUTHOR', 'SYNOPSIS')); + for (var i=0; i < lines.length ; i++) { + var element = lines[i]; + var res = JSON.parse(element); + // the last bit of js from gerrit is a row-count + status + if (res.url == null) { + continue; + } + var created = new Date(0); + created.setUTCSeconds(res.createdOn); + var subject = res.subject; + var SUBJ_LIMIT = 80; + if (subject.length > SUBJ_LIMIT) { + subject = subject.slice(0, SUBJ_LIMIT) + '...'; + } + console.log( + extsprintf( + fmt, created.toISOString(), res.url, res.owner['email'], + subject)); + } + }); +} // ---- exports /* @@ -1220,6 +1273,28 @@ function grrDelete(opts, cb) { ]}, cb); } +/* + * List open CRs for the current repository. Intended for use by engineers + * who want to help cleaning up any CR backlog that may exist. + */ +function grrList(opts, cb) { + assert.object(opts, 'opts'); + assert.object(opts.log, 'opts.log'); + assert.func(cb, 'cb'); + + var context = { + log: opts.log, + }; + + vasync.pipeline({arg: context, funcs: [ + stepGetGrrConfig, + stepGetGitRemoteUrlOrigin, + stepGetGerritUsername, + stepGetRepoName, + stepListCRs + ]}, cb); +} + function grrUpdateOrCreate(opts, cb) { assert.object(opts, 'opts'); assert.object(opts.log, 'opts.log'); @@ -1450,6 +1525,7 @@ function grrUpdateOrCreate(opts, cb) { module.exports = { grrDelete: grrDelete, + grrList: grrList, grrUpdateOrCreate: grrUpdateOrCreate }; diff --git a/package.json b/package.json index 39bdd1c..bdfad78 100644 --- a/package.json +++ b/package.json @@ -8,6 +8,7 @@ "assert-plus": "1.x", "bunyan": "1.x", "dashdash": "1.x", + "extsprintf": "^1.3.0", "forkexec": "1.x", "request": "2.74.x", "strsplit": "1.x", From 8d0acf708ecd93af99dbf517e121701687fcdece Mon Sep 17 00:00:00 2001 From: Tim Foster Date: Mon, 18 Feb 2019 14:49:05 +0000 Subject: [PATCH 2/3] Add -r [-d] option to list open reviews and diffs --- lib/cli.js | 43 +++++++++--- lib/grr.js | 201 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 225 insertions(+), 19 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index 49b840e..5c1c3a2 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -21,6 +21,7 @@ var VError = require('verror').VError; var grr = require('./grr'); var pkg = require('../package.json'); +var VALID_REVIEWS = ['cr', 'ia']; // ---- globals and constants @@ -49,18 +50,28 @@ var options = [ { group: 'For creating/updating CRs' }, - //{ - // names: ['dry-run', 'n'], - // type: 'bool', - // help: 'Go through the motions without actually commiting or pushing.', - // 'default': false - //}, + { + names: ['dry-run', 'n'], + type: 'bool', + help: 'Go through the motions without actually commiting, pushing or' + + ' reviewing.', + 'default': false + }, { names: ['parenthetical', 'p'], type: 'string', help: 'Add a parenthetical comment to the commit message. Typically ' + 'used for followup commits on an issue already commited to.' }, + { + names: ['review', 'r'], + type: 'string', + help: 'Add either "cr" or "ia" +1s to any matching issues.' + }, + { names: ['diff', 'd'], + type: 'bool', + help: 'When using -r, show the latest diffs of reviews.' + }, { names: ['update', 'u'], type: 'bool', @@ -110,6 +121,7 @@ function usage() { ' grr [] [] # create or update a CR', ' grr -D [] # delete feature branch, switch to master', ' grr -L # list outstanding CRs for this component.', + ' grr -r [cr|ia] # add CR or IA +1s to matching issues.', '', 'Options:', help, @@ -121,7 +133,8 @@ function usage() { ' grr TOOLS-123 # Start or update a branch and CR for this ticket', //' grr --commit TOOLS-123 # ... also commit uncommited changes', ' grr # Update the current CR (using the cached ticket)', - ' grr -D # All done. Del the branch and switch back to master' + ' grr -D # All done. Del the branch and switch back to master', + ' grr -r cr [-d] TOOLS-123 # Add +1 CRs for TOOLS-123 tickets' ].join('\n')); /* END JSSTYLED */ } @@ -186,10 +199,22 @@ function main(argv) { grr.grrDelete({ log: log, issueArg: issueArg, - dryRun: opts.dry_run + dryRun: opts.dry_run, }, mainFinish); } else if (opts['list']) { - grr.grrList({log: log,}, mainFinish); + grr.grrList({log: log}, mainFinish); + } else if (opts['review']) { + if (VALID_REVIEWS.lastIndexOf(opts['review']) == -1) { + console.error('grr: unknown review type'); + process.exit(1); + } + grr.grrReview({ + log: log, + issueArg: issueArg, + dryRun: opts.dry_run, + reviewArg: opts.review, + diffArg: opts.diff, + verbose: opts.verbose}, mainFinish); } else { grr.grrUpdateOrCreate({ log: log, diff --git a/lib/grr.js b/lib/grr.js index 89b34af..a514aa7 100644 --- a/lib/grr.js +++ b/lib/grr.js @@ -20,6 +20,7 @@ var strsplit = require('strsplit'); var vasync = require('vasync'); var VError = require('verror').VError; var extsprintf = require('extsprintf').sprintf; +var mod_url = require('url'); var common = require('./common'); var config = require('./config'); @@ -36,6 +37,10 @@ var JIRA_URL = 'https://jira.joyent.us'; var USER_AGENT = 'grr/' + pkg.version; +// Hardcoded for Joyent eng usage right now. +var GERRIT_HOST = 'cr.joyent.us'; +// XXX timf: use my private instance instead +// var GERRIT_HOST = 'kgerrit'; // ---- internal support functions @@ -196,10 +201,10 @@ function stepGetGerritUsername(arg, cb) { forkExecWait({ argv: ['ssh', '-q', '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', - '-p', '29418', 'cr.joyent.us'] + '-p', '29418', GERRIT_HOST] }, function (err, res) { if (res && res.stderr) { - var re = /ssh:\/\/(.*?)@cr.joyent.us\/REPOSITORY_NAME.git/; + var re = /ssh:\/\/(.*?)@.*\/REPOSITORY_NAME.git/; var match = re.exec(res.stderr); if (match) { arg.gerritUsername = match[1]; @@ -317,7 +322,10 @@ function stepGetRepoName(arg, cb) { var patterns = [ new RegExp('github\.com:([^:]+?)(\.git)?$'), - new RegExp('github\.com\/(.+?)(\.git)?$') + new RegExp('github\.com\/(.+?)(\.git)?$'), + // XXX timf, only needed for my private gerrit instance which doesn't + // have .git suffices on the repos. Remove before putback. + new RegExp('.*\/(.+)?(\.git)$') ]; for (var i = 0; i < patterns.length; i++) { @@ -1067,6 +1075,9 @@ function stepCreateUpdateCr(arg, cb) { * remote: https://cr.joyent.us/272 TOOLS-1516 t... */ var re = /^remote: https:\/\/cr\.joyent\.us\/(\d+) /m; + // XXX timf: hack, my private instance isn't using https + // remove before putback. + // var re = /^remote: http:\/\/kgerrit:8080\/c\/(.*)\/(\d+) /m; var match = re.exec(res.stderr); if (!match) { next(new VError('could not determine created CR ' @@ -1172,7 +1183,7 @@ function stepListCRs(arg, cb) { common.forkExecWaitAndLog({ argv: ['ssh', '-q', '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', '-p', '29418', - arg.gerritUsername + '@cr.joyent.us', + arg.gerritUsername + '@' + GERRIT_HOST, '-p', '29418', 'gerrit', 'query', '--format=JSON', 'project:' + arg.repoName, 'status:open', 'limit:500'], @@ -1205,6 +1216,11 @@ function stepListCRs(arg, cb) { if (subject.length > SUBJ_LIMIT) { subject = subject.slice(0, SUBJ_LIMIT) + '...'; } + // XXX timf: my private gerrit instance doesn't have emails set for + // all users. Remove before putback. + if (res.owner['email'] == null) { + res.owner['email'] = ''; + } console.log( extsprintf( fmt, created.toISOString(), res.url, res.owner['email'], @@ -1212,6 +1228,142 @@ function stepListCRs(arg, cb) { } }); } + +/* + * List open code reviews for *any* repository that match our issue along with + * diffs showing the latest revision. Eventually, ask the user whether they + * want to apply a given CR +1 type to each review, respecting any --dry-run + * option passed. XXX for now, we only list matching reviews and optionally + * their diffs. + */ +function stepQueryCRs(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.optionalBool(arg.dryRun, 'arg.dryRun'); + assert.optionalBool(arg.verbose, 'arg.verbose'); + assert.optionalBool(arg.diffArg, 'arg.diffArg'); + assert.string(arg.issueArg, 'arg.issueArg'); + // should probably futher verify that this is one of our known + // review type values, 'cr' or 'ia' + assert.string(arg.reviewArg, 'arg.reviewArg'); + assert.string(arg.gerritUsername, 'arg.gerritUsername'); + assert.object(arg.grrConfig, 'arg.grrConfig'); + assert.func(cb, 'cb'); + + common.forkExecWaitAndLog({ + argv: ['ssh', '-q', '-o', 'StrictHostKeyChecking=no', + '-o', 'UserKnownHostsFile=/dev/null', '-p', '29418', + arg.gerritUsername + '@cr.joyent.us', + 'gerrit', 'query', '--format=JSON', + '--all-approvals', '--current-patch-set', + 'status:open', + 'intopic:' + arg.issueArg, 'OR', 'message:' + arg.issueArg], + log: arg.log + }, function (err, res) { + if (err) { + cb(err); + return; + } + var lines = res.stdout.trim().split(/\n/g); + if (lines.length == 1) { + console.log( + 'No outstanding code reviews matching ' + arg.issueArg + '.'); + return; + } + + var fmt = '%-26s%-26s%-27s%-25s%-4s%-4s%s'; + console.log(extsprintf(fmt, 'CREATED', 'PROJECT', 'URL', 'AUTHOR', 'CR', 'IA', 'SUBJECT')); + for (var i=0; i < lines.length ; i++) { + var element = lines[i]; + var cr_res = JSON.parse(element); + // the last bit of js from gerrit is a row-count + status which + // has an empty url field. + if (cr_res.url == null) { + continue; + } + + // gather data on the code reviews for this ticket (if any) + var created = new Date(0); + created.setUTCSeconds(cr_res.createdOn); + var subject = cr_res.subject; + var SUBJ_LIMIT = 80; + var cr = 'no'; + var ia = 'no'; + if (subject.length > SUBJ_LIMIT) { + subject = subject.slice(0, SUBJ_LIMIT) + '...'; + } + var approvals = cr_res.currentPatchSet.approvals; + if (approvals !== undefined) { + for (var j=0; j< approvals.length; j++) { + var approve = approvals[j]; + if (approve.type == 'Integration-Approval' && + approve.value == 1) { + ia = 'yes'; + } else if (approve.type == 'Code-Review' && + approve.value == 1) { + cr = 'yes'; + } + } + } + + var review_summary = extsprintf( + fmt, created.toISOString(), cr_res.project, cr_res.url, + cr_res.owner['email'], cr, ia, subject); + if (!arg.diffArg) { + // emit a summary of the status of the review + console.log(review_summary); + } else { + // I'd like to list diffs after each status line, so perhaps + // the answer here is to merely gather the status data for now, + // then have another pipeline stage to gather the diff data, + // and a final stage to do all the printing? + var revision = cr_res.currentPatchSet.revision; + var url = mod_url.parse(cr_res.url); + var gerrit_url = url.protocol + '//' + url.host; + var patch_url = gerrit_url + '/changes/' + cr_res.number + + '/revisions/' + revision + '/patch?download'; + request.get(patch_url, { + auth: { + username: arg.gerritUsername, + password: arg.grrConfig.jira.password + } + }, function (err, res, body) { + if (err) { + cb(new VError(err, 'could not retrieve JIRA patch %s', + patch_url)); + return; + } else if (res.statusCode === 404) { + cb(new VError(err, 'no such JIRA patch %s', patch_url)); + return; + } else if (res.statusCode !== 200) { + cb(new VError('unexpected JIRA response status for patch %s: %s', + patch_url, res.statusCode)); + return; + } + try { + var buf = Buffer.from(body, 'base64'); + console.log(review_summary); + console.log('patch:'); + console.log(buf.toString('utf8')); + cb(); + } catch (parseErr) { + cb(parseErr); + return; + } + }); + } + // XXX timf The plan at this point, is that we'd now prompt the user + // with a "Do you want to apply this review? (y/n)" question, after + // which we'd proceed to apply the +1 CR or IA. We'd include a + // --force or --yes option to assume 'y', or honor the --dry-run/-n + // option to do nothing. My async js-fu is starting to hurt here, + // because I can't tell whether all of that that should be done as + // separate pipeline steps, passing data between stages. + // Either way, the notion of nesting this into further function + // calls to do all that here feels really messy :-( ) + + } + }); +} // ---- exports /* @@ -1237,7 +1389,6 @@ function grrDelete(opts, cb) { vasync.pipeline({arg: context, funcs: [ stepGetGitRemoteUrlOrigin, - stepGetRepoName, stepValidateIssueArg, stepGetGrrConfig, @@ -1295,6 +1446,32 @@ function grrList(opts, cb) { ]}, cb); } +function grrReview(opts, cb) { + /* + * Search for a grr issue, and add CR/IA flags to all issues + */ + assert.object(opts, 'opts'); + assert.object(opts.log, 'opts.log'); + assert.func(cb, 'cb'); + assert.string(opts.issueArg, 'opts.issueArg'); + assert.string(opts.reviewArg, 'opts.reviewArg'); + assert.optionalBool(opts.dryRun, 'opts.dryRun'); + + var context = { + log: opts.log, + issueArg: opts.issueArg, + dryRun: opts.dryRun, + diffArg: opts.diffArg, + reviewArg: opts.reviewArg + }; + + vasync.pipeline({arg: context, funcs: [ + stepGetGrrConfig, + stepGetGerritUsername, + stepQueryCRs + ]}, cb); +} + function grrUpdateOrCreate(opts, cb) { assert.object(opts, 'opts'); assert.object(opts.log, 'opts.log'); @@ -1486,8 +1663,8 @@ function grrUpdateOrCreate(opts, cb) { switch (arg.crAction) { case 'none': if (arg.branchConfigCr) { - console.log('CR unchanged: %s ', - arg.branchConfigCr, arg.branchConfigCr); + console.log('CR unchanged: %s ', + arg.branchConfigCr, GERRIT_HOST, arg.branchConfigCr); } else { console.log('\nMake commits and run `grr` in this branch ' + 'to create a CR.'); @@ -1495,23 +1672,26 @@ function grrUpdateOrCreate(opts, cb) { break; case 'updateCommitMessage': console.log('CR commit message updated: %s patchset %s ' - + '', + + '', arg.branchConfigCr, arg.currentCr.currentPatchSet.number, + GERRIT_HOST, arg.branchConfigCr); break; case 'create': console.log('CR created: %s patchset %s ' - + '', + + '', arg.branchConfigCr, arg.currentCr.currentPatchSet.number, + GERRIT_HOST, arg.branchConfigCr); break; case 'update': console.log('CR updated: %s patchset %s ' - + '', + + '', arg.branchConfigCr, arg.currentCr.currentPatchSet.number, + GERRIT_HOST, arg.branchConfigCr); break; default: @@ -1526,6 +1706,7 @@ function grrUpdateOrCreate(opts, cb) { module.exports = { grrDelete: grrDelete, grrList: grrList, + grrReview: grrReview, grrUpdateOrCreate: grrUpdateOrCreate }; From 75056524e637f0aa83623ab643cc0286a7e30748 Mon Sep 17 00:00:00 2001 From: Tim Foster Date: Mon, 11 Mar 2019 14:50:02 +0000 Subject: [PATCH 3/3] Iterate over reviews, prompting for IA/CR, applying result to gerrit --- CHANGES.md | 9 +++ lib/cli.js | 10 +-- lib/grr.js | 183 +++++++++++++++++++++++++++++++++++++-------------- package.json | 1 + 4 files changed, 150 insertions(+), 53 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 39f1475..1b6f401 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,15 @@ ## not yet released +## 1.8.0 + +- grr -r support to vote on Code-Review or Integration-Approval + on projects + +## 1.7.0 + +- grr -L support to list outstanding reviews for this project + ## 1.6.0 - Update grr to use the new https://jira.joyent.us diff --git a/lib/cli.js b/lib/cli.js index 5c1c3a2..2bf5ae1 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -21,7 +21,9 @@ var VError = require('verror').VError; var grr = require('./grr'); var pkg = require('../package.json'); -var VALID_REVIEWS = ['cr', 'ia']; +var VALID_REVIEWS = { + 'cr': 'Code-Review', + 'ia': 'Integration-Approval'}; // ---- globals and constants @@ -199,12 +201,12 @@ function main(argv) { grr.grrDelete({ log: log, issueArg: issueArg, - dryRun: opts.dry_run, + dryRun: opts.dry_run }, mainFinish); } else if (opts['list']) { grr.grrList({log: log}, mainFinish); } else if (opts['review']) { - if (VALID_REVIEWS.lastIndexOf(opts['review']) == -1) { + if (!VALID_REVIEWS.hasOwnProperty(opts['review'])) { console.error('grr: unknown review type'); process.exit(1); } @@ -212,7 +214,7 @@ function main(argv) { log: log, issueArg: issueArg, dryRun: opts.dry_run, - reviewArg: opts.review, + reviewArg: VALID_REVIEWS[opts.review], diffArg: opts.diff, verbose: opts.verbose}, mainFinish); } else { diff --git a/lib/grr.js b/lib/grr.js index a514aa7..980ff62 100644 --- a/lib/grr.js +++ b/lib/grr.js @@ -20,6 +20,7 @@ var strsplit = require('strsplit'); var vasync = require('vasync'); var VError = require('verror').VError; var extsprintf = require('extsprintf').sprintf; +var prompt = require('prompt'); var mod_url = require('url'); var common = require('./common'); @@ -1075,9 +1076,6 @@ function stepCreateUpdateCr(arg, cb) { * remote: https://cr.joyent.us/272 TOOLS-1516 t... */ var re = /^remote: https:\/\/cr\.joyent\.us\/(\d+) /m; - // XXX timf: hack, my private instance isn't using https - // remove before putback. - // var re = /^remote: http:\/\/kgerrit:8080\/c\/(.*)\/(\d+) /m; var match = re.exec(res.stderr); if (!match) { next(new VError('could not determine created CR ' @@ -1188,25 +1186,25 @@ function stepListCRs(arg, cb) { 'project:' + arg.repoName, 'status:open', 'limit:500'], log: arg.log - }, function (err, res) { + }, function (err, query_res) { if (err) { cb(err); return; } - var lines = res.stdout.trim().split(/\n/g); + var lines = query_res.stdout.trim().split(/\n/g); if (lines.length == 1) { console.log( - "No outstanding code reviews for " + arg.repoName + '.'); - return + 'No outstanding code reviews for ' + arg.repoName + '.'); + return; } var fmt = '%-26s%-27s%-30s%s'; console.log(extsprintf(fmt, 'CREATED', 'URL', 'AUTHOR', 'SYNOPSIS')); - for (var i=0; i < lines.length ; i++) { + for (var i = 0; i < lines.length; i++) { var element = lines[i]; var res = JSON.parse(element); // the last bit of js from gerrit is a row-count + status - if (res.url == null) { + if (res.url === null) { continue; } var created = new Date(0); @@ -1218,7 +1216,7 @@ function stepListCRs(arg, cb) { } // XXX timf: my private gerrit instance doesn't have emails set for // all users. Remove before putback. - if (res.owner['email'] == null) { + if (res.owner['email'] === null) { res.owner['email'] = ''; } console.log( @@ -1230,25 +1228,18 @@ function stepListCRs(arg, cb) { } /* - * List open code reviews for *any* repository that match our issue along with - * diffs showing the latest revision. Eventually, ask the user whether they - * want to apply a given CR +1 type to each review, respecting any --dry-run - * option passed. XXX for now, we only list matching reviews and optionally - * their diffs. + * Search for open code reviews for *any* repository that match our issue */ function stepQueryCRs(arg, cb) { assert.object(arg.log, 'arg.log'); assert.optionalBool(arg.dryRun, 'arg.dryRun'); assert.optionalBool(arg.verbose, 'arg.verbose'); - assert.optionalBool(arg.diffArg, 'arg.diffArg'); assert.string(arg.issueArg, 'arg.issueArg'); - // should probably futher verify that this is one of our known - // review type values, 'cr' or 'ia' - assert.string(arg.reviewArg, 'arg.reviewArg'); assert.string(arg.gerritUsername, 'arg.gerritUsername'); assert.object(arg.grrConfig, 'arg.grrConfig'); assert.func(cb, 'cb'); + arg.grrReviews = []; common.forkExecWaitAndLog({ argv: ['ssh', '-q', '-o', 'StrictHostKeyChecking=no', '-o', 'UserKnownHostsFile=/dev/null', '-p', '29418', @@ -1269,18 +1260,40 @@ function stepQueryCRs(arg, cb) { 'No outstanding code reviews matching ' + arg.issueArg + '.'); return; } - - var fmt = '%-26s%-26s%-27s%-25s%-4s%-4s%s'; - console.log(extsprintf(fmt, 'CREATED', 'PROJECT', 'URL', 'AUTHOR', 'CR', 'IA', 'SUBJECT')); - for (var i=0; i < lines.length ; i++) { + for (var i = 0; i < lines.length; i++) { var element = lines[i]; var cr_res = JSON.parse(element); // the last bit of js from gerrit is a row-count + status which // has an empty url field. - if (cr_res.url == null) { + if (!cr_res.hasOwnProperty('url')) { continue; + } else { + arg.grrReviews.push(cr_res); } + } + cb(); + }); +} +/* + * List reviews for those CRs + */ +function stepReviewCRs(arg, cb) { + assert.object(arg.grrReviews); + // should probably futher verify that this is one of our known + // review type values, 'cr' or 'ia' + assert.string(arg.reviewArg, 'arg.reviewArg'); + assert.optionalBool(arg.diffArg, 'arg.diffArg'); + + var fmt = '%-26s%-26s%-27s%-25s%-4s%-4s%s'; + console.log(extsprintf( + fmt, 'CREATED', 'PROJECT', 'URL', 'AUTHOR', 'CR', 'IA', 'SUBJECT')); + + // Start a pipeline iterating over all reviews, prompting for user feedback + vasync.forEachPipeline({ + inputs: arg.grrReviews, + func: function processReview(argv, next) { + var cr_res = argv; // gather data on the code reviews for this ticket (if any) var created = new Date(0); created.setUTCSeconds(cr_res.createdOn); @@ -1293,7 +1306,7 @@ function stepQueryCRs(arg, cb) { } var approvals = cr_res.currentPatchSet.approvals; if (approvals !== undefined) { - for (var j=0; j< approvals.length; j++) { + for (var j = 0; j < approvals.length; j++) { var approve = approvals[j]; if (approve.type == 'Integration-Approval' && approve.value == 1) { @@ -1308,14 +1321,31 @@ function stepQueryCRs(arg, cb) { var review_summary = extsprintf( fmt, created.toISOString(), cr_res.project, cr_res.url, cr_res.owner['email'], cr, ia, subject); + console.log(review_summary); + if (!arg.diffArg) { - // emit a summary of the status of the review - console.log(review_summary); + // We explicity do not let users approve reviews without + // showing them a diff, so just continue now. + return next(); } else { - // I'd like to list diffs after each status line, so perhaps - // the answer here is to merely gather the status data for now, - // then have another pipeline stage to gather the diff data, - // and a final stage to do all the printing? + // Before we go further, check that if CR has not been granted, + // do not allow an IA to be granted. + if (arg.reviewArg == 'Integration-Approval') { + var has_cr = -1; + if (approvals !== undefined) { + for (var k = 0; k < approvals.length; k++) { + var approval = approvals[k]; + if (approval.type == 'Code-Review') { + has_cr = approval.value; + } + } + } + if (has_cr === -1) { + return next(new VError( + 'cannot grant IA when CR has not been granted')); + } + } + var revision = cr_res.currentPatchSet.revision; var url = mod_url.parse(cr_res.url); var gerrit_url = url.protocol + '//' + url.host; @@ -1328,39 +1358,93 @@ function stepQueryCRs(arg, cb) { } }, function (err, res, body) { if (err) { - cb(new VError(err, 'could not retrieve JIRA patch %s', + next(new VError(err, 'could not retrieve JIRA patch %s', patch_url)); return; } else if (res.statusCode === 404) { - cb(new VError(err, 'no such JIRA patch %s', patch_url)); + next(new VError(err, 'no such JIRA patch %s', + patch_url)); return; } else if (res.statusCode !== 200) { - cb(new VError('unexpected JIRA response status for patch %s: %s', + next(new VError( + 'unexpected JIRA response status for patch %s: %s', patch_url, res.statusCode)); return; } try { var buf = Buffer.from(body, 'base64'); - console.log(review_summary); console.log('patch:'); console.log(buf.toString('utf8')); - cb(); + if (arg.dryRun) { + console.log('Dry run, will not prompt for review.'); + return next(); + } + var review_desc = extsprintf( + 'Do you want to give a \'%s\' +1 for this review?', + arg.reviewArg); + // prompt for user input, and apply CR + var prompt_schema = { + properties: { + answer: { + description: review_desc, + pattern: /[y|n]/, + message: 'y or n will do!', + required: true + } + } + }; + prompt.colors = false; + prompt.message = ''; + prompt.start(); + prompt.get( + prompt_schema, + function user_input(prompt_err, result) { + if (prompt_err) { + next(new VError( + prompt_err, + 'problem trying to prompt user')); + return; + } + if (result.answer == 'y') { + console.log(extsprintf( + 'Granting a \'%s\' +1 to this review', + arg.reviewArg)); + common.forkExecWaitAndLog({ + argv: ['ssh', '-q', + '-o', 'StrictHostKeyChecking=no', + '-o', 'UserKnownHostsFile=/dev/null', + '-p', '29418', + arg.gerritUsername + '@cr.joyent.us', + 'gerrit', 'review', + '--project', cr_res.project, + '--label', arg.reviewArg + '=+1', + cr_res.currentPatchSet.revision + ], + log: arg.log + // eslint-disable-next-line no-unused-vars + }, function (fork_err, review_res) { + if (fork_err) { + next(fork_err); + return; + } + console.log(review_res); + }); + } else { + console.log('skipping this review.'); + } + next(); + }); } catch (parseErr) { - cb(parseErr); + next(parseErr); return; } }); } - // XXX timf The plan at this point, is that we'd now prompt the user - // with a "Do you want to apply this review? (y/n)" question, after - // which we'd proceed to apply the +1 CR or IA. We'd include a - // --force or --yes option to assume 'y', or honor the --dry-run/-n - // option to do nothing. My async js-fu is starting to hurt here, - // because I can't tell whether all of that that should be done as - // separate pipeline steps, passing data between stages. - // Either way, the notion of nesting this into further function - // calls to do all that here feels really messy :-( ) - + } + }, function (pipelineErr) { + if (pipelineErr) { + arg.log.trace({err: pipelineErr}, 'pipelineErr'); + cb(pipelineErr); } }); } @@ -1434,7 +1518,7 @@ function grrList(opts, cb) { assert.func(cb, 'cb'); var context = { - log: opts.log, + log: opts.log }; vasync.pipeline({arg: context, funcs: [ @@ -1468,7 +1552,8 @@ function grrReview(opts, cb) { vasync.pipeline({arg: context, funcs: [ stepGetGrrConfig, stepGetGerritUsername, - stepQueryCRs + stepQueryCRs, + stepReviewCRs ]}, cb); } diff --git a/package.json b/package.json index bdfad78..58999e1 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ "dashdash": "1.x", "extsprintf": "^1.3.0", "forkexec": "1.x", + "prompt": "1.0.0", "request": "2.74.x", "strsplit": "1.x", "toml": "2.3.x",