Skip to content

Commit

Permalink
NONE eslint sqs (#2442)
Browse files Browse the repository at this point in the history
* NONE eslint sqs
  • Loading branch information
atraversatlassian authored Sep 20, 2023
1 parent 23c61bb commit ccbe4ae
Show file tree
Hide file tree
Showing 18 changed files with 313 additions and 189 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"rules": {
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/no-non-null-assertion": "off",
"jest/no-alias-methods": "off",
"jest/no-done-callback": "off",
"jest/expect-expect": [
"error",
Expand All @@ -103,7 +104,6 @@
"src/models/**",
"src/rest/**",
"src/routes/**",
"src/sqs/**",
"src/sync/**",
"src/transforms/**",
"src/util/**"
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@
"@typescript-eslint/parser": "~5.62.0",
"chai": "^4.3.6",
"dotenv-cli": "^6.0.0",
"eslint": "~8.9.0",
"eslint": "^8.49.0",
"eslint-config-prettier": "~8.3.0",
"eslint-import-resolver-typescript": "^2.5.0",
"eslint-plugin-import": "^2.25.4",
"eslint-plugin-jest": "~26.1.1",
"eslint-plugin-jest": "^27.4.0",
"eslint-plugin-jsdoc": "^37.9.3",
"eslint-plugin-no-only-tests": "^2.6.0",
"eslint-plugin-node": "^11.1.0",
Expand Down
2 changes: 1 addition & 1 deletion src/models/axios-error-event-decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class AxiosErrorEventDecorator {
return this.response?.request;
}

static decorate(event: any, hint: any): any {
static decorate(this: void, event: any, hint: any): any {
return new AxiosErrorEventDecorator(event, hint).decorate();
}

Expand Down
2 changes: 1 addition & 1 deletion src/models/sentry-scope-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class SentryScopeProxy {
this.extra = {};
}

static processEvent(event: any, hint: any) {
static processEvent(this: void, event: any, hint: any) {
if (hint.originalException.sentryScope) {
hint.originalException.sentryScope.addTo(event);
}
Expand Down
5 changes: 2 additions & 3 deletions src/sqs/backfill-discovery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,9 @@ describe("Discovery Queue Test - GitHub Client", () => {

when(numberFlag).calledWith(
NumberFlags.PREEMPTIVE_RATE_LIMIT_THRESHOLD,
expect.anything(),
expect.anything()
100,
jiraHost
).mockResolvedValue(100);

});

afterEach(async () => {
Expand Down
35 changes: 18 additions & 17 deletions src/sqs/backfill-error-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ describe("backfillErrorHandler", () => {
const client = await createAnonymousClient(gheUrl, jiraHost, { trigger: "test" }, getLogger("test"));
try {
await client.getPage(1000);
} catch (err) {
return err;
} catch (err: unknown) {
return err as GithubClientRateLimitingError;
}
return undefined;
};
Expand All @@ -78,8 +78,8 @@ describe("backfillErrorHandler", () => {
const client = await createAnonymousClient(gheUrl, jiraHost, { trigger: "test" }, getLogger("test"));
try {
await client.getPage(1000);
} catch (err) {
return err;
} catch (err: unknown) {
return err as GithubClientError;
}
return undefined;
};
Expand All @@ -91,8 +91,8 @@ describe("backfillErrorHandler", () => {

try {
await client?.appPropertiesGet();
} catch (ex) {
return ex;
} catch (ex: unknown) {
return ex as JiraClientError;
}
return undefined;
};
Expand All @@ -101,9 +101,7 @@ describe("backfillErrorHandler", () => {
({
receiveCount, lastAttempt, log: getLogger("test"), message: {}, payload: {
jiraHost,
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
installationId: subscription?.gitHubInstallationId
installationId: subscription?.gitHubInstallationId || 0
}
});

Expand Down Expand Up @@ -147,8 +145,8 @@ describe("backfillErrorHandler", () => {
database: "your_database"
});
await sequelize.authenticate();
} catch (err) {
sequelizeConnectionError = err;
} catch (err: unknown) {
sequelizeConnectionError = err as Error;
}

const result = await backfillErrorHandler(jest.fn())(new TaskError(task, sequelizeConnectionError), createContext(2, false));
Expand Down Expand Up @@ -192,10 +190,11 @@ describe("backfillErrorHandler", () => {
expect(result).toEqual({
isFailure: false
});
expect(sendMessageMock.mock.calls[0][0]).toEqual(
const mockMessage = sendMessageMock.mock.calls[0] as any[];
expect(mockMessage[0]).toEqual(
{ installationId: DatabaseStateCreator.GITHUB_INSTALLATION_ID, jiraHost }
);
expect(sendMessageMock.mock.calls[0][1]).toEqual(0);
expect(mockMessage[1]).toEqual(0);
expect((await RepoSyncState.findByPk(repoSyncState!.id))?.commitStatus).toEqual("failed");
});

Expand All @@ -208,10 +207,11 @@ describe("backfillErrorHandler", () => {
expect(result).toEqual({
isFailure: false
});
expect(sendMessageMock.mock.calls[0][0]).toEqual(
const mockMessage = sendMessageMock.mock.calls[0] as any[];
expect(mockMessage[0]).toEqual(
{ installationId: DatabaseStateCreator.GITHUB_INSTALLATION_ID, jiraHost }
);
expect(sendMessageMock.mock.calls[0][1]).toEqual(0);
expect(mockMessage[1]).toEqual(0);
expect((await RepoSyncState.findByPk(repoSyncState!.id))?.commitStatus).toEqual("failed");
expect((await Subscription.findByPk(repoSyncState!.subscriptionId))?.syncWarning).toEqual("Invalid permissions for commit task");
});
Expand Down Expand Up @@ -257,10 +257,11 @@ describe("backfillErrorHandler", () => {
expect(result).toEqual({
isFailure: false
});
expect(sendMessageMock.mock.calls[0][0]).toEqual(
const mockMessage = sendMessageMock.mock.calls[0] as any[];
expect(mockMessage[0]).toEqual(
{ installationId: DatabaseStateCreator.GITHUB_INSTALLATION_ID, jiraHost }
);
expect(sendMessageMock.mock.calls[0][1]).toEqual(0);
expect(mockMessage[1]).toEqual(0);
expect((await RepoSyncState.findByPk(repoSyncState!.id))?.commitStatus).toEqual("complete");
});
});
4 changes: 2 additions & 2 deletions src/sqs/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ export const backfillQueueMessageHandler =
}

try {
const processor = await processInstallation(sendSQSBackfillMessage);
const processor = processInstallation(sendSQSBackfillMessage);
await processor(backfillData, sentry, context.log);
} catch (err) {
} catch (err: unknown) {
logAdditionalData ? context.log.warn({ err, installationId }, "processInstallation threw a error")
: context.log.warn({ err }, "processInstallation threw a error");

Expand Down
23 changes: 13 additions & 10 deletions src/sqs/branch.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { Installation } from "models/installation";
import { Subscription } from "models/subscription";
import { waitUntil } from "test/utils/wait-until";
Expand Down Expand Up @@ -31,7 +30,7 @@ describe("Branch Webhook", () => {
jiraHost,
jiraClientKey: clientKey
});
await sqsQueues.branch.start();
sqsQueues.branch.start();
});

afterEach(async () => {
Expand Down Expand Up @@ -109,35 +108,38 @@ describe("Branch Webhook", () => {

mockSystemTime(12345678);

await expect(app.receive(branchBasic as any)).toResolve();
await expect(app.receive(branchBasic)).toResolve();

await waitUntil(async () => {
await waitUntil(() => {
expect(githubNock).toBeDone();
expect(jiraNock).toBeDone();
return Promise.resolve();
});
});

it("should not update Jira issue if there are no issue Keys in the branch name", async () => {
const getLastCommit = jest.fn();

await expect(app.receive(branchNoIssues as any)).toResolve();
await expect(app.receive(branchNoIssues)).toResolve();
expect(getLastCommit).not.toBeCalled();

await waitUntil(async () => {
await waitUntil(() => {
expect(githubNock).toBeDone();
expect(jiraNock).toBeDone();
return Promise.resolve();
});
});

it("should exit early if ref_type is not a branch", async () => {
const parseSmartCommit = jest.fn();

await expect(app.receive(branchInvalidRef as any)).toResolve();
await expect(app.receive(branchInvalidRef)).toResolve();
expect(parseSmartCommit).not.toBeCalled();

await waitUntil(async () => {
await waitUntil(() => {
expect(githubNock).toBeDone();
expect(jiraNock).toBeDone();
return Promise.resolve();
});
});
});
Expand All @@ -153,11 +155,12 @@ describe("Branch Webhook", () => {

mockSystemTime(12345678);

await expect(app.receive(branchDelete as any)).toResolve();
await expect(app.receive(branchDelete)).toResolve();

await waitUntil(async () => {
await waitUntil(() => {
expect(githubNock).toBeDone();
expect(jiraNock).toBeDone();
return Promise.resolve();
});
});
});
Expand Down
5 changes: 5 additions & 0 deletions src/sqs/branch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import { getCloudOrServerFromGitHubAppId } from "utils/get-cloud-or-server";

export const branchQueueMessageHandler: MessageHandler<BranchMessagePayload> = async (context: SQSMessageContext<BranchMessagePayload>) => {
const messagePayload: BranchMessagePayload = context.payload;
if (messagePayload.webhookReceived === undefined || messagePayload.webhookPayload === undefined || messagePayload.webhookId === undefined) {
context.log.error({ messagePayload }, "Missing required fields");
return;
}

const { webhookId, installationId, jiraHost } = context.payload;
context.log = context.log.child({
webhookId,
Expand Down
10 changes: 5 additions & 5 deletions src/sqs/deployment.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { Installation } from "models/installation";
import { Subscription } from "models/subscription";
import { waitUntil } from "test/utils/wait-until";
Expand Down Expand Up @@ -32,8 +31,8 @@ const mockGitHubRateLimit = (limit: number, remaining: number, resetTime: number
const mockRatelimitThreshold = (threshold: number) => {
when(numberFlag).calledWith(
NumberFlags.PREEMPTIVE_RATE_LIMIT_THRESHOLD,
expect.anything(),
expect.anything()
100,
jiraHost
).mockResolvedValue(threshold);
};

Expand Down Expand Up @@ -202,11 +201,12 @@ describe("Deployment Webhook", () => {
operationType: "NORMAL"
}).reply(200);

await expect(app.receive(deploymentStatusBasic as any)).toResolve();
await expect(app.receive(deploymentStatusBasic)).toResolve();

await waitUntil(async () => {
await waitUntil(() => {
expect(githubNock).toBeDone();
expect(jiraNock).toBeDone();
return Promise.resolve();
});
});
});
Expand Down
4 changes: 4 additions & 0 deletions src/sqs/deployment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import { DeploymentMessagePayload, MessageHandler, SQSMessageContext } from "./s

export const deploymentQueueMessageHandler: MessageHandler<DeploymentMessagePayload> = async (context: SQSMessageContext<DeploymentMessagePayload>) => {
const messagePayload: DeploymentMessagePayload = context.payload;
if (messagePayload.webhookReceived === undefined || messagePayload.webhookPayload === undefined || messagePayload.webhookId === undefined) {
context.log.error({ messagePayload }, "Missing required fields");
}

const { webhookId, jiraHost, installationId } = messagePayload;

context.log = context.log.child({
Expand Down
12 changes: 6 additions & 6 deletions src/sqs/error-handlers.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { statsd } from "config/statsd";
import { jiraAndGitHubErrorsHandler, webhookMetricWrapper } from "./error-handlers";
import { getLogger } from "config/logger";
Expand All @@ -20,6 +19,7 @@ describe("error-handlers", () => {

afterEach(() => {
// Unlock Time
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
statsdIncrementSpy.mockRestore();
jest.useRealTimers();
});
Expand Down Expand Up @@ -142,7 +142,7 @@ describe("error-handlers", () => {
it("Doesn't sent metric for a non-error case when not retryable", async () => {

const mockedResponse: ErrorHandlingResult = { retryable: false, isFailure: false };
const handlerUnderTest = webhookMetricWrapper(async () => mockedResponse, "test");
const handlerUnderTest = webhookMetricWrapper(() => Promise.resolve(mockedResponse), "test");

const result = await handlerUnderTest(new Error(), createContext(1, false));
expect(result).toBe(mockedResponse);
Expand All @@ -152,7 +152,7 @@ describe("error-handlers", () => {
it("Doesn't sent metric for a non-error case when lastAttempt", async () => {

const mockedResponse: ErrorHandlingResult = { retryable: true, isFailure: false };
const handlerUnderTest = webhookMetricWrapper(async () => mockedResponse, "test");
const handlerUnderTest = webhookMetricWrapper(() => Promise.resolve(mockedResponse), "test");

const result = await handlerUnderTest(new Error(), createContext(3, true));
expect(result).toBe(mockedResponse);
Expand All @@ -162,7 +162,7 @@ describe("error-handlers", () => {
it("Doesn't sent metric for an error when retryable but not last attempt", async () => {

const mockedResponse: ErrorHandlingResult = { retryable: true, isFailure: true };
const handlerUnderTest = webhookMetricWrapper(async () => mockedResponse, "test");
const handlerUnderTest = webhookMetricWrapper(() => Promise.resolve(mockedResponse), "test");

const result = await handlerUnderTest(new Error(), createContext(2, false));
expect(result).toBe(mockedResponse);
Expand All @@ -172,7 +172,7 @@ describe("error-handlers", () => {
it("Sends metric for an error case when not retryable", async () => {

const mockedResponse: ErrorHandlingResult = { retryable: false, isFailure: true };
const handlerUnderTest = webhookMetricWrapper(async () => mockedResponse, "test");
const handlerUnderTest = webhookMetricWrapper(() => Promise.resolve(mockedResponse), "test");

const result = await handlerUnderTest(new Error(), createContext(1, false));
expect(result).toBe(mockedResponse);
Expand All @@ -182,7 +182,7 @@ describe("error-handlers", () => {
it("Sends metric for a non-error case when lastAttempt", async () => {

const mockedResponse: ErrorHandlingResult = { retryable: true, isFailure: true };
const handlerUnderTest = webhookMetricWrapper(async () => mockedResponse, "test");
const handlerUnderTest = webhookMetricWrapper(() => Promise.resolve(mockedResponse), "test");

const result = await handlerUnderTest(new Error(), createContext(3, true));
expect(result).toBe(mockedResponse);
Expand Down
6 changes: 3 additions & 3 deletions src/sqs/error-handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ const RATE_LIMITING_DELAY_BUFFER_SEC = 10;
const EXPONENTIAL_BACKOFF_BASE_SEC = 60;
const EXPONENTIAL_BACKOFF_MULTIPLIER = 3;

export const handleUnknownError: ErrorHandler<BaseMessagePayload> = async <MessagePayload extends BaseMessagePayload>(
export const handleUnknownError: ErrorHandler<BaseMessagePayload> = <MessagePayload extends BaseMessagePayload>(
err: Error,
context: SQSMessageContext<MessagePayload>
): Promise<ErrorHandlingResult> => {
const delaySec = EXPONENTIAL_BACKOFF_BASE_SEC * Math.pow(EXPONENTIAL_BACKOFF_MULTIPLIER, context.receiveCount);
context.log.warn({ err, delaySec }, "Unknown error: retrying with exponential backoff");
return { retryable: true, retryDelaySec: delaySec, isFailure: true };
return Promise.resolve({ retryable: true, retryDelaySec: delaySec, isFailure: true });
};

export const jiraAndGitHubErrorsHandler: ErrorHandler<BaseMessagePayload> = async <MessagePayload extends BaseMessagePayload> (error: Error,
Expand Down Expand Up @@ -75,7 +75,7 @@ const maybeHandleNonRetryableResponseCode = <MessagePayload extends BaseMessageP
//Unfortunately we can't check if error is instance of Octokit.HookError because it is not a class, so we'll just rely on status
//New GitHub Client error (GithubClientError) also has status parameter, so it will be covered by the following check too
//TODO When we get rid of Octokit completely add check if (error instanceof GithubClientError) before the following code
const status: number | undefined = error["status"];
const status: number | undefined = error["status"] as number | undefined;
if (status && UNRETRYABLE_STATUS_CODES.includes(status)) {
context.log.warn({ err: error }, `Received error with ${status} status. Unretryable. Discarding the message`);
return { retryable: false, isFailure: false };
Expand Down
Loading

0 comments on commit ccbe4ae

Please sign in to comment.