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

fix(requests): got calls will be retried on response code 429 #361 #728

Closed
wants to merge 1 commit into from
Closed

fix(requests): got calls will be retried on response code 429 #361 #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions lib/definitions/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
export const HOME_URL = 'https://github.com/semantic-release/semantic-release';

export const RELEASE_NAME = 'GitLab release';

export const RETRY_CONFIGURATION = {
retry: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could try to calculate the retryAfter property into optimizing the calculation for the next try.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think got does that already, see https://github.com/sindresorhus/got/blob/cc346b79561b13e3a266b22b7f612fca5a45eb1e/source/core/index.ts#L357

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed. I assumed we are overriding the retry options when setting the apiOptions. I was wrong. Thanks for properly looking into it. I will close this PR.
What is still confusing though are the reported 429 responses of some users...

limit: 3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default in got is 2. I am open for discussion.

statusCodes: [429],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can go with the default here:

Suggested change
statusCodes: [429],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will close this PR. Thanks for reviewing!

},
};
8 changes: 7 additions & 1 deletion lib/fail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const debug = _debug("semantic-release:gitlab");
import resolveConfig from "./resolve-config.js";
import getRepoId from "./get-repo-id.js";
import getFailComment from "./get-fail-comment.js";
import { RETRY_CONFIGURATION } from "./definitions/constants.js";

export default async (pluginConfig, context) => {
const {
Expand All @@ -20,7 +21,12 @@ export default async (pluginConfig, context) => {
);
const repoId = getRepoId(context, gitlabUrl, repositoryUrl);
const encodedRepoId = encodeURIComponent(repoId);
const apiOptions = { headers: { "PRIVATE-TOKEN": gitlabToken } };
const apiOptions = {
headers: {
"PRIVATE-TOKEN": gitlabToken,
},
...RETRY_CONFIGURATION,
};

if (failComment === false || failTitle === false) {
logger.log("Skip issue creation.");
Expand Down
3 changes: 2 additions & 1 deletion lib/publish.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const debug = _debug("semantic-release:gitlab");
import resolveConfig from "./resolve-config.js";
import getRepoId from "./get-repo-id.js";
import getAssets from "./glob-assets.js";
import { RELEASE_NAME } from "./definitions/constants.js";
import { RELEASE_NAME, RETRY_CONFIGURATION } from "./definitions/constants.js";

const isUrlScheme = (value) => /^(https|http|ftp):\/\//.test(value);

Expand Down Expand Up @@ -46,6 +46,7 @@ export default async (pluginConfig, context) => {
},
],
},
...RETRY_CONFIGURATION,
};

debug("repoId: %o", repoId);
Expand Down
8 changes: 7 additions & 1 deletion lib/success.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const debug = _debug("semantic-release:gitlab");
import resolveConfig from "./resolve-config.js";
import getRepoId from "./get-repo-id.js";
import getSuccessComment from "./get-success-comment.js";
import { RETRY_CONFIGURATION } from "./definitions/constants.js";

export default async (pluginConfig, context) => {
const {
Expand All @@ -18,7 +19,12 @@ export default async (pluginConfig, context) => {
const { gitlabToken, gitlabUrl, gitlabApiUrl, successComment, proxy } = resolveConfig(pluginConfig, context);
const repoId = getRepoId(context, gitlabUrl, repositoryUrl);
const encodedRepoId = encodeURIComponent(repoId);
const apiOptions = { headers: { "PRIVATE-TOKEN": gitlabToken } };
const apiOptions = {
headers: {
"PRIVATE-TOKEN": gitlabToken,
},
...RETRY_CONFIGURATION,
};

if (successComment === false) {
logger.log("Skip commenting on issues and pull requests.");
Expand Down
9 changes: 8 additions & 1 deletion lib/verify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import AggregateError from "aggregate-error";
import resolveConfig from "./resolve-config.js";
import getRepoId from "./get-repo-id.js";
import getError from "./get-error.js";
import { RETRY_CONFIGURATION } from "./definitions/constants.js";

const isNonEmptyString = (value) => isString(value) && value.trim();
const isStringOrStringArray = (value) =>
Expand All @@ -33,6 +34,12 @@ export default async (pluginConfig, context) => {
} = context;
const { gitlabToken, gitlabUrl, gitlabApiUrl, proxy, ...options } = resolveConfig(pluginConfig, context);
const repoId = getRepoId(context, gitlabUrl, repositoryUrl);
const apiOptions = {
headers: {
"PRIVATE-TOKEN": gitlabToken,
},
...RETRY_CONFIGURATION,
};

debug("apiUrl: %o", gitlabApiUrl);
debug("repoId: %o", repoId);
Expand Down Expand Up @@ -65,7 +72,7 @@ export default async (pluginConfig, context) => {
permissions: { project_access: projectAccess, group_access: groupAccess },
} = await got
.get(urlJoin(gitlabApiUrl, `/projects/${encodeURIComponent(repoId)}`), {
headers: { "PRIVATE-TOKEN": gitlabToken },
...apiOptions,
...proxy,
})
.json());
Expand Down
24 changes: 24 additions & 0 deletions test/publish.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,3 +641,27 @@ test.serial("Publish a release with error response", async (t) => {
t.is(error.message, `Response code 499 (Something went wrong)`);
t.true(gitlab.isDone());
});

test.serial("Publish a release with error response of 429 - too many requests - with persist", async (t) => {
const owner = "test_user";
const repo = "test_repo";
const env = { GITLAB_TOKEN: "gitlab_token" };
const pluginConfig = {};
const nextRelease = { gitHead: "123", gitTag: "v1.0.0", notes: "Test release note body" };
const options = { repositoryUrl: `https://gitlab.com/${owner}/${repo}.git` };
const encodedRepoId = encodeURIComponent(`${owner}/${repo}`);
const gitlab = authenticate(env)
.post(`/projects/${encodedRepoId}/releases`, {
tag_name: nextRelease.gitTag,
description: nextRelease.notes,
assets: {
links: [],
},
})
.reply(429, { message: "Too many requests" })
.persist();
fgreinacher marked this conversation as resolved.
Show resolved Hide resolved

const error = await t.throwsAsync(publish(pluginConfig, { env, options, nextRelease, logger: t.context.logger }));
t.is(error.message, `Response code 429 (Too many requests)`);
t.true(gitlab.isDone());
});
2 changes: 1 addition & 1 deletion test/verify.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ test.serial("Throw error if GitLab API return any other errors", async (t) => {
const owner = "test_user";
const repo = "test_repo";
const env = { GITLAB_TOKEN: "gitlab_token" };
const gitlab = authenticate(env).get(`/projects/${owner}%2F${repo}`).times(3).reply(500);
const gitlab = authenticate(env).get(`/projects/${owner}%2F${repo}`).times(3).reply(500).persist();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.persist is needed to avoid confusing nock errors.

Copy link
Contributor

@fgreinacher fgreinacher Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See comment above


const error = await t.throwsAsync(
verify({}, { env, options: { repositoryUrl: `https://gitlab.com:${owner}/${repo}.git` }, logger: t.context.logger })
Expand Down