Skip to content

Commit 223264a

Browse files
gxueatlassiangxueatlassian
authored
ARC-2759 skip the error "Commit not found by sha..." if on last try (#2618)
1 parent 38acece commit 223264a

File tree

7 files changed

+205
-77
lines changed

7 files changed

+205
-77
lines changed

‎src/config/feature-flags.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ export enum BooleanFlags {
2626
USE_CUSTOM_ROOT_CA_BUNDLE = "use-custom-root-ca-bundle",
2727
GENERATE_CORE_HEAP_DUMPS_ON_LOW_MEM = "generate-core-heap-dumps-on-low-mem",
2828
USE_RATELIMIT_ON_JIRA_CLIENT = "use-ratelimit-on-jira-client",
29-
SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND = "skip-process-queue-when-issue-not-exists"
29+
SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND = "skip-process-queue-when-issue-not-exists",
30+
SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY = "skip-commit-if-sha-not-found-on-last-try"
3031
}
3132

3233
export enum StringFlags {

‎src/github/client/github-client-errors.ts

+7
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,13 @@ export class GithubClientNotFoundError extends GithubClientError {
8686
}
8787
}
8888

89+
export class GithubClientCommitNotFoundBySHAError extends GithubClientError {
90+
constructor(cause: AxiosError) {
91+
super("Commit not found by sha", cause);
92+
this.uiErrorCode = "RESOURCE_NOT_FOUND";
93+
}
94+
}
95+
8996

9097
/**
9198
* Type for errors section in GraphQL response

‎src/github/client/github-client-interceptors.test.ts

+19-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ import { getLogger } from "config/logger";
33
import {
44
GithubClientBlockedIpError,
55
GithubClientInvalidPermissionsError,
6-
GithubClientNotFoundError, GithubClientSSOLoginError
6+
GithubClientNotFoundError,
7+
GithubClientSSOLoginError,
8+
GithubClientCommitNotFoundBySHAError
79
} from "~/src/github/client/github-client-errors";
810

911
describe("github-client-interceptors", () => {
@@ -66,4 +68,20 @@ describe("github-client-interceptors", () => {
6668
}
6769
expect(error!).toBeInstanceOf(GithubClientSSOLoginError);
6870
});
71+
72+
it("correctly maps commits not found by sha error", async () => {
73+
gheNock.get("/").reply(422, {
74+
"message": "No commit found for SHA: whatever the sha is",
75+
"documentation_url": "https://docs.github.com"
76+
});
77+
78+
let error: Error;
79+
const client = await createAnonymousClient(gheUrl, jiraHost, { trigger: "test" }, getLogger("test"));
80+
try {
81+
await client.getPage(1000);
82+
} catch (err: unknown) {
83+
error = err as Error;
84+
}
85+
expect(error!).toBeInstanceOf(GithubClientCommitNotFoundBySHAError);
86+
});
6987
});

‎src/github/client/github-client-interceptors.ts

+13-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import {
55
GithubClientInvalidPermissionsError,
66
GithubClientRateLimitingError,
77
GithubClientNotFoundError,
8-
GithubClientSSOLoginError
8+
GithubClientSSOLoginError,
9+
GithubClientCommitNotFoundBySHAError
910
} from "./github-client-errors";
1011
import Logger from "bunyan";
1112
import { statsd } from "config/statsd";
@@ -166,6 +167,17 @@ export const handleFailedRequest = (rootLogger: Logger) =>
166167
return Promise.reject(mappedError);
167168
}
168169

170+
if (status === 422) {
171+
if ((String(err.response?.data?.message) || "").toLocaleLowerCase().includes("no commit found for sha")) {
172+
const mappedError = new GithubClientCommitNotFoundBySHAError(err);
173+
logger.warn({
174+
err: mappedError,
175+
remote: response.data.message
176+
}, "commit not found by sha");
177+
return Promise.reject(mappedError);
178+
}
179+
}
180+
169181
const isWarning = status && (status >= 300 && status < 500 && status !== 400);
170182

171183
if (isWarning) {

‎src/sqs/push.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,5 @@ export const pushQueueMessageHandler: MessageHandler<PushQueueMessagePayload> =
3030
subTrigger: "push"
3131
};
3232
const gitHubInstallationClient = await createInstallationClient(installationId, jiraHost, metrics, context.log, payload.gitHubAppConfig?.gitHubAppId);
33-
await processPush(gitHubInstallationClient, payload, context.log);
33+
await processPush(gitHubInstallationClient, context, payload, context.log);
3434
};

‎src/transforms/push.test.ts

+152-70
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,29 @@ import { GitHubCommit, GitHubRepository } from "interfaces/github";
55
import { shouldSendAll, booleanFlag, BooleanFlags, numberFlag, NumberFlags } from "config/feature-flags";
66
import { getLogger } from "config/logger";
77
import { GitHubInstallationClient } from "../github/client/github-installation-client";
8+
import { GithubClientCommitNotFoundBySHAError } from "../github/client/github-client-errors";
89
import { DatabaseStateCreator, CreatorResult } from "test/utils/database-state-creator";
910

1011
jest.mock("../sqs/queues");
1112
jest.mock("config/feature-flags");
1213
const logger = getLogger("test");
1314

1415
describe("Enqueue push", () => {
16+
17+
let db: CreatorResult;
18+
let issueKey: string;
19+
let sha: string;
20+
beforeEach(async () => {
21+
db = await new DatabaseStateCreator()
22+
.forCloud()
23+
.create();
24+
when(shouldSendAll).calledWith("commits", expect.anything(), expect.anything()).mockResolvedValue(false);
25+
when(numberFlag).calledWith(NumberFlags.SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND_TIMEOUT, expect.anything(), expect.anything())
26+
.mockResolvedValue(10000);
27+
issueKey = `KEY-${new Date().getTime()}`;
28+
sha = `sha-${issueKey}`;
29+
});
30+
1531
it("should push GitHubAppConfig to payload", async () => {
1632
await enqueuePush({
1733
installation: { id: 123, node_id: 456 },
@@ -105,21 +121,72 @@ describe("Enqueue push", () => {
105121
}), 0, expect.anything());
106122
});
107123

108-
describe("Skipping msg when issue not exist", () => {
124+
describe("Maybe skipp commit when ff is on and commimt sha on not found", () => {
125+
it("when ff is OFF, should throw error for the problematic commits if 422 and even on last try", async () => {
126+
127+
when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything())
128+
.mockResolvedValue(false);
129+
130+
githubUserTokenNock(db.subscription.gitHubInstallationId);
131+
githubUserTokenNock(db.subscription.gitHubInstallationId);
132+
mockGitHubCommitRestApi();
133+
mockGitHubCommitRestApi422("invalid-sha");
134+
135+
const pushPayload = getPushPayload();
136+
pushPayload.shas.push({
137+
id: "invalid-sha",
138+
issueKeys: [ issueKey ]
139+
});
140+
141+
await expect(async () => {
142+
await processPush(getGitHubClient(), getContext({ lastAttempt: true }), pushPayload, logger);
143+
}).rejects.toThrow(GithubClientCommitNotFoundBySHAError);
144+
});
145+
146+
it("should throw error for the problematic commits if 422 but NOT on last try", async () => {
147+
148+
when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything())
149+
.mockResolvedValue(true);
150+
151+
githubUserTokenNock(db.subscription.gitHubInstallationId);
152+
githubUserTokenNock(db.subscription.gitHubInstallationId);
153+
mockGitHubCommitRestApi();
154+
mockGitHubCommitRestApi422("invalid-sha");
155+
156+
const pushPayload = getPushPayload();
157+
pushPayload.shas.push({
158+
id: "invalid-sha",
159+
issueKeys: [ issueKey ]
160+
});
161+
162+
await expect(async () => {
163+
await processPush(getGitHubClient(), getContext({ lastAttempt: false }), pushPayload, logger);
164+
}).rejects.toThrow(GithubClientCommitNotFoundBySHAError);
165+
});
109166

110-
let db: CreatorResult;
111-
let issueKey;
112-
let sha;
113-
beforeEach(async () => {
114-
db = await new DatabaseStateCreator()
115-
.forCloud()
116-
.create();
117-
when(shouldSendAll).calledWith("commits", expect.anything(), expect.anything()).mockResolvedValue(false);
118-
when(numberFlag).calledWith(NumberFlags.SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND_TIMEOUT, expect.anything(), expect.anything())
119-
.mockResolvedValue(10000);
120-
issueKey = `KEY-${new Date().getTime()}`;
121-
sha = `sha-${issueKey}`;
167+
it("should success skip the problematic commits if 422 on last try", async () => {
168+
169+
when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything())
170+
.mockResolvedValue(true);
171+
172+
githubUserTokenNock(db.subscription.gitHubInstallationId);
173+
githubUserTokenNock(db.subscription.gitHubInstallationId);
174+
mockGitHubCommitRestApi();
175+
mockGitHubCommitRestApi422("invalid-sha");
176+
177+
mockJiraDevInfoAcceptUpdate();
178+
179+
const pushPayload = getPushPayload();
180+
pushPayload.shas.push({
181+
id: "invalid-sha",
182+
issueKeys: [ issueKey ]
183+
});
184+
185+
await processPush(getGitHubClient(), getContext({ lastAttempt: true }), pushPayload, logger);
122186
});
187+
});
188+
189+
describe("Skipping msg when issue not exist", () => {
123190

124191
describe("Use redis to avoid overload jira", () => {
125192
it("should reuse status from redis and only call jira once for same issue-key", async () => {
@@ -128,8 +195,8 @@ describe("Enqueue push", () => {
128195

129196
mockIssueNotExists();
130197

131-
await processPush(getGitHubClient(), getPushPayload(), logger);
132-
await processPush(getGitHubClient(), getPushPayload(), logger);
198+
await processPush(getGitHubClient(), getContext(), getPushPayload(), logger);
199+
await processPush(getGitHubClient(), getContext(), getPushPayload(), logger);
133200
});
134201
});
135202

@@ -140,7 +207,7 @@ describe("Enqueue push", () => {
140207

141208
mockIssueNotExists();
142209

143-
await processPush(getGitHubClient(), getPushPayload(), logger);
210+
await processPush(getGitHubClient(), getContext(), getPushPayload(), logger);
144211

145212
});
146213

@@ -156,7 +223,7 @@ describe("Enqueue push", () => {
156223

157224
mockJiraDevInfoAcceptUpdate();
158225

159-
await processPush(getGitHubClient(), getPushPayload(), logger);
226+
await processPush(getGitHubClient(), getContext(), getPushPayload(), logger);
160227

161228
});
162229

@@ -170,66 +237,81 @@ describe("Enqueue push", () => {
170237

171238
mockJiraDevInfoAcceptUpdate();
172239

173-
await processPush(getGitHubClient(), getPushPayload(), logger);
240+
await processPush(getGitHubClient(), getContext(), getPushPayload(), logger);
174241

175242
});
243+
});
176244

177-
const mockJiraDevInfoAcceptUpdate = () => {
178-
jiraNock.post("/rest/devinfo/0.10/bulk", (reqBody) => {
179-
return reqBody.repositories[0].commits.flatMap(c => c.issueKeys).some(ck => ck === issueKey);
180-
}).reply(202, "");
181-
};
182-
183-
const mockGitHubCommitRestApi = () => {
184-
githubNock
185-
.get("/repos/org1/repo1/commits/" + sha)
186-
.reply(200, {
187-
files: [],
188-
sha
189-
});
190-
};
191-
192-
const getPushPayload = () => {
193-
return {
194-
jiraHost,
195-
installationId: db.subscription.gitHubInstallationId,
196-
gitHubAppConfig: undefined,
197-
webhookId: "aaa",
198-
repository: {
199-
owner: { login: "org1" },
200-
name: "repo1"
201-
} as GitHubRepository,
202-
shas: [{
203-
id: sha,
204-
issueKeys: [issueKey]
205-
}]
206-
};
245+
const mockJiraDevInfoAcceptUpdate = () => {
246+
jiraNock.post("/rest/devinfo/0.10/bulk", (reqBody) => {
247+
return reqBody.repositories[0].commits.flatMap(c => c.issueKeys).some(ck => ck === issueKey);
248+
}).reply(202, "");
249+
};
250+
251+
const mockGitHubCommitRestApi = () => {
252+
githubNock
253+
.get("/repos/org1/repo1/commits/" + sha)
254+
.reply(200, {
255+
files: [],
256+
sha
257+
});
258+
};
259+
260+
const mockGitHubCommitRestApi422 = (sha: string) => {
261+
githubNock
262+
.get("/repos/org1/repo1/commits/" + sha)
263+
.reply(422, {
264+
message: `No commit found for SHA: ${sha}`,
265+
documentation_url: "https://docs.github.com"
266+
});
267+
};
268+
269+
const getPushPayload = () => {
270+
return {
271+
jiraHost,
272+
installationId: db.subscription.gitHubInstallationId,
273+
gitHubAppConfig: undefined,
274+
webhookId: "aaa",
275+
repository: {
276+
owner: { login: "org1" },
277+
name: "repo1"
278+
} as GitHubRepository,
279+
shas: [{
280+
id: sha,
281+
issueKeys: [issueKey]
282+
}]
207283
};
284+
};
208285

209-
const mockIssueExists = () => {
210-
jiraNock.get(`/rest/api/latest/issue/${issueKey}`)
211-
.query({ fields: "summary" }).reply(200, {});
212-
};
286+
const mockIssueExists = () => {
287+
jiraNock.get(`/rest/api/latest/issue/${issueKey}`)
288+
.query({ fields: "summary" }).reply(200, {});
289+
};
213290

214-
const mockIssueNotExists = () => {
215-
jiraNock.get(`/rest/api/latest/issue/${issueKey}`)
216-
.query({ fields: "summary" }).reply(404, "");
217-
};
291+
const mockIssueNotExists = () => {
292+
jiraNock.get(`/rest/api/latest/issue/${issueKey}`)
293+
.query({ fields: "summary" }).reply(404, "");
294+
};
218295

219-
const getGitHubClient = () => {
220-
return new GitHubInstallationClient({
221-
appId: 2,
222-
githubBaseUrl: "https://api.github.com",
223-
installationId: db.subscription.gitHubInstallationId
224-
}, {
225-
apiUrl: "https://api.github.com",
226-
baseUrl: "https://github.com",
227-
graphqlUrl: "https://api.github.com/graphql",
228-
hostname: "https://github.com",
229-
apiKeyConfig: undefined,
230-
proxyBaseUrl: undefined
231-
}, jiraHost, { trigger: "test" }, logger, undefined);
296+
const getGitHubClient = () => {
297+
return new GitHubInstallationClient({
298+
appId: 2,
299+
githubBaseUrl: "https://api.github.com",
300+
installationId: db.subscription.gitHubInstallationId
301+
}, {
302+
apiUrl: "https://api.github.com",
303+
baseUrl: "https://github.com",
304+
graphqlUrl: "https://api.github.com/graphql",
305+
hostname: "https://github.com",
306+
apiKeyConfig: undefined,
307+
proxyBaseUrl: undefined
308+
}, jiraHost, { trigger: "test" }, logger, undefined);
309+
};
310+
311+
const getContext = (override?: any): any => {
312+
return {
313+
...override
232314
};
233-
});
315+
};
234316

235317
});

0 commit comments

Comments
 (0)