git-node: add --backport flag to land (#383)

This commit adds functionality to land that allows you to land backports with the correct metadata Co-authored-by: AshCripps <ashley.cripps@ibm.com>
nodejs · Feb 14, 2020 · 189c167 · 189c167
1 parent cbd01f5
commit 189c167
Show file tree

Hide file tree

Showing 9 changed files with 136 additions and 29 deletions.
diff --git a/components/git/land.js b/components/git/land.js
@@ -37,6 +37,11 @@ const landOptions = {
     describe: 'Assume "yes" as answer to all prompts and run ' +
     'non-interactively. If an undesirable situation occurs, such as a pull ' +
     'request or commit check fails, then git node land will abort.'
+  },
+  backport: {
+    describe: 'Land a backport PR onto a staging branch',
+    default: false,
+    type: 'boolean'
   }
 };
 
@@ -152,8 +157,14 @@ async function main(state, argv, cli, req, dir) {
       cli.log('run `git node land --abort` before starting a new session');
       return;
     }
-    session = new LandingSession(cli, req, dir, argv.prid);
+    session = new LandingSession(cli, req, dir, argv.prid, argv.backport);
     const metadata = await getMetadata(session.argv, cli);
+    if (argv.backport) {
+      const split = metadata.metadata.split('\n')[0];
+      if (split === 'PR-URL: ') {
+        cli.error('Commit message is missing PR-URL');
+      }
+    }
     return session.start(metadata);
   } else if (state === APPLY) {
     return session.apply();

diff --git a/docs/git-node.md b/docs/git-node.md
@@ -40,19 +40,22 @@ Options:
   --continue, -c  Continue the landing session                         [boolean]
   --final         Verify the landed PR and clean up                    [boolean]
   --abort         Abort the current landing session                    [boolean]
+  --backport      Land a backport PR on a staging branch               [boolean]
   --yes           Assume "yes" as answer to all prompts and run
                   non-interactively. If an undesirable situation occurs, such as
                   a pull request or commit check fails, then git node land will
                   abort.                              [boolean] [default: false]
 
 
 Examples:
-  git node land 12344       Land https://github.com/nodejs/node/pull/12344 in
-                            the current directory
-  git node land --abort     Abort the current session
-  git node land --amend     Append metadata to the current commit message
-  git node land --final     Verify the landed PR and clean up
-  git node land --continue  Continue the current landing session
+  git node land 12344            Land https://github.com/nodejs/node/pull/12344
+                                 in the current directory
+  git node land --abort          Abort the current session
+  git node land --amend          Append metadata to the current commit message
+  git node land --final          Verify the landed PR and clean up
+  git node land --continue       Continue the current landing session
+  git node land --backport 30072 Land https://github.com/nodejs/node/pull/30072
+                                 as a backport in the current directory
 ```
 
 <a id="git-node-land-prerequisites"></a>

diff --git a/lib/landing_session.js b/lib/landing_session.js
@@ -13,9 +13,16 @@ const {
 const isWindows = process.platform === 'win32';
 
 class LandingSession extends Session {
-  constructor(cli, req, dir, prid) {
+  constructor(cli, req, dir, prid, backport) {
     super(cli, dir, prid);
     this.req = req;
+    this.backport = backport;
+  }
+
+  get argv() {
+    const args = super.argv;
+    args.backport = this.backport;
+    return args;
   }
 
   async start(metadata) {
@@ -163,13 +170,25 @@ class LandingSession extends Session {
       amended.push('');
     }
 
+    const BACKPORT_RE = /BACKPORT-PR-URL\s*:\s*(\S+)/i;
+    const PR_RE = /PR-URL\s*:\s*(\S+)/i;
+    const REVIEW_RE = /Reviewed-By\s*:\s*(\S+)/i;
+
     for (const line of metadata) {
       if (original.includes(line)) {
         if (line) {
           cli.warn(`Found ${line}, skipping..`);
         }
       } else {
-        amended.push(line);
+        if (line.match(BACKPORT_RE)) {
+          let prIndex = amended.findIndex(datum => datum.match(PR_RE));
+          if (prIndex === -1) {
+            prIndex = amended.findIndex(datum => datum.match(REVIEW_RE)) - 1;
+          }
+          amended.splice(prIndex + 1, 0, line);
+        } else {
+          amended.push(line);
+        }
       }
     }
 

diff --git a/lib/links.js b/lib/links.js
@@ -4,6 +4,7 @@ const FIXES_RE = /(Close[ds]?|Fix(e[ds])?|Resolve[sd]?)\s*:\s*(\S+)/mgi;
 const FIX_RE = /(Close[ds]?|Fix(e[ds])?|Resolve[sd]?)\s*:\s*(\S+)/i;
 const REFS_RE = /Refs?\s*:\s*(\S+)/mgi;
 const REF_RE = /Refs?\s*:\s*(\S+)/i;
+const PR_RE = /PR-URL\s*:\s*(\S+)/i;
 const cheerio = require('cheerio');
 
 /**
@@ -36,15 +37,27 @@ class LinkParser {
       const m = item.match(REF_RE);
       if (!m) continue;
       const ref = m[1];
-      const url = this.getRefUrlFromOP(ref);
+      const url = this.getUrlFromOP(ref);
+      if (url) result.push(url);
+    }
+    return result;
+  }
+
+  getPRUrlsFromArray(arr) {
+    const result = [];
+    for (const item of arr) {
+      const m = item.match(PR_RE);
+      if (!m) continue;
+      const prUrl = m[1];
+      const url = this.getUrlFromOP(prUrl);
       if (url) result.push(url);
     }
     return result;
   }
 
   // Do this so we can reliably get the correct url.
   // Otherwise, the number could reference a PR or an issue.
-  getRefUrlFromOP(ref) {
+  getUrlFromOP(ref) {
     const as = this.$('a');
     const links = as.map((i, el) => this.$(el)).get();
     for (const link of links) {
@@ -58,22 +71,22 @@ class LinkParser {
 
   getFixes() {
     const text = this.$.text();
-    const fixes = text.match(FIXES_RE);
-    if (fixes) {
-      return this.getFixesUrlsFromArray(fixes);
-    }
-    return [];
+    const fixes = text.match(FIXES_RE) || [];
+    return this.getFixesUrlsFromArray(fixes);
   }
 
   getRefs() {
     const text = this.$.text();
-    const refs = text.match(REFS_RE);
-    if (refs) {
-      return this.getRefsUrlsFromArray(refs);
-    }
-    return [];
+    const refs = text.match(REFS_RE) || [];
+    return this.getRefsUrlsFromArray(refs);
   }
-};
+
+  getAltPrUrl() {
+    const text = this.$.text();
+    const refs = text.match(PR_RE) || [];
+    return this.getPRUrlsFromArray(refs);
+  }
+}
 
 const GITHUB_PULL_REQUEST_URL = /github.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/;
 

diff --git a/lib/metadata_gen.js b/lib/metadata_gen.js
@@ -10,11 +10,12 @@ class MetadataGenerator {
    * @param {PRData} data
    */
   constructor(data) {
-    const { owner, repo, pr, reviewers } = data;
+    const { owner, repo, pr, reviewers, argv } = data;
     this.owner = owner;
     this.repo = repo;
     this.pr = pr;
     this.reviewers = reviewers;
+    this.argv = argv;
   }
 
   /**
@@ -31,15 +32,24 @@ class MetadataGenerator {
     const parser = new LinkParser(owner, repo, op);
     const fixes = parser.getFixes();
     const refs = parser.getRefs();
-
+    const altPrUrl = parser.getAltPrUrl();
     const meta = [
-      `PR-URL: ${prUrl}`,
       ...fixes.map((fix) => `Fixes: ${fix}`),
-      ...refs.map((ref) => `Refs: ${ref}`),
-      ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`),
-      '' // creates final EOL
+      ...refs.map((ref) => `Refs: ${ref}`)
     ];
