Merge pull request #1424 from intuit/abuse

Fix various rate limiting issues
intuit · Jul 31, 2020 · 8bf2f44 · 8bf2f44
2 parents f26a302 + d6e7be2
commit 8bf2f44
Show file tree

Hide file tree

Showing 11 changed files with 81 additions and 99 deletions.
diff --git a/packages/cli/__tests__/main.test.ts b/packages/cli/__tests__/main.test.ts
@@ -2,6 +2,8 @@ import main, { run } from "../src/run";
 
 process.env.GH_TOKEN = "XXXX";
 
+jest.mock("@octokit/rest");
+
 test("throws error for unknown args", async () => {
   process.exit = jest.fn() as any;
   console.log = jest.fn() as any;

diff --git a/packages/core/package.json b/packages/core/package.json
@@ -64,6 +64,7 @@
     "node-fetch": "2.6.0",
     "parse-author": "^2.0.0",
     "parse-github-url": "1.0.2",
+    "pretty-ms": "^7.0.0",
     "semver": "^7.0.0",
     "signale": "^1.4.0",
     "tapable": "^2.0.0-beta.2",

diff --git a/packages/core/src/__tests__/auto.test.ts b/packages/core/src/__tests__/auto.test.ts
@@ -1191,34 +1191,6 @@ describe("Auto", () => {
       expect(auto.git!.addToPrBody).toHaveBeenCalled();
     });
 
-    test("falls back to first commit", async () => {
-      const auto = new Auto({ ...defaults, plugins: [] });
-      // @ts-ignore
-      auto.checkClean = () => Promise.resolve(true);
-      auto.logger = dummyLog();
-      await auto.loadConfig();
-      auto.git!.getLatestTagInBranch = () => {
-        throw new Error();
-      };
-
-      auto.git!.getLatestRelease = () => Promise.resolve("1.2.3");
-      auto.git!.getSha = () => Promise.resolve("abc");
-      auto.release!.getCommits = () => Promise.resolve([]);
-      auto.git!.getFirstCommit = () => Promise.resolve("abcd");
-      jest.spyOn(auto.git!, "addToPrBody").mockImplementation();
-      jest
-        .spyOn(auto.release!, "getCommitsInRelease")
-        .mockImplementation()
-        .mockReturnValue(Promise.resolve([makeCommitFromMsg("Test Commit")]));
-      const canary = jest.fn();
-      canary.mockReturnValue("abcd");
-      auto.hooks.canary.tap("test", canary);
-      jest.spyOn(auto.release!, "getCommits").mockImplementation();
-
-      await auto.canary({ pr: 123, build: 1 });
-      expect(auto.release!.getCommits).toHaveBeenCalledWith("abcd");
-    });
-
     test("adds sha if no pr or build number is found", async () => {
       const auto = new Auto({ ...defaults, plugins: [] });
       // @ts-ignore

diff --git a/packages/core/src/__tests__/git.test.ts b/packages/core/src/__tests__/git.test.ts
@@ -75,17 +75,15 @@ jest.mock("@octokit/rest", () => {
     hook = {
       error: errorHook,
     };
+
+    graphql = () => ({
+      data: [],
+    });
   };
 
   return { Octokit };
 });
 
-jest.mock("@octokit/graphql", () => ({
-  graphql: () => ({
-    data: [],
-  }),
-}));
-
 const options = {
   owner: "Adam Dierkens",
   repo: "test",

diff --git a/packages/core/src/__tests__/release.test.ts b/packages/core/src/__tests__/release.test.ts
@@ -1049,7 +1049,7 @@ describe("Release", () => {
       expect(await gh.getSemverBump("1234", "123")).toBe(SEMVER.minor);
     });
 
-    test("should be able to configure labels", async () => {
+    test("should be able to configure labels - no release", async () => {
       const customLabels = [
         ...defaultLabels,
         { name: "Version: Major", releaseType: SEMVER.major },
@@ -1083,7 +1083,30 @@ describe("Release", () => {
       );
 
       expect(await gh.getSemverBump("1234", "123")).toBe("");
+    });
+
+    test("should be able to configure labels", async () => {
+      const customLabels = [
+        ...defaultLabels,
+        { name: "Version: Major", releaseType: SEMVER.major },
+        { name: "Version: Minor", releaseType: SEMVER.minor },
+        { name: "Version: Patch", releaseType: SEMVER.patch },
+        { name: "Deploy", releaseType: "release" },
+      ] as ILabelDefinition[];
+
+      const gh = new Release(git, {
+        onlyPublishWithReleaseLabel: true,
+        prereleaseBranches: ["next"],
+        labels: customLabels,
+        baseBranch: "master",
+      });
+      const commits = [
+        makeCommitFromMsg("First (#1234)"),
+        makeCommitFromMsg("Second (#1235)"),
+        makeCommitFromMsg("Third (#1236)"),
+      ];
 
+      // Test deploy label creates release
       getGitLog.mockReturnValueOnce(commits);
       getPr.mockReturnValueOnce(
         Promise.resolve(mockLabels(["Version: Minor", "Deploy"]))

diff --git a/packages/core/src/auto.ts b/packages/core/src/auto.ts
@@ -1107,8 +1107,8 @@ export default class Auto {
     this.logger.verbose.info("Canary info found:", { pr, build });
 
     const from = (await this.git.shaExists("HEAD^")) ? "HEAD^" : "HEAD";
-    const head = await this.release.getCommitsInRelease(from);
-    const labels = head.map((commit) => commit.labels);
+    const commitsInRelease = await this.release.getCommitsInRelease(from);
+    const labels = commitsInRelease.map((commit) => commit.labels);
     let version = calculateSemVerBump(labels, this.semVerLabels!, this.config);
 
     if (version === SEMVER.noVersion) {
@@ -1210,15 +1210,6 @@ export default class Auto {
       await gitReset();
     }
 
-    let latestTag: string;
-
-    try {
-      latestTag = await this.git.getLatestTagInBranch();
-    } catch (error) {
-      latestTag = await this.git.getFirstCommit();
-    }
-
-    const commitsInRelease = await this.release.getCommits(latestTag);
     return { newVersion, commitsInRelease, context: "canary" };
   }
 

diff --git a/packages/core/src/git.ts b/packages/core/src/git.ts
@@ -1,4 +1,3 @@
-import { graphql } from "@octokit/graphql";
 import { enterpriseCompatibility } from "@octokit/plugin-enterprise-compatibility";
 import path from "path";
 import { retry } from "@octokit/plugin-retry";
@@ -11,6 +10,7 @@ import endent from "endent";
 import on from "await-to-js";
 import join from "url-join";
 import { gt, lt } from "semver";
+import prettyMs from "pretty-ms";
 
 import { Memoize as memoize } from "typescript-memoize";
 
@@ -128,9 +128,7 @@ export default class Git {
       ? this.options.graphqlBaseUrl || join(new URL(this.baseUrl).origin, "api")
       : this.baseUrl;
     this.logger.veryVerbose.info(`Initializing GitHub with: ${this.baseUrl}`);
-    const GitHub = Octokit.plugin(enterpriseCompatibility)
-      .plugin(retry)
-      .plugin(throttling);
+    const GitHub = Octokit.plugin(enterpriseCompatibility, retry, throttling);
     this.github = new GitHub({
       baseUrl: this.baseUrl,
       auth: this.options.token,
@@ -144,15 +142,20 @@ export default class Git {
           );
 
           if (opts.request.retryCount < 5) {
-            this.logger.verbose.log(`Retrying after ${retryAfter} seconds!`);
+            this.logger.log.log(
+              `Retrying after ${prettyMs(retryAfter * 1000)}!`
+            );
             return true;
           }
         },
-        /** does not retry, only logs an error */
-        onAbuseLimit: (_: number, opts: ThrottleOpts) => {
+        /** wait after abuse */
+        onAbuseLimit: (retryAfter: number, opts: ThrottleOpts) => {
           this.logger.log.error(
-            `Went over abuse rate limit ${opts.method} ${opts.url}`
+            `Went over abuse rate limit ${opts.method} ${
+              opts.url
+            }, retrying in ${prettyMs(retryAfter * 1000)}.`
           );
+          return true;
         },
       },
     });
@@ -488,6 +491,7 @@ export default class Git {
   }
 
   /** Search to GitHub project's issue and pull requests */
+  @memoize()
   async searchRepo(
     options: RestEndpointMethodTypes["search"]["issuesAndPullRequests"]["parameters"]
   ) {
@@ -505,10 +509,11 @@ export default class Git {
   }
 
   /** Run a graphql query on the GitHub project */
+  @memoize()
   async graphql<T>(query: string) {
     this.logger.verbose.info("Querying Github using GraphQL:\n", query);
 
-    const data = await graphql<T>(query, {
+    const data = await this.github.graphql<T>(query, {
       baseUrl: this.graphqlBaseUrl,
       request: { agent: this.options.agent },
       headers: {
@@ -687,7 +692,7 @@ export default class Git {
     });
 
     this.logger.veryVerbose.info(`Got response from PR #${pr}\n`, result);
-    this.logger.verbose.info(`Got commits for PR #${pr}.`);
+    this.logger.verbose.info(`Got commits for PR #${pr}`);
 
     return result;
   }

diff --git a/packages/core/src/release.ts b/packages/core/src/release.ts
@@ -283,6 +283,7 @@ export default class Release {
    * @param from - Tag or SHA to start at
    * @param to - Tag or SHA to end at (defaults to HEAD)
    */
+  @memoize()
   async getCommits(from: string, to = "HEAD"): Promise<IExtendedCommit[]> {
     this.logger.verbose.info(`Getting commits from ${from} to ${to}`);
 
@@ -611,12 +612,9 @@ export default class Release {
           hash: commit.hash,
         });
       } else if (commit.authorEmail) {
-        const author = await this.git.getUserByEmail(commit.authorEmail);
-
         resolvedAuthors.push({
           email: commit.authorEmail,
           name: commit.authorName,
-          ...author,
           hash: commit.hash,
         });
       }

diff --git a/plugins/first-time-contributor/src/index.ts b/plugins/first-time-contributor/src/index.ts
@@ -14,9 +14,6 @@ export default class FirstTimeContributorPlugin implements IPlugin {
 
   /** Tap into auto plugin points. */
   apply(auto: Auto) {
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    const cache: Record<string, Record<string, any>> = {};
-
     auto.hooks.onCreateChangelog.tap(this.name, (changelog) => {
       const base = new URL(changelog.options.baseUrl).origin;
 
@@ -26,46 +23,30 @@ export default class FirstTimeContributorPlugin implements IPlugin {
         return `${name}${username ? (name ? ` (${link})` : link) : ""}`;
       };
 
-      /** Get the PRs made by a user */
-      const getContributions = async (username: string) => {
-        if (cache[username]) {
-          return cache[username];
-        }
-
-        const response = await auto.git?.graphql<Record<string, any>>(`
-          {
-            search(first: 2, type: ISSUE, query: "repo:${auto.git?.options.owner}/${auto.git?.options.repo} is:pr is:merged author:${username}") {
-              issueCount
-            }
-          }
-        `);
-
-        if (response) {
-          cache[username] = response;
-        }
-
-        return response;
-      };
-
       changelog.hooks.addToBody.tapPromise(
         this.name,
         async (notes, commits) => {
-          const newContributors = (
-            await Promise.all(
-              flatMap(commits, (c) => c.authors).map(async (author) => {
-                if (!author.username || author.type === "Bot") {
-                  return;
-                }
+          const newContributors: ICommitAuthor[] = [];
 
-                // prettier-ignore
-                const prs = await getContributions(author.username)
+          for (const author of flatMap(commits, (c) => c.authors)) {
+            if (!author.username || author.type === "Bot") {
+              continue;
+            }
 
-                if (prs && prs.search.issueCount <= 1) {
-                  return author;
+            // prettier-ignore
+            // eslint-disable-next-line no-await-in-loop
+            const prs = await auto.git?.graphql<Record<string, any>>(`
+              {
+                search(first: 2, type: ISSUE, query: "repo:${auto.git?.options.owner}/${auto.git?.options.repo} is:pr is:merged author:${author.username}") {
+                  issueCount
                 }
-              })
-            )
-          ).filter((a): a is ICommitAuthor => Boolean(a));
+              }
+            `)
+
+            if (prs && prs.search.issueCount <= 1) {
+              newContributors.push(author);
+            }
+          }
 
           if (!newContributors.length) {
             return notes;

diff --git a/plugins/released/src/index.ts b/plugins/released/src/index.ts
@@ -122,8 +122,8 @@ export default class ReleasedLabelPlugin implements IPlugin {
     const isPrerelease = releases.some((r) => r.data.prerelease);
 
     if (commit.pullRequest) {
-      const branch = (await auto.git?.getPullRequest(commit.pullRequest.number))
-        ?.data.head.ref;
+      const pr = await auto.git!.getPullRequest(commit.pullRequest.number);
+      const branch = pr?.data.head.ref;
 
       if (branch && auto.config?.prereleaseBranches.includes(branch)) {
         return;
@@ -136,7 +136,6 @@ export default class ReleasedLabelPlugin implements IPlugin {
         releases,
       });
 
-      const pr = await auto.git!.getPullRequest(commit.pullRequest.number);
       pr.data.body.split("\n").map((line) => messages.push(line));
 
       const commitsInPr = await auto.git!.getCommitsForPR(

diff --git a/yarn.lock b/yarn.lock
@@ -13328,6 +13328,11 @@ parse-json@^5.0.0:
     json-parse-better-errors "^1.0.1"
     lines-and-columns "^1.1.6"
 
+parse-ms@^2.1.0:
+  version "2.1.0"
+  resolved "https://registry.yarnpkg.com/parse-ms/-/parse-ms-2.1.0.tgz#348565a753d4391fa524029956b172cb7753097d"
+  integrity sha512-kHt7kzLoS9VBZfUsiKjv43mr91ea+U05EyKkEtqp7vNbHxmaVuEqN7XxeEVnGrMtYOAxGrDElSi96K7EgO1zCA==
+
 parse-numeric-range@^0.0.2:
   version "0.0.2"
   resolved "https://registry.yarnpkg.com/parse-numeric-range/-/parse-numeric-range-0.0.2.tgz#b4f09d413c7adbcd987f6e9233c7b4b210c938e4"
@@ -14186,6 +14191,13 @@ pretty-format@^26.1.0:
     ansi-styles "^4.0.0"
     react-is "^16.12.0"
 
+pretty-ms@^7.0.0:
+  version "7.0.0"
+  resolved "https://registry.yarnpkg.com/pretty-ms/-/pretty-ms-7.0.0.tgz#45781273110caf35f55cab21a8a9bd403a233dc0"
+  integrity sha512-J3aPWiC5e9ZeZFuSeBraGxSkGMOvulSWsxDByOcbD1Pr75YL3LSNIKIb52WXbCLE1sS5s4inBBbryjF4Y05Ceg==
+  dependencies:
+    parse-ms "^2.1.0"
+
 prismjs@~1.17.0:
   version "1.17.1"
   resolved "https://registry.yarnpkg.com/prismjs/-/prismjs-1.17.1.tgz#e669fcbd4cdd873c35102881c33b14d0d68519be"