diff --git a/.gitignore b/.gitignore index 99ba8f9..4b7c027 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ /tmp /npm-debug.log /joyent-grr-*.tgz +/package-lock.json diff --git a/TODO.txt b/TODO.txt index 6f39fe1..95dc648 100644 --- a/TODO.txt +++ b/TODO.txt @@ -2,7 +2,6 @@ # TODO - grr option to adopt an existing CR and patchset -- grr support for *multiple* tickets - `grr --integrate,-I` that checks for approvals, does the integration, and then calls `grrDelete`. - change to being an *option*, makes it clearer how to update diff --git a/docs/example.md b/docs/example.md index 4e9aa1c..726b91a 100644 --- a/docs/example.md +++ b/docs/example.md @@ -67,7 +67,7 @@ CR created: 275 ``` It squashes all the commits (in a separate branch used only for this push) -and pushed to gerrit. Now make more commits and `grr` again to update: +and pushes to gerrit. Now make more commits and `grr` again to update: ``` [trentm@danger0:~/tm/play (grr-TOOLS-1516)] @@ -116,8 +116,31 @@ It cached the CR number from earlier, so does the right thing on push. If you look at , you'll notice that the commit message format is handled for you. -When you are done, use `grr -D` to clean up (remove the branch) and it pops -you back to `master`: +If we now end up fixing another issue as part of this CR, we can add it to our +commit message with `grr -a`: + +``` +[trentm@danger0:~/tm/play (grr-TOOLS-1516)] +$ grr -a TOOLS-1517 +... +``` + +When you have the requisite +1's on gerrit, run `grr` one more time to update +the commit message with the correct `Reviewed-by` and `Approved-by` lines: + +``` +[trentm@danger0:~/tm/play (grr-TOOLS-1516)] +$ grr +... +``` + +Be aware: if you try to update the commit message and rebase at the same time, +gerrit gets confused, and drops all of the +1s from the CR. + +You can now *Submit* in gerrit to integrate. + +Finally, use `grr -D` to clean up (remove the branch), which pops you back to +`master`: ``` [trentm@danger0:~/tm/play (grr-TOOLS-1516)] @@ -129,4 +152,4 @@ Deleting local grr branch "grr-TOOLS-1516" [trentm@danger0:~/tm/play (master)] $ -``` \ No newline at end of file +``` diff --git a/lib/cli.js b/lib/cli.js index fe3a1e9..52bcecf 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -68,6 +68,16 @@ var options = [ + 'was changed.', 'default': false }, + { + names: ['add-extra', 'a'], + type: 'string', + help: 'Add the given comma-separated list of issues to the commit.' + }, + { + names: ['remove-extra', 'r'], + type: 'string', + help: 'Remove the given comma-separated list of issues from the commit.' + }, // TODO //{ // names: ['commit', 'c'], @@ -76,7 +86,7 @@ var options = [ // 'default': false //}, { - group: 'When done your CR' + group: 'When done with your CR' }, { names: ['delete', 'D'], @@ -121,6 +131,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 -a TOOLS-124 # Add an extra issue to the current CR', + ' grr -r TOOLS-124 # Remove an extra issue from the current CR', ' grr -D # All done. Del the branch and switch back to master' ].join('\n')); /* END JSSTYLED */ @@ -183,6 +195,11 @@ function main(argv) { } if (opts['delete']) { + if (opts.add_extra || opts.remove_extra) { + console.error('extra issues specified when deleting'); + process.exit(1); + } + grr.grrDelete({ log: log, issueArg: issueArg, @@ -195,6 +212,8 @@ function main(argv) { log: log, issueArg: issueArg, parenthetical: opts.parenthetical, + addExtraIssuesArg: opts.add_extra, + removeExtraIssuesArg: opts.remove_extra, dryRun: opts.dry_run, commit: opts.commit, update: opts.update diff --git a/lib/common.js b/lib/common.js index 582d996..51eccfa 100644 --- a/lib/common.js +++ b/lib/common.js @@ -5,7 +5,7 @@ */ /* - * Copyright 2016 Joyent, Inc. + * Copyright 2019 Joyent, Inc. */ var assert = require('assert-plus'); @@ -16,6 +16,14 @@ var VError = require('verror').VError; // ---- support stuff +/// A split with sensible semantics for forEach(). +function fsplit(str, separator) { + return str.split(separator).filter(function (s) { + if (s !== '') + return s; + }); +} + function objCopy(obj, target) { assert.object(obj, 'obj'); assert.optionalObject(obj, 'target'); @@ -114,6 +122,7 @@ function forkExecWaitAndLog(opts, cb) { //---- exports module.exports = { + fsplit: fsplit, objCopy: objCopy, deepObjCopy: deepObjCopy, tildeSync: tildeSync, diff --git a/lib/grr.js b/lib/grr.js index eb1c22c..400aaf7 100644 --- a/lib/grr.js +++ b/lib/grr.js @@ -21,6 +21,7 @@ var vasync = require('vasync'); var VError = require('verror').VError; var extsprintf = require('extsprintf').sprintf; +var fsplit = require('./common').fsplit; var common = require('./common'); var config = require('./config'); var pkg = require('../package.json'); @@ -40,13 +41,13 @@ var USER_AGENT = 'grr/' + pkg.version; // ---- internal support functions /* - * `issueArg` is the user-given argument. Validate it and normalize + * `issueArg` is a user-given argument. Validate it and normalize * to these fields: * - * - arg.issueType is one of "jira" or "github". - * - arg.issueName is of the form "FOO-123" for JIRA and "account/project#123" + * - issue.issueType is one of "jira" or "github". + * - issue.issueName is of the form "FOO-123" for JIRA and "account/project#123" * for GitHub. - * - arg.issueId is of the form "FOO-123" for JIRA and "123" for GitHub. + * - issue.issueId is of the form "FOO-123" for JIRA and "123" for GitHub. * * Supported inputs: * - A Jira issue key: @@ -60,85 +61,104 @@ var USER_AGENT = 'grr/' + pkg.version; * - A GitHub issue URL: * https://github.com/trentm/node-bunyan/issues/426 */ -function stepValidateIssueArg(arg, cb) { - assert.object(arg.log, 'arg.log'); - assert.optionalString(arg.issueArg, 'arg.issueArg'); - assert.string(arg.repoName, 'arg.repoName'); +function parseIssueArg(issue, repoName, issueArg, cb) { + assert.object(issue, 'issue'); + assert.string(repoName, 'repoName'); + assert.string(issueArg, 'issueArg'); + assert.func(cb, 'cb'); var match; - if (!arg.issueArg) { + // FOO-123 + match = /^[A-Z]+-\d+$/.exec(issueArg); + if (match) { + issue.issueType = 'jira'; + issue.issueId = issue.issueName = issueArg; cb(); return; } - // FOO-123 - match = /^[A-Z]+-\d+$/.exec(arg.issueArg); + // https://jira.joyent.us/browse/FOO-123 + match = /^https:\/\/jira\.joyent\.us\/browse\/([A-Z]+-\d+)$/.exec(issueArg); if (match) { - arg.issueType = 'jira'; - arg.issueId = arg.issueName = arg.issueArg; + issue.issueType = 'jira'; + issue.issueId = issue.issueName = match[1]; + cb(); + return; } - if (!arg.issueId) { - // https://jira.joyent.us/browse/FOO-123 - match = /^https:\/\/jira\.joyent\.us\/browse\/([A-Z]+-\d+)$/ - .exec(arg.issueArg); - if (match) { - arg.issueType = 'jira'; - arg.issueId = arg.issueName = match[1]; - } + // 123 + match = /^\d+$/.exec(issueArg); + if (match) { + issue.issueType = 'github'; + issue.issueName = repoName + '#' + issueArg; + issue.issueId = issueArg; + cb(); + return; } - if (!arg.issueId) { - // 123 - match = /^\d+$/.exec(arg.issueArg); - if (match) { - arg.issueType = 'github'; - arg.issueName = arg.repoName + '#' + arg.issueArg; - arg.issueId = arg.issueArg; + // account/project#123 + match = /^([\w_-]+\/[\w_-]+)#(\d+)$/.exec(issueArg); + if (match) { + issue.issueType = 'github'; + issue.issueName = issueArg; + issue.issueId = match[2]; + if (match[1] !== repoName) { + cb(new VError( + 'project from , %s, does not match repo name, %s', + match[1], repoName)); + return; } + cb(); + return; } - if (!arg.issueId) { - // account/project#123 - match = /^([\w_-]+\/[\w_-]+)#(\d+)$/.exec(arg.issueArg); - if (match) { - arg.issueType = 'github'; - arg.issueName = arg.issueArg; - arg.issueId = match[2]; - if (match[1] !== arg.repoName) { - cb(new VError( - 'project from , %s, does not match repo name, %s', - match[1], arg.repoName)); - } + // https://github.com/trentm/node-bunyan/issues/426 + match = /^https:\/\/github\.com\/([\w_-]+\/[\w_-]+)\/issues\/(\d+)$/ + .exec(issueArg); + if (match) { + issue.issueType = 'github'; + issue.issueName = match[1] + '#' + match[2]; + issue.issueId = match[2]; + if (match[1] !== repoName) { + cb(new VError( + 'project from , %s, does not match repo name, %s', + match[1], repoName)); + return; } + + cb(); + return; } + cb(new VError('invalid arg: ' + issueArg)); +} - if (!arg.issueId) { - // https://github.com/trentm/node-bunyan/issues/426 - match = /^https:\/\/github\.com\/([\w_-]+\/[\w_-]+)\/issues\/(\d+)$/ - .exec(arg.issueArg); - if (match) { - arg.issueType = 'github'; - arg.issueName = match[1] + '#' + match[2]; - arg.issueId = match[2]; - if (match[1] !== arg.repoName) { - cb(new VError( - 'project from , %s, does not match repo name, %s', - match[1], arg.repoName)); - } - } +/* + * Validate the main issue arg provided. + */ +function stepValidateIssueArg(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.optionalString(arg.issueArg, 'arg.issueArg'); + assert.string(arg.repoName, 'arg.repoName'); + assert.func(cb, 'cb'); + + if (!arg.issueArg) { + cb(); + return; } - if (arg.issueId) { + parseIssueArg(arg, arg.repoName, arg.issueArg, function (err) { + if (err) { + cb(err); + return; + } + arg.log.trace({issueArg: arg.issueArg, issueType: arg.issueType, issueId: arg.issueId, issueName: arg.issueName}, 'stepValidateIssueArg'); cb(); - } else { - cb(new VError('invalid arg: ' + arg.issueArg)); - } + }); } function stepGetGrrConfig(arg, cb) { @@ -420,6 +440,155 @@ function stepGetBranchConfigIssue(arg, cb) { }); } +function stepGetBranchConfigExtraIssues(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.string(arg.gitBranch, 'arg.gitBranch'); + + getBranchConfigKey('extraIssues', arg, function (err, val) { + if (err) { + cb(err); + return; + } + + fsplit(val, ',').forEach(function (id, _) { + arg.extraIssues.push({ issueId: id }); + }); + + cb(); + }); +} + +function getIssueTitle(arg, issue, cb) { + assert.object(arg.log, 'arg.log'); + assert.object(issue, 'issue'); + assert.func(cb, 'cb'); + + if (issue.issueType === 'jira') { + // TODO: Attempt to scrape from public bugview. + + if (!arg.grrConfig.jira || !arg.grrConfig.jira.username || + !arg.grrConfig.jira.password) + { + cb(new VError( + 'cannot retrieve issue %s info: missing config ' + + 'in "%s":\n' + + ' [jira]\n' + + ' username = ""\n' + + ' password = ""\n' + + 'Note: Be sure to escape double-quotes and ' + + 'backslases in your password per\n' + + ' https://github.com/toml-lang/toml#string', + issue.issueName, config.CONFIG_FILE)); + return; + } + + getJiraIssue(arg, issue, function (err, issueInfo) { + if (err) { + cb(err); + return; + } + cb(null, issueInfo.fields.summary); + }); + } else { + assert.equal(issue.issueType, 'github'); + getGitHubIssue(arg, issue, function (err, issueInfo) { + if (err) { + cb(err); + return; + } + cb(null, issueInfo.title); + }); + } +} + + +function stepRemoveExtraIssues(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.string(arg.removeExtraIssuesArg, 'arg.removeExtraIssuesArg'); + assert.array(arg.extraIssues, 'arg.extraIssues'); + assert.func(cb, 'cb'); + + var remove = []; + + fsplit(arg.removeExtraIssuesArg, ',').forEach(function (item, _) { + remove.push({ issueId: item }); + }); + + vasync.forEachPipeline({ + inputs: remove, + func: function (issue, next) { + parseIssueArg(issue, arg.repoName, issue.issueId, function (err) { + if (err) { + next(err); + return; + } + + if (issue.issueId === arg.issueId) { + next(new VError('cannot remove main issue ' + arg.issueId)); + return; + } + + arg.extraIssues = arg.extraIssues.filter(function (item) { + return item.issueId !== issue.issueId; + }); + + next(); + }); + } + }, cb); +} + + +function stepParseExtraIssues(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.string(arg.addExtraIssuesArg, 'arg.addExtraIssuesArg'); + assert.string(arg.issueId, 'arg.issueId'); + assert.array(arg.extraIssues, 'arg.extraIssues'); + assert.func(cb, 'cb'); + + fsplit(arg.addExtraIssuesArg, ',').forEach(function (item, _) { + arg.extraIssues.push({ issueId: item }); + }); + + var seen_already = {}; + + vasync.forEachParallel({ + inputs: arg.extraIssues, func: function (issue, next) { + parseIssueArg(issue, arg.repoName, issue.issueId, function (err) { + if (err) { + next(err); + return; + } + + if (issue.issueId === arg.issueId) { + next(new VError('cannot add main issue ' + arg.issueId + + ' as an extra issue')); + return; + } + + if (seen_already.hasOwnProperty(issue.issueId)) { + next(new VError('duplicate extra issue ' + issue.issueId)); + return; + } + + seen_already[issue.issueId] = true; + + getIssueTitle(arg, issue, function (err2, title) { + if (err2) { + next(err2); + return; + } + + issue.issueTitle = title; + arg.log.trace({extraIssue: issue}); + next(); + }); + }); + } + }, cb); +} + + function stepGetBranchConfigTitle(arg, cb) { assert.object(arg.log, 'arg.log'); assert.string(arg.gitBranch, 'arg.gitBranch'); @@ -561,6 +730,18 @@ function stepSetBranchConfigTitle(arg, cb) { setBranchConfigKey('title', arg.issueTitle, arg, cb); } +function stepSetBranchConfigExtraIssues(arg, cb) { + assert.object(arg.log, 'arg.log'); + assert.array(arg.extraIssues, 'arg.extraIssues'); + assert.func(cb, 'cb'); + + arg.branchConfigExtraIssues = arg.extraIssues.map(function (issue) { + return issue.issueId; } + ).join(','); + + setBranchConfigKey('extraIssues', arg.branchConfigExtraIssues, arg, cb); +} + function stepSetBranchConfigParenthetical(arg, cb) { assert.object(arg.log, 'arg.log'); assert.string(arg.branchConfigParenthetical, @@ -582,15 +763,15 @@ function stepSetBranchConfigParenthetical(arg, cb) { * curl -i -u trent.mick \ * https://jira.joyent.us/rest/api/2/issue/TOOLS-1516 | json */ -function getJiraIssue(opts, cb) { +function getJiraIssue(opts, issue, cb) { assert.object(opts.log, 'opts.log'); assert.object(opts.grrConfig, 'opts.grrConfig'); assert.string(opts.grrConfig.jira.username, 'opts.grrConfig.jira.username'); assert.string(opts.grrConfig.jira.password, 'opts.grrConfig.jira.password'); - assert.string(opts.issueId, 'opts.issueId'); + assert.string(issue.issueId, 'issue.issueId'); var url = JIRA_URL + '/rest/api/2/issue/' - + encodeURIComponent(opts.issueId); + + encodeURIComponent(issue.issueId); request.get(url, { auth: { username: opts.grrConfig.jira.username, @@ -603,11 +784,11 @@ function getJiraIssue(opts, cb) { opts.issueId)); return; } else if (res.statusCode === 404) { - cb(new VError(err, 'no such JIRA issue %s', opts.issueId)); + cb(new VError(err, 'no such JIRA issue %s', issue.issueId)); return; } else if (res.statusCode !== 200) { cb(new VError('unexpected JIRA response status for issue %s: %s', - opts.issueId, res.statusCode)); + issue.issueId, res.statusCode)); return; } try { @@ -623,15 +804,15 @@ function getJiraIssue(opts, cb) { /* * Example: https://api.github.com/repos/trentm/play/issues/6 */ -function getGitHubIssue(opts, cb) { +function getGitHubIssue(opts, issue, cb) { assert.object(opts.log, 'opts.log'); assert.string(opts.repoName, 'opts.repoName'); - assert.equal(opts.issueType, 'github'); - assert.string(opts.issueId, 'opts.issueId'); - assert.string(opts.issueName, 'opts.issueName'); + assert.equal(issue.issueType, 'github'); + assert.string(issue.issueId, 'issue.issueId'); + assert.string(issue.issueName, 'issue.issueName'); var url = 'https://api.github.com/repos/' - + opts.repoName + '/issues/' + opts.issueId; + + opts.repoName + '/issues/' + issue.issueId; request.get(url, { headers: { // https://developer.github.com/v3/#user-agent-required @@ -641,14 +822,14 @@ function getGitHubIssue(opts, cb) { opts.log.trace({err: err, res: res}, 'getGitHubIssue response'); if (err) { cb(new VError(err, 'could not retrieve GitHub issue %s info', - opts.issueName)); + issue.issueName)); return; } else if (res.statusCode === 404) { - cb(new VError(err, 'no such GitHub issue %s', opts.issueName)); + cb(new VError(err, 'no such GitHub issue %s', issue.issueName)); return; } else if (res.statusCode !== 200) { cb(new VError('unexpected GitHub response status for issue %s: %s', - opts.issueName, res.statusCode)); + issue.issueName, res.statusCode)); return; } try { @@ -878,16 +1059,21 @@ function stepGetCurrentCr(arg, cb) { function stepGetCommitMessage(arg, cb) { assert.string(arg.issueName, 'arg.issueName'); assert.string(arg.issueTitle, 'arg.issueTitle'); - assert.optionalString(arg.issueTitle, 'arg.issueTitle'); assert.optionalString(arg.branchConfigParenthetical, 'arg.branchConfigParenthetical'); assert.optionalObject(arg.currentCr, 'arg.currentCr'); + assert.array(arg.extraIssues, 'arg.extraIssues'); + assert.func(cb, 'cb'); var msg = arg.issueName + ' ' + arg.issueTitle; if (arg.branchConfigParenthetical) { msg += ' (' + arg.branchConfigParenthetical + ')'; } + arg.extraIssues.forEach(function (item, _) { + msg += '\n' + item.issueName + ' ' + item.issueTitle; + }); + var approvals = (arg.currentCr && arg.currentCr.currentPatchSet && arg.currentCr.currentPatchSet.approvals); if (approvals) { @@ -921,7 +1107,8 @@ function stepGetCommitMessage(arg, cb) { * - determine correct commit message: * (a) possibly update title from the issue (if `-u` used) * (b) from local git config values - * (c) gathered "Reviewed by" from gerrit + * (c) look up any extra issues + * (d) gather "Reviewed by" from gerrit * - squash merge and/or ammend commit message, if necessary * - push to gerrit * - parse out the CR num and set local git config: @@ -1230,6 +1417,11 @@ function grrDelete(opts, cb) { return; } + if (opts.addExtraIssuesArg || opts.removeExtraIssuesArg) { + cb(new VError('extra issues specified when deleting')); + return; + } + var context = { log: opts.log, dryRun: opts.dryRun @@ -1300,6 +1492,9 @@ function grrUpdateOrCreate(opts, cb) { assert.object(opts, 'opts'); assert.object(opts.log, 'opts.log'); assert.optionalString(opts.issueArg, 'opts.issueArg'); + assert.optionalString(opts.addExtraIssuesArg, 'opts.addExtraIssuesArg'); + assert.optionalString(opts.removeExtraIssuesArg, + 'opts.removeExtraIssuesArg'); assert.optionalString(opts.parenthetical, 'opts.parenthetical'); assert.optionalBool(opts.dryRun, 'opts.dryRun'); assert.optionalBool(opts.commit, 'opts.commit'); @@ -1319,6 +1514,9 @@ function grrUpdateOrCreate(opts, cb) { log: opts.log, dryRun: opts.dryRun, issueArg: opts.issueArg, + extraIssues: [], + addExtraIssuesArg: opts.addExtraIssuesArg || '', + removeExtraIssuesArg: opts.removeExtraIssuesArg || '', parenthetical: opts.parenthetical }; @@ -1335,6 +1533,8 @@ function grrUpdateOrCreate(opts, cb) { // arlier failure stepGetBranchConfigIssue, + stepGetBranchConfigExtraIssues, + stepGetGerritUsername, stepGetGitRemotes, stepGetGitRemoteUrlCr, @@ -1358,76 +1558,38 @@ function grrUpdateOrCreate(opts, cb) { + 'this branch, use `grr `')); } } else if (!arg.issueId) { - // Set `issue{Id,Name,Type}` from branchConfigIssue. See - // `stepValidateIssueArg` for details on these fields. - if (/^\d+$/.test(arg.branchConfigIssue)) { - arg.issueType = 'github'; - arg.issueId = arg.branchConfigIssue; - arg.issueName = arg.repoName + '#' + arg.issueId; - } else { - arg.issueType = 'jira'; - arg.issueId = arg.issueName = arg.branchConfigIssue; - } - next(); + parseIssueArg(arg, arg.repoName, arg.branchConfigIssue, next); } else { next(); } }, + stepRemoveExtraIssues, + stepParseExtraIssues, + stepGetBranchConfigTitle, stepGetBranchConfigParenthetical, stepGetBranchConfigLastPushedSha, stepGetBranchConfigCr, stepEnsureCrRemote, - function getIssueTitle(arg, next) { + function stepGetIssueTitle(arg, next) { + // We cache the main bug title normally if (arg.branchConfigTitle && !opts.update) { arg.issueTitle = arg.branchConfigTitle; next(); return; } - // Need to retrieve this from the issue tracker. - - if (arg.issueType === 'jira') { - // TODO: Attempt to scrap from public bugview. - - if (!arg.grrConfig.jira || !arg.grrConfig.jira.username || - !arg.grrConfig.jira.password) - { - next(new VError( - 'cannot retrieve issue %s info: missing config ' - + 'in "%s":\n' - + ' [jira]\n' - + ' username = ""\n' - + ' password = ""\n' - + 'Note: Be sure to escape double-quotes and ' - + 'backslases in your password per\n' - + ' https://github.com/toml-lang/toml#string', - arg.issueName, config.CONFIG_FILE)); - return; + getIssueTitle(arg, arg, function (err, title) { + if (!err) { + arg.issueTitle = title; } - getJiraIssue(arg, function (err, issueInfo) { - if (err) { - next(err); - return; - } - arg.issueTitle = issueInfo.fields.summary; - next(); - }); - } else { - assert.equal(arg.issueType, 'github'); - getGitHubIssue(arg, function (err, issueInfo) { - if (err) { - next(err); - return; - } - arg.issueTitle = issueInfo.title; - next(); - }); - } + next(err); + }); }, + function printIssue(arg, next) { console.log('Issue: %s %s', arg.issueName, arg.issueTitle); next(); @@ -1439,8 +1601,17 @@ function grrUpdateOrCreate(opts, cb) { next(); }, + function printExtraIssues(arg, next) { + console.log('Extra issues: %s', + arg.extraIssues.map(function (issue) { + return issue.issueId; } + ).join(',')); + next(); + }, + stepEnsureOnIssueBranch, stepSetBranchConfigIssue, + stepSetBranchConfigExtraIssues, stepSetBranchConfigTitle, stepSetBranchConfigParenthetical,