Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix various rate limiting issues #1424

Merged
merged 10 commits into from
Jul 31, 2020
Merged

Fix various rate limiting issues #1424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
80b3922
retry after abuse limit
hipstersmoothie Jul 30, 2020
84109da
memoize searchRepo
hipstersmoothie Jul 30, 2020
0ef0b9a
pretty print time left on rate limit
hipstersmoothie Jul 30, 2020
3d64d66
use graphql api on octokit instance
hipstersmoothie Jul 30, 2020
b4831c0
mock octokit in cli tests
hipstersmoothie Jul 31, 2020
66f7e2a
return only commits in the canary release instead of all commits sinc…
hipstersmoothie Jul 30, 2020
5d9bb97
remove author email match, easily hits rate limit
hipstersmoothie Jul 31, 2020
2a0f270
memoize getCommits
hipstersmoothie Jul 31, 2020
baab296
call `getPullRequest` 1 less time
hipstersmoothie Jul 31, 2020
d6e7be2
search for past contributions serially so that memoized graphql call …
hipstersmoothie Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/cli/__tests__/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
28 changes: 0 additions & 28 deletions packages/core/src/__tests__/auto.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 4 additions & 6 deletions packages/core/src/__tests__/git.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
25 changes: 24 additions & 1 deletion packages/core/src/__tests__/release.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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"]))
Expand Down
13 changes: 2 additions & 11 deletions packages/core/src/auto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this because getting all the commits in a canary release with no recent release can lead to hitting the rate limits. With canaries this is especially easy to hit because they are made so often.

return { newVersion, commitsInRelease, context: "canary" };
}

Expand Down
25 changes: 15 additions & 10 deletions packages/core/src/git.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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";

Expand Down Expand Up @@ -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,
Expand All @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will now actually retry after abuse limit is hit

},
},
});
Expand Down Expand Up @@ -488,6 +491,7 @@ export default class Git {
}

/** Search to GitHub project's issue and pull requests */
@memoize()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a single run if the search query is the same it should be safe to memoize. The search endpoints have super low rate limits so this should help a bit.

async searchRepo(
options: RestEndpointMethodTypes["search"]["issuesAndPullRequests"]["parameters"]
) {
Expand All @@ -505,10 +509,11 @@ export default class Git {
}

/** Run a graphql query on the GitHub project */
@memoize()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing goes for graphql queries. Easy to 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, {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to octokit's graphql function will make sure that it is rate/abuse limited properly

baseUrl: this.graphqlBaseUrl,
request: { agent: this.options.agent },
headers: {
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 1 addition & 3 deletions packages/core/src/release.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was iffy on adding memoization here. But since auto will never care about generated commits I think it's safe to memoize this function. This should also help with some log reduction

async getCommits(from: string, to = "HEAD"): Promise<IExtendedCommit[]> {
this.logger.verbose.info(`Getting commits from ${from} to ${to}`);

Expand Down Expand Up @@ -611,12 +612,9 @@ export default class Release {
hash: commit.hash,
});
} else if (commit.authorEmail) {
const author = await this.git.getUserByEmail(commit.authorEmail);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUserByEmail used search which is easily rate-limited, just omitting instead. It's 50/50 that the user has their email public anyway


resolvedAuthors.push({
email: commit.authorEmail,
name: commit.authorName,
...author,
hash: commit.hash,
});
}
Expand Down
55 changes: 18 additions & 37 deletions plugins/first-time-contributor/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>> = {};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rely on git client memoization instead of custom cache


auto.hooks.onCreateChangelog.tap(this.name, (changelog) => {
const base = new URL(changelog.options.baseUrl).origin;

Expand All @@ -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>>(`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call graphql API serially so we can use the memoized the graphql call

{
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;
Expand Down
5 changes: 2 additions & 3 deletions plugins/released/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand Down
20 changes: 16 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@
integrity sha512-m2OzlTDbhvzK4hgswH9Lx0cdpq1AntXu53Klz5RGkFfuoDvKsnlU7tmtVfNWtUiP0zZbGtrb/BZYH7aB6TRnMA==

"@auto-it/bot-list@link:packages/bot-list":
version "9.48.2"
version "9.49.1"

"@auto-it/core@link:packages/core":
version "9.48.2"
version "9.49.1"
dependencies:
"@auto-it/bot-list" "link:packages/bot-list"
"@octokit/graphql" "^4.4.0"
Expand Down Expand Up @@ -95,7 +95,7 @@
url-join "^4.0.0"

"@auto-it/npm@link:plugins/npm":
version "9.48.2"
version "9.49.1"
dependencies:
"@auto-it/core" "link:packages/core"
await-to-js "^2.1.1"
Expand All @@ -113,7 +113,7 @@
user-home "^2.0.0"

"@auto-it/released@link:plugins/released":
version "9.48.2"
version "9.49.1"
dependencies:
"@auto-it/core" "link:packages/core"
deepmerge "^4.0.0"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down