From 14cc3aae42c5b54ae23925d7e523e99773c6eec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=B8ller=20Ellehauge?= Date: Wed, 13 Mar 2024 15:12:08 -0700 Subject: [PATCH 1/4] fix: also retry secondary rate limits --- workspaces/data/lib/api/rest.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/workspaces/data/lib/api/rest.js b/workspaces/data/lib/api/rest.js index 33e45625b..56a8f5d63 100644 --- a/workspaces/data/lib/api/rest.js +++ b/workspaces/data/lib/api/rest.js @@ -32,6 +32,12 @@ module.exports = ({ auth }) => { octokit.log.warn( `SecondaryRateLimit detected for request ${options.method} ${options.url}` ) + + if (options.request.retryCount === 0) { + // only retries once + octokit.log.info(`Retrying after ${retryAfter} seconds!`) + return true + } }, }, }) From f50b41394177d5759347a875cee559a3b411c74f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=B8ller=20Ellehauge?= Date: Wed, 13 Mar 2024 15:17:18 -0700 Subject: [PATCH 2/4] lintfix --- workspaces/data/lib/api/rest.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/workspaces/data/lib/api/rest.js b/workspaces/data/lib/api/rest.js index 56a8f5d63..ec315b68c 100644 --- a/workspaces/data/lib/api/rest.js +++ b/workspaces/data/lib/api/rest.js @@ -33,11 +33,11 @@ module.exports = ({ auth }) => { `SecondaryRateLimit detected for request ${options.method} ${options.url}` ) - if (options.request.retryCount === 0) { - // only retries once - octokit.log.info(`Retrying after ${retryAfter} seconds!`) - return true - } + if (options.request.retryCount === 0) { + // only retries once + octokit.log.info(`Retrying after ${retryAfter} seconds!`) + return true + } }, }, }) From 623b40b946fe88af4511a11181ad4c14be2595cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=B8ller=20Ellehauge?= Date: Wed, 13 Mar 2024 15:19:10 -0700 Subject: [PATCH 3/4] rename variable so it's actually there --- workspaces/data/lib/api/rest.js | 119 ++++++++++++++++++-------------- 1 file changed, 67 insertions(+), 52 deletions(-) diff --git a/workspaces/data/lib/api/rest.js b/workspaces/data/lib/api/rest.js index ec315b68c..00dbae25b 100644 --- a/workspaces/data/lib/api/rest.js +++ b/workspaces/data/lib/api/rest.js @@ -1,9 +1,9 @@ -const { Octokit } = require('@octokit/rest') -const { retry } = require('@octokit/plugin-retry') -const { throttling } = require('@octokit/plugin-throttling') -const log = require('proc-log') -const { groupBy, orderBy } = require('lodash') -const config = require('../config') +const { Octokit } = require("@octokit/rest"); +const { retry } = require("@octokit/plugin-retry"); +const { throttling } = require("@octokit/plugin-throttling"); +const log = require("proc-log"); +const { groupBy, orderBy } = require("lodash"); +const config = require("../config"); module.exports = ({ auth }) => { const REST = new (Octokit.plugin(retry, throttling))({ @@ -20,46 +20,48 @@ module.exports = ({ auth }) => { onRateLimit: (retryAfter, options, octokit) => { octokit.log.warn( `Request quota exhausted for request ${options.method} ${options.url}` - ) + ); if (options.request.retryCount === 0) { // only retries once - octokit.log.info(`Retrying after ${retryAfter} seconds!`) - return true + octokit.log.info(`Retrying after ${retryAfter} seconds!`); + return true; } }, - onSecondaryRateLimit: (__, options, octokit) => { + onSecondaryRateLimit: (retryAfter, options, octokit) => { octokit.log.warn( `SecondaryRateLimit detected for request ${options.method} ${options.url}` - ) + ); if (options.request.retryCount === 0) { // only retries once - octokit.log.info(`Retrying after ${retryAfter} seconds!`) - return true + octokit.log.info(`Retrying after ${retryAfter} seconds!`); + return true; } }, }, - }) + }); const getRepo = (owner, repo) => { - log.verbose(`rest:repo:get`, `${owner}/${repo}`) - return REST.repos.get({ owner, repo }).then(d => d.data) - } + log.verbose(`rest:repo:get`, `${owner}/${repo}`); + return REST.repos.get({ owner, repo }).then((d) => d.data); + }; const getCommit = async (owner, name, p) => { - log.verbose(`rest:repo:commit`, `${owner}/${name}${p ? `/${p}` : ''}`) - - return REST.repos.listCommits({ - owner, - repo: name, - path: p, - per_page: 1, - }).then((r) => r.data[0]) - } + log.verbose(`rest:repo:commit`, `${owner}/${name}${p ? `/${p}` : ""}`); + + return REST.repos + .listCommits({ + owner, + repo: name, + path: p, + per_page: 1, + }) + .then((r) => r.data[0]); + }; const getStatus = async (owner, name, ref) => { - log.verbose(`rest:repo:status`, `${owner}/${name}#${ref}`) + log.verbose(`rest:repo:status`, `${owner}/${name}#${ref}`); const allCheckRuns = await REST.paginate(REST.checks.listForRef, { owner, @@ -70,56 +72,69 @@ module.exports = ({ auth }) => { // makes it very flaky and prone to 500 for npm/cli. its safer (but slower) // to make more but smaller requests per_page: 30, - }) + }); // For some of our repos that don't get very many commits, we still run // a lot of scheduled actions against them. This reduces a status to the // latest check for each run name (eg, 'test (12.13.0, windows-latest, cmd)') - const runsByName = groupBy(allCheckRuns, 'name') - - const checkRuns = Object.entries(runsByName).reduce((acc, [checkName, runs]) => { - if (config.checkRunFilter(checkName, runs)) { - const [latestRun] = orderBy(runs, 'completed_at', 'desc') - acc.push(latestRun) - } - return acc - }, []) - - log.verbose(`rest:repo:status:names`, checkRuns.map(c => c.name).join('\n')) + const runsByName = groupBy(allCheckRuns, "name"); - const failures = ['action_required', 'cancelled', 'failure', 'stale', 'timed_out', null] - const statuses = { neutral: false, success: false, skipped: false } + const checkRuns = Object.entries(runsByName).reduce( + (acc, [checkName, runs]) => { + if (config.checkRunFilter(checkName, runs)) { + const [latestRun] = orderBy(runs, "completed_at", "desc"); + acc.push(latestRun); + } + return acc; + }, + [] + ); + + log.verbose( + `rest:repo:status:names`, + checkRuns.map((c) => c.name).join("\n") + ); + + const failures = [ + "action_required", + "cancelled", + "failure", + "stale", + "timed_out", + null, + ]; + const statuses = { neutral: false, success: false, skipped: false }; for (const checkRun of checkRuns) { // return early for any failures if (failures.includes(checkRun.conclusion)) { - return { url: checkRun.html_url, conclusion: 'failure' } + return { url: checkRun.html_url, conclusion: "failure" }; } - statuses[checkRun.conclusion] = true + statuses[checkRun.conclusion] = true; } // Skipped or neutral and no successes is neutral if ((statuses.neutral || statuses.skipped) && !statuses.success) { - return { conclusion: 'neutral' } + return { conclusion: "neutral" }; } // otherwise allow some neutral/skipped as long as there // are other successful runs - return { conclusion: 'success' } - } + return { conclusion: "success" }; + }; - const getAllIssuesAndPullRequests = (owner, name, query = '') => { - log.verbose('rest:issues:getAll', `${owner}/${name}`) + const getAllIssuesAndPullRequests = (owner, name, query = "") => { + log.verbose("rest:issues:getAll", `${owner}/${name}`); return REST.paginate(REST.search.issuesAndPullRequests, { q: `repo:${owner}/${name}+${query}`, - }) - } + }); + }; return { getRepo, getCommit, getStatus, getAllIssuesAndPullRequests, - } -} + }; +}; From b0ab11e09eb41e2a8f87ad53e83c2f11432e9177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julian=20M=C3=B8ller=20Ellehauge?= Date: Wed, 13 Mar 2024 15:21:58 -0700 Subject: [PATCH 4/4] avoid prettier --- workspaces/data/lib/api/rest.js | 117 ++++++++++++++------------------ 1 file changed, 51 insertions(+), 66 deletions(-) diff --git a/workspaces/data/lib/api/rest.js b/workspaces/data/lib/api/rest.js index 00dbae25b..d7461b736 100644 --- a/workspaces/data/lib/api/rest.js +++ b/workspaces/data/lib/api/rest.js @@ -1,9 +1,9 @@ -const { Octokit } = require("@octokit/rest"); -const { retry } = require("@octokit/plugin-retry"); -const { throttling } = require("@octokit/plugin-throttling"); -const log = require("proc-log"); -const { groupBy, orderBy } = require("lodash"); -const config = require("../config"); +const { Octokit } = require('@octokit/rest') +const { retry } = require('@octokit/plugin-retry') +const { throttling } = require('@octokit/plugin-throttling') +const log = require('proc-log') +const { groupBy, orderBy } = require('lodash') +const config = require('../config') module.exports = ({ auth }) => { const REST = new (Octokit.plugin(retry, throttling))({ @@ -20,48 +20,46 @@ module.exports = ({ auth }) => { onRateLimit: (retryAfter, options, octokit) => { octokit.log.warn( `Request quota exhausted for request ${options.method} ${options.url}` - ); + ) if (options.request.retryCount === 0) { // only retries once - octokit.log.info(`Retrying after ${retryAfter} seconds!`); - return true; + octokit.log.info(`Retrying after ${retryAfter} seconds!`) + return true } }, onSecondaryRateLimit: (retryAfter, options, octokit) => { octokit.log.warn( `SecondaryRateLimit detected for request ${options.method} ${options.url}` - ); + ) if (options.request.retryCount === 0) { // only retries once - octokit.log.info(`Retrying after ${retryAfter} seconds!`); - return true; + octokit.log.info(`Retrying after ${retryAfter} seconds!`) + return true } }, }, - }); + }) const getRepo = (owner, repo) => { - log.verbose(`rest:repo:get`, `${owner}/${repo}`); - return REST.repos.get({ owner, repo }).then((d) => d.data); - }; + log.verbose(`rest:repo:get`, `${owner}/${repo}`) + return REST.repos.get({ owner, repo }).then(d => d.data) + } const getCommit = async (owner, name, p) => { - log.verbose(`rest:repo:commit`, `${owner}/${name}${p ? `/${p}` : ""}`); - - return REST.repos - .listCommits({ - owner, - repo: name, - path: p, - per_page: 1, - }) - .then((r) => r.data[0]); - }; + log.verbose(`rest:repo:commit`, `${owner}/${name}${p ? `/${p}` : ''}`) + + return REST.repos.listCommits({ + owner, + repo: name, + path: p, + per_page: 1, + }).then((r) => r.data[0]) + } const getStatus = async (owner, name, ref) => { - log.verbose(`rest:repo:status`, `${owner}/${name}#${ref}`); + log.verbose(`rest:repo:status`, `${owner}/${name}#${ref}`) const allCheckRuns = await REST.paginate(REST.checks.listForRef, { owner, @@ -72,69 +70,56 @@ module.exports = ({ auth }) => { // makes it very flaky and prone to 500 for npm/cli. its safer (but slower) // to make more but smaller requests per_page: 30, - }); + }) // For some of our repos that don't get very many commits, we still run // a lot of scheduled actions against them. This reduces a status to the // latest check for each run name (eg, 'test (12.13.0, windows-latest, cmd)') - const runsByName = groupBy(allCheckRuns, "name"); + const runsByName = groupBy(allCheckRuns, 'name') - const checkRuns = Object.entries(runsByName).reduce( - (acc, [checkName, runs]) => { - if (config.checkRunFilter(checkName, runs)) { - const [latestRun] = orderBy(runs, "completed_at", "desc"); - acc.push(latestRun); - } - return acc; - }, - [] - ); - - log.verbose( - `rest:repo:status:names`, - checkRuns.map((c) => c.name).join("\n") - ); - - const failures = [ - "action_required", - "cancelled", - "failure", - "stale", - "timed_out", - null, - ]; - const statuses = { neutral: false, success: false, skipped: false }; + const checkRuns = Object.entries(runsByName).reduce((acc, [checkName, runs]) => { + if (config.checkRunFilter(checkName, runs)) { + const [latestRun] = orderBy(runs, 'completed_at', 'desc') + acc.push(latestRun) + } + return acc + }, []) + + log.verbose(`rest:repo:status:names`, checkRuns.map(c => c.name).join('\n')) + + const failures = ['action_required', 'cancelled', 'failure', 'stale', 'timed_out', null] + const statuses = { neutral: false, success: false, skipped: false } for (const checkRun of checkRuns) { // return early for any failures if (failures.includes(checkRun.conclusion)) { - return { url: checkRun.html_url, conclusion: "failure" }; + return { url: checkRun.html_url, conclusion: 'failure' } } - statuses[checkRun.conclusion] = true; + statuses[checkRun.conclusion] = true } // Skipped or neutral and no successes is neutral if ((statuses.neutral || statuses.skipped) && !statuses.success) { - return { conclusion: "neutral" }; + return { conclusion: 'neutral' } } // otherwise allow some neutral/skipped as long as there // are other successful runs - return { conclusion: "success" }; - }; + return { conclusion: 'success' } + } - const getAllIssuesAndPullRequests = (owner, name, query = "") => { - log.verbose("rest:issues:getAll", `${owner}/${name}`); + const getAllIssuesAndPullRequests = (owner, name, query = '') => { + log.verbose('rest:issues:getAll', `${owner}/${name}`) return REST.paginate(REST.search.issuesAndPullRequests, { q: `repo:${owner}/${name}+${query}`, - }); - }; + }) + } return { getRepo, getCommit, getStatus, getAllIssuesAndPullRequests, - }; -}; + } +}