-
+    const backport = this.argv ? this.argv.backport : undefined;
+    if (backport) {
+      meta.unshift(`Backport-PR-URL: ${prUrl}`);
+      meta.unshift(`PR-URL: ${altPrUrl}`);
+    } else {
+      // Reviews are only added here as backports should not contain reviews
+      // for the backport itself in the metadata
+      meta.unshift(`PR-URL: ${prUrl}`);
+      meta.push(
+        ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`)
+      );
+    }
+    meta.push(''); // creates final EOL
     return meta.join('\n');
   }
 }

diff --git a/lib/session.js b/lib/session.js
@@ -369,6 +369,7 @@ class Session {
       `  $ ncu-config set branch ${rev}`);
     cli.separator();
     return true;
+  // TODO warn if backporting onto master branch
   }
 }
 

diff --git a/test/fixtures/data.js b/test/fixtures/data.js
@@ -75,6 +75,7 @@ const firstTimerPrivatePR = readJSON('first_timer_pr_with_private_email.json');
 const semverMajorPR = readJSON('semver_major_pr.json');
 const fixAndRefPR = readJSON('pr_with_fixes_and_refs.json');
 const fixCrossPR = readJSON('pr_with_fixes_cross.json');
+const backportPR = readJSON('pr_with_backport.json');
 const conflictingPR = readJSON('conflicting_pr.json');
 const emptyProfilePR = readJSON('empty_profile_pr.json');
 const closedPR = readJSON('./closed_pr.json');
@@ -114,6 +115,7 @@ module.exports = {
   semverMajorPR,
   fixAndRefPR,
   fixCrossPR,
+  backportPR,
   conflictingPR,
   emptyProfilePR,
   readme,

diff --git a/test/fixtures/pr_with_backport.json b/test/fixtures/pr_with_backport.json
@@ -0,0 +1,22 @@
+{
+  "createdAt": "2019-10-22T22:42:25Z",
+  "authorAssociation": "CONTRIBUTOR",
+  "author": {
+    "login": "gabrielschulhof",
+    "email": "gabriel.schulhof@intel.com",
+    "name": "Gabriel Schulhof"
+  },
+  "url": "https://github.com/nodejs/node/pull/30072",
+  "bodyHTML":  "<p>Build the addons for benchmarks in the same way that the addons for<br>\ntests are built.</p>\n<p>PR-URL: <a class='issue-link js-issue-link' data-error-text='Failed to load issue title' data-id='508006081' data-permission-text='Issue title is private' data-url='https://github.com/nodejs/node/issues/29995' data-hovercard-type='pull_request' data-hovercard-url='/nodejs/node/pull/29995/hovercard' href='https://github.com/nodejs/node/pull/29995'>#29995</a><br>\n<span class='issue-keyword tooltipped tooltipped-se' aria-label='This pull request closes issue #1961.'>Fixes</span>: <a class='ssue-link js-issue-link' data-error-text='Failed to load issue title' data-id='507966018' data-permission-text='Issue title is private' data-url='https://github.com/nodejs/build/issues/1961' data-hovercard-type='issue' data-hovercard-url='/nodejs/build/issues/1961/hovercard' href='https://github.com/nodejs/build/issues/1961'>nodejs/build#1961</a><br>\nRefs: <a class='commit-link' href='https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896'><tt>53ca0b9</tt>#commitcomment-35494896</a><br>\nReviewed-By: <a class='user-mention' data-hovercard-type='user' data-hovercard-url='/users/addaleax/hovercard' data-octo-click='hovercard-link-click' data-octo-dimensions='link_type:self' href='https://github.com/addaleax'>@addaleax</a><br>\nReviewed-By: <a class='user-mention' data-hovercard-type='user' data-hovercard-url='/users/Trott/hovercard' data-octo-click='hovercard-link-click' data-octo-dimensions='link_type:self' href='https://github.com/Trott'>@Trott</a><br>\nReviewed-By: <a class='user-mention' data-hovercard-type='user' data-hovercard-url='/users/BethGriggs/hovercard' data-octo-click='hovercard-link-click' data-octo-dimensions='link_type:self' href='https://github.com/BethGriggs'>@BethGriggs</a><br>\nReviewed-By: <a class='user-mention' data-hovercard-type='user' data-hovercard-url='/users/gengjiawen/hovercard' data-octo-click='hovercard-link-click' data-octo-dimensions='link_type:self' href='https://github.com/gengjiawen'>@gengjiawen</a></p>\n\n<h5>Checklist</h5>\n\n<ul class='contains-task-list'>\n<li class='task-list-item'><input type='checkbox' id='' disabled='' class='task-list-item-checkbox' checked=''> <code>make -j4 test</code> (UNIX), or <code>vcbuild test</code> (Windows) passes</li>\n<li class='task-list-item'><input type='checkbox' id='' disabled='' class='task-list-item-checkbox' checked=''> commit message follows <a href='https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines'>commit guidelines</a></li>\n</ul>\n\n<p>We need this PR along with <a class='issue-link js-issue-link' data-error-text='Failed to load issue title' data-id='510938668' data-permission-text='Issue title is private' data-url='https://github.com/nodejs/node/issues/30070' data-hovercard-type='pull_request' data-hovercard-url='/nodejs/node/pull/30070/hovercard' href='https://github.com/nodejs/node/pull/30070'>#30070</a> because <a class='commit-link' href='https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896'><tt>53ca0b9</tt>#commitcomment-35494896</a></p>",
+  "bodyText": "Build the addons for benchmarks in the same way that the addons for\ntests are built.\nPR-URL: #29995\nFixes: nodejs/build#1961\nRefs: 53ca0b9#commitcomment-35494896\nReviewed-By: @addaleax\nReviewed-By: @Trott\nReviewed-By: @BethGriggs\nReviewed-By: @gengjiawen\n\nChecklist\n\n\n make -j4 test (UNIX), or vcbuild test (Windows) passes\n commit message follows commit guidelines\n\n\nWe need this PR along with #30070 because 53ca0b9#commitcomment-35494896",
+  "labels": {
+    "nodes": [
+      {
+        "name": "build"
+      },
+      {
+        "name": "v12.x"
+      }
+    ]
+  }
+}
diff --git a/test/unit/metadata_gen.test.js b/test/unit/metadata_gen.test.js
@@ -4,6 +4,7 @@ const MetadataGenerator = require('../../lib/metadata_gen');
 const {
   fixAndRefPR,
   fixCrossPR,
+  backportPR,
   allGreenReviewers
 } = require('../fixtures/data');
 
@@ -15,6 +16,21 @@ const data = {
   reviewers: allGreenReviewers
 };
 const crossData = Object.assign({}, data, { pr: fixCrossPR });
+const backportArgv = {
+  argv: {
+    owner: 'nodejs',
+    repo: 'node',
+    upstream: 'upstream',
+    branch: 'v12.x-staging',
+    readme: undefined,
+    waitTimeSingleApproval: undefined,
+    waitTimeMultiApproval: undefined,
+    prid: 30072,
+    backport: true
+  }
+};
+
+const backportData = Object.assign({}, data, { pr: backportPR }, backportArgv);
 
 const expected = `PR-URL: https://github.com/nodejs/node/pull/16438
 Fixes: https://github.com/nodejs/node/issues/16437
@@ -31,6 +47,11 @@ Reviewed-By: Quux User <quux@example.com>
 Reviewed-By: Baz User <baz@example.com>
 Reviewed-By: Bar User <bar@example.com>
 `;
+const backportExpected = `PR-URL: https://github.com/nodejs/node/pull/29995
+Backport-PR-URL: https://github.com/nodejs/node/pull/30072
+Fixes: https://github.com/nodejs/build/issues/1961
+Refs: https://github.com/nodejs/node/commit/53ca0b9ae145c430842bf78e553e3b6cbd2823aa#commitcomment-35494896
+`;
 
 describe('MetadataGenerator', () => {
   it('should generate metadata properly', () => {
@@ -42,4 +63,9 @@ describe('MetadataGenerator', () => {
     const results = new MetadataGenerator(crossData).getMetadata();
     assert.strictEqual(crossExpected, results);
   });
+
+  it('should generate correct metadata for a backport', () => {
+    const backportResults = new MetadataGenerator(backportData).getMetadata();
+    assert.strictEqual(backportExpected, backportResults);
+  });
 });