Refactor issue tracing (#45034)

* refactor and improve milestone check code

* whitespace

release/src/github.ts

@@ -164,3 +164,31 @@ export const tagRelease = async ({
     throw new Error(`failed to tag release ${version}`);
   }
 };
+
+const _issueCache: Record<number, Issue> = {};
+
+export async function getIssueWithCache ({
+  github,
+  owner,
+  repo,
+  issueNumber,
+}: GithubProps & { issueNumber: number }) {
+  if (_issueCache[issueNumber]) {
+    return _issueCache[issueNumber];
+  }
+
+  const issue = await github.rest.issues.get({
+    owner,
+    repo,
+    issue_number: issueNumber,
+  }).catch((err) => {
+    console.log(err);
+    return null;
+  });
+
+  if (issue?.data) {
+    _issueCache[issueNumber] = issue.data as Issue;
+  }
+
+  return issue?.data as Issue | null;
+}

release/src/linked-issues.ts

 export function getLinkedIssues(body: string) {
   const matches = body.match(
-    /(close(s|d)?|fixe?(s|d)?|resolve(s|d)?) (#|https?:\/\/github.com\/.+\/issues\/)(\d+)/gi,
+    /(close(s|d)?|fixe?(s|d)?|resolve(s|d)?)(:?) (#|https?:\/\/github.com\/.+\/issues\/)(\d+)/gi,
   );
 
   if (matches) {
@@ -17,7 +17,7 @@ export function getPRsFromCommitMessage(message: string) {
   const firstLine = message.split('\n\n')[0];
   const result = [ ...firstLine.matchAll(/\(#(\d+)\)/g) ];
   if (!result.length) {
-    console.log('no pr found in commit message', message);
+    console.log('No pr found in commit message', message);
     return null;
   }
 

release/src/linked-issues.unit.spec.ts

@@ -41,7 +41,7 @@ describe("getLinkedIssues", () => {
       ).toBeNull();
     });
 
-    it("should return `null` when the issue doesn't immediatelly follow the closing keyword", () => {
+    it("should return `null` when the issue doesn't immediately follow the closing keyword", () => {
       // Two or more spaces
       expect(getLinkedIssues("Fix  #123")).toBeNull();
       // Newline
@@ -71,6 +71,12 @@ describe("getLinkedIssues", () => {
           ["123"],
         );
       });
+
+      it(`should return the issue id for ${closingKeyword.toLowerCase()} with a colon`, () => {
+        expect(getLinkedIssues(`${closingKeyword.toLowerCase()}: #123`)).toEqual(
+          ["123"],
+        );
+      });
     });
 
     describe("https syntax", () => {

release/src/milestones.ts

 import _ from "underscore";
+import 'zx/globals';
 
-import { getMilestones } from "./github";
-import { getLinkedIssues, getPRsFromCommitMessage, getBackportSourcePRNumber } from "./linked-issues";
+import {
+  getMilestones,
+  getIssueWithCache,
+} from "./github";
+import {
+  getLinkedIssues,
+  getPRsFromCommitMessage,
+  getBackportSourcePRNumber,
+} from "./linked-issues";
 import type { Issue, GithubProps, Milestone } from "./types";
 import {
   getMajorVersion,
   getVersionFromReleaseBranch,
+  versionSort,
+  ignorePatches,
 } from "./version-helpers";
 
 function isBackport(pullRequest: Issue) {
@@ -16,76 +26,67 @@ function isBackport(pullRequest: Issue) {
     );
 }
 
-// for auto-setting milestones, we don't ever want to auto-set a patch milestone
-// which we release VERY rarely
-function ignorePatches(version: string) {
-  return version.split('.').length < 4;
-}
-
-function versionSort(a: string, b: string) {
-  const [aMajor, aMinor] = a.split('.').map(Number);
-  const [bMajor, bMinor] = b.split('.').map(Number);
-
-  if (aMajor !== bMajor) {
-    return aMajor - bMajor;
-  }
-
-  if (aMinor !== bMinor) {
-    return aMinor - bMinor;
-  }
-
-  return 0;
-}
-
 const isNotNull = <T>(value: T | null): value is T => value !== null;
 
-
-async function getOriginalPR({
+async function getOriginalIssues({
   github,
   repo,
   owner,
-  pullRequestNumber,
-}: GithubProps & { pullRequestNumber: number }) {
-  // every PR in the release branch should have a pr number
-  // it could be a backport PR or an original PR
-  const pull = await github.rest.pulls.get({
+  issueNumber,
+}: GithubProps & { issueNumber: number }) {
+  console.log('checking', issueNumber);
+  const issue = await getIssueWithCache({
+    github,
     owner,
     repo,
-    pull_number: pullRequestNumber,
+    issueNumber,
   });
 
-  if (pull?.data && isBackport(pull.data) && pull.data.body) {
-    const sourcePRNumber = getBackportSourcePRNumber(pull.data.body);
-    if (sourcePRNumber && sourcePRNumber !== pullRequestNumber) {
-      console.log('found backport PR', pull.data.number, 'source PR', sourcePRNumber);
-      return getOriginalPR({
+  if (!issue) {
+    console.log(`  Issue ${issueNumber} not found`);
+    return [];
+  }
+
+  // if this isn't a pull request, we don't need to trace further
+  if (!issue.pull_request) {
+    console.log('  Found an issue');
+    return [issue.number];
+  }
+
+  if (isBackport(issue)) {
+    const sourcePRNumber = getBackportSourcePRNumber(issue.body);
+    if (sourcePRNumber && sourcePRNumber !== issueNumber) {
+      console.log('  found backport PR for ', sourcePRNumber);
+      return getOriginalIssues({
         github,
         repo,
         owner,
-        pullRequestNumber: sourcePRNumber,
+        issueNumber: sourcePRNumber,
       });
     }
   }
 
-  const linkedIssues = await getLinkedIssues(pull.data.body ?? '');
+  const linkedIssues = await getLinkedIssues(issue.body ?? '');
 
   if (linkedIssues) {
-    console.log('found linked issue for PR', pull.data.number, linkedIssues);
+    console.log('  found linked issues', linkedIssues);
     return linkedIssues.map(Number);
   }
-  console.log("no linked issues found in PR body", pull.data.number);
-  return [pull.data.number];
+
+  console.log("  no linked issues found in body");
+  return [issue.number];
 }
 
 async function setMilestone({ github, owner, repo, issueNumber, milestone }: GithubProps & { issueNumber: number, milestone: Milestone }) {
   // we can use this for both issues and PRs since they're the same for many purposes in github
-  const issue = await github.rest.issues.get({
+  const issue = await getIssueWithCache({
+    github,
     owner,
     repo,
-    issue_number: issueNumber,
+    issueNumber,
   });
 
-  if (!issue.data.milestone) {
+  if (!issue?.milestone) {
     return github.rest.issues.update({
       owner,
       repo,
@@ -94,7 +95,7 @@ async function setMilestone({ github, owner, repo, issueNumber, milestone }: Git
     });
   }
 
-  const existingMilestone = issue.data.milestone;
+  const existingMilestone = issue.milestone;
 
   if (existingMilestone.number === milestone.number) {
     console.log(`Issue ${issueNumber} is already tagged with this ${milestone.title} milestone`);
@@ -178,11 +179,11 @@ export async function setMilestoneForCommits({
   const issuesToTag = [];
 
   for (const prNumber of PRsToCheck) { // for loop to avoid rate limiting
-    issuesToTag.push(...(await getOriginalPR({
+    issuesToTag.push(...(await getOriginalIssues({
       github,
       owner,
       repo,
-      pullRequestNumber: prNumber,
+      issueNumber: prNumber,
     })));
   }
 
@@ -194,3 +195,4 @@ export async function setMilestoneForCommits({
     await setMilestone({ github, owner, repo, issueNumber, milestone: nextMilestone });
   }
 }
+

release/src/types.ts

@@ -39,9 +39,13 @@ export interface VersionInfoFile {
 
 export type Issue = {
   number: number;
+  node_id: string;
   title: string;
   html_url: string;
-  labels: string | ({ name?: string }[]);
+  body: string;
+  pull_request?: { html_url: string }; // only present on PRs
+  milestone?: Milestone;
+  labels: string | { name?: string }[];
   assignee: null |  { login: string };
   created_at: string;
 };
@@ -57,3 +61,21 @@ export type Milestone =  {
   title: string;
   description: string | null;
 };
+
+export type Commit = {
+  sha: string;
+  commit: {
+    message: string;
+  };
+};
+
+export type Tag = {
+  ref: string;
+  node_id: string,
+  url: string,
+  object: {
+    sha: string,
+    type: string,
+    url: string,
+  }
+};

release/src/version-helpers.ts

 import { compare as compareVersions, coerce } from "semver";
 
+import type { GithubProps, Tag } from "./types";
+
 // https://regexr.com/7l1ip
 export const isValidVersionString = (versionString: string) => {
   return /^(v0|v1)\.(\d|\.){3,}(\-rc\d+|\-RC\d+)*$/.test(versionString);
@@ -195,3 +197,56 @@ export const getMilestoneName = (version: string) => {
     .replace(/-rc\d+$/i, "") // RC versions use the major version milestone
     .replace(/\.0$/, "");
 };
+
+// for auto-setting milestones, we don't ever want to auto-set a patch milestone
+// which we release VERY rarely
+export function ignorePatches(version: string) {
+  return version.split('.').length < 4;
+}
+
+const normalizeVersionForSorting = (version: string) =>
+  version.replace(/^(v?)(0|1)\./, '');
+
+export function versionSort(a: string, b: string) {
+  const [aMajor, aMinor] = normalizeVersionForSorting(a).split('.').map(Number);
+  const [bMajor, bMinor] = normalizeVersionForSorting(b).split('.').map(Number);
+
+  if (aMajor !== bMajor) {
+    return aMajor - bMajor;
+  }
+
+  if (aMinor !== bMinor) {
+    return aMinor - bMinor;
+  }
+
+  return 0;
+}
+
+export function getLastReleaseFromTags(tags: Tag[]) {
+  return tags
+    .map(tag => tag.ref.replace('refs/tags/', ''))
+    .filter(tag => !isRCVersion(tag)) // we want to ignore RC tags because release notes should be cumulative
+    .sort(versionSort)
+    .reverse()[0];
+}
+
+/**
+ * queries the github api to get all release version tags,
+ * optionally filtered by a major version
+ */
+export async function getLastReleaseTag({
+  github,
+  owner,
+  repo,
+  version = '',
+}: GithubProps & { version?: string }) {
+  const tags =  await github.paginate(github.rest.git.listMatchingRefs, {
+    owner,
+    repo,
+    ref: `tags/v0.${version ? getMajorVersion(version) : ''}`,
+  })
+
+  const lastRelease = getLastReleaseFromTags(tags);
+
+  return lastRelease;
+}

release/src/version-helpers.unit.spec.ts

+import type { Tag } from "./types";
 import {
   isValidVersionString,
   getOSSVersion,
@@ -12,8 +13,11 @@ import {
   getNextVersions,
   getGenericVersion,
   getMilestoneName,
+  getLastReleaseFromTags,
+  versionSort,
 } from "./version-helpers";
 
+
 describe("version-helpers", () => {
   describe("isValidVersionString", () => {
     const validCases = [
@@ -468,3 +472,84 @@ describe("getMilestoneName", () => {
     expect(getMilestoneName(input)).toBe(expected);
   });
 });
+
+describe('getLatReleaseFromTags', () => {
+  it('should return the latest release tag for minor versions', () => {
+    const latest = getLastReleaseFromTags([
+      { ref: 'refs/tags/v0.12.0' },
+      { ref: 'refs/tags/v0.12.2' },
+      { ref: 'refs/tags/v0.12.1' },
+    ] as Tag[]);
+    expect(latest).toBe('v0.12.2');
+  });
+
+  it('should return the latest tag for major version', () => {
+    const latest = getLastReleaseFromTags([
+      { ref: 'refs/tags/v0.12.9' },
+      { ref: 'refs/tags/v0.12.8' },
+      { ref: 'refs/tags/v0.13.0' },
+    ] as Tag[]);
+    expect(latest).toBe('v0.13.0');
+  });
+
+  it('should ignore release candidates', () => {
+    const latest = getLastReleaseFromTags([
+      { ref: 'refs/tags/v0.12.0' },
+      { ref: 'refs/tags/v0.12.2-RC99' },
+      { ref: 'refs/tags/v0.12.1' },
+    ] as Tag[]);
+    expect(latest).toBe('v0.12.1');
+  });
+});
+
+describe('verisonSort', () => {
+  it('should sort major versions', () => {
+    const diff1 = versionSort('v0.50.9', 'v0.48.1');
+    expect(diff1).toBeGreaterThan(0);
+
+    const diff2 = versionSort('v0.40.9', 'v0.50.1');
+    expect(diff2).toBeLessThan(0);
+
+    const diff3 = versionSort('v0.50.0', 'v0.50.0');
+    expect(diff3).toBe(0);
+  });
+
+  it('should sort minor versions', () => {
+    const diff1 = versionSort('v0.48.10', 'v0.48.1');
+    expect(diff1).toBeGreaterThan(0);
+
+    const diff2 = versionSort('v0.40.2', 'v0.40.4');
+    expect(diff2).toBeLessThan(0);
+
+    const diff3 = versionSort('v0.50.11', 'v0.50.11');
+    expect(diff3).toBe(0);
+  });
+
+  it('should handle versions with or without Vs', () => {
+    const diff1 = versionSort('v0.48.10', 'v0.48.1');
+    expect(diff1).toBeGreaterThan(0);
+
+    const diff2 = versionSort('0.40.2', 'v0.40.4');
+    expect(diff2).toBeLessThan(0);
+
+    const diff3 = versionSort('v0.50.11', '0.50.11');
+    expect(diff3).toBe(0);
+
+    const diff4 = versionSort('0.50.12', '0.50.11');
+    expect(diff4).toBeGreaterThan(0);
+  });
+
+  it('should ignore the ee/oss prefix', () => {
+    const diff1 = versionSort('v0.48.10', 'v1.48.1');
+    expect(diff1).toBeGreaterThan(0);
+
+    const diff2 = versionSort('1.40.2', 'v0.40.4');
+    expect(diff2).toBeLessThan(0);
+
+    const diff3 = versionSort('v0.50.11', '1.50.11');
+    expect(diff3).toBe(0);
+
+    const diff4 = versionSort('50.12', '1.50.11');
+    expect(diff4).toBeGreaterThan(0);
+  });
+});