Skip to content

Commit

Permalink
fix: remove fetch override for octokit and versions (#4042)
Browse files Browse the repository at this point in the history
## Problem
Updating octkit auth app to 6.1.1. fails runtime. Not in our tests due
to most of the SDK layer is mocked.

Only information we catch is a generic Error.
```
 HttpError: response.json(...).catch is not a function
```

Via [issue](octokit/auth-app.js#634)
@wolfy1339 provides feedback that the cause could be related to
overriding the fetch in octokit. In PR #3554 where tracing was
introduced also the fetch command was overriden, this due the fact the
node fetch was not working properly for tracing HTTP calls, which are in
our case the cals to GitHub. This problem seems now to be fixed in the
[AWS xray
sdk](https://github.com/aws/aws-xray-sdk-node/blob/master/CHANGELOG.md#370).

## Solution
The solution is quite simple. Just remove axios as fetch implementation
(override) and use the default provide by octokit.


## Testing
The problem was not detected in the unit tests. Reason is that the unit
test mocking the octokit SDK. In case of an ovrride we have doen. We
hsould have added a test using nock to mock the HTTP calls and not the
full SDK to catch problems like this in the unit test. By removing the
override, we can relay again on the test efforts done by octokit.

## References
- fix #3966
  • Loading branch information
npalm authored Aug 7, 2024
1 parent c15c99d commit 6ac19e6
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 480 deletions.
2 changes: 0 additions & 2 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@ updates:
update-types: ["version-update:semver-major"]
- dependency-name: "@octokit/*"
update-types: ["version-update:semver-major"]
- dependency-name: "@octokit/rest"
- dependency-name: "@octokit/auth-app"
- dependency-name: "eslint"
update-types: ["version-update:semver-major"]
commit-message:
Expand Down
8 changes: 4 additions & 4 deletions lambdas/functions/control-plane/jest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ const config: Config = {
...defaultConfig,
coverageThreshold: {
global: {
statements: 98.01,
branches: 97.28,
functions: 95.6,
lines: 97.94,
statements: 97.99,
branches: 97.26,
functions: 95.45,
lines: 97.92,
},
},
};
Expand Down
5 changes: 2 additions & 3 deletions lambdas/functions/control-plane/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,13 @@
"@aws-sdk/client-ec2": "^3.624.0",
"@aws-sdk/types": "^3.609.0",
"@middy/core": "^4.7.0",
"@octokit/auth-app": "6.0.3",
"@octokit/auth-app": "6.1.1",
"@octokit/core": "5.2.0",
"@octokit/plugin-throttling": "8.2.0",
"@octokit/rest": "20.0.2",
"@octokit/rest": "20.1.1",
"@octokit/types": "^13.5.0",
"@terraform-aws-github-runner/aws-powertools-util": "*",
"@terraform-aws-github-runner/aws-ssm-util": "*",
"axios": "^1.7.2",
"cron-parser": "^4.9.0",
"typescript": "^5.5.4"
},
Expand Down
31 changes: 0 additions & 31 deletions lambdas/functions/control-plane/src/axios/fetch-override.test.ts

This file was deleted.

19 changes: 0 additions & 19 deletions lambdas/functions/control-plane/src/axios/fetch-override.ts

This file was deleted.

4 changes: 2 additions & 2 deletions lambdas/functions/control-plane/src/gh-auth/gh-auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ ${decryptedValue}`,

// Assert
expect(mockedCreatAppAuth).toBeCalledTimes(1);
expect(mockedCreatAppAuth).toBeCalledWith({ ...authOptions, request: expect.anything() });
expect(mockedCreatAppAuth).toBeCalledWith({ ...authOptions });
});

test('Creates auth object for public GitHub', async () => {
Expand All @@ -121,7 +121,7 @@ ${decryptedValue}`,
expect(getParameter).toBeCalledWith(PARAMETER_GITHUB_APP_KEY_BASE64_NAME);

expect(mockedCreatAppAuth).toBeCalledTimes(1);
expect(mockedCreatAppAuth).toBeCalledWith({ ...authOptions, request: expect.anything() });
expect(mockedCreatAppAuth).toBeCalledWith({ ...authOptions });
expect(mockedAuth).toBeCalledWith({ type: authType });
expect(result.token).toBe(token);
});
Expand Down
13 changes: 3 additions & 10 deletions lambdas/functions/control-plane/src/gh-auth/gh-auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,14 @@ import { Octokit } from '@octokit/rest';
import { throttling } from '@octokit/plugin-throttling';
import { createChildLogger } from '@terraform-aws-github-runner/aws-powertools-util';
import { getParameter } from '@terraform-aws-github-runner/aws-ssm-util';

import { axiosFetch } from '../axios/fetch-override';
import { EndpointDefaults } from '@octokit/types';

const logger = createChildLogger('gh-auth');

export async function createOctoClient(token: string, ghesApiUrl = ''): Promise<Octokit> {
const CustomOctokit = Octokit.plugin(throttling);
const ocktokitOptions: OctokitOptions = {
auth: token,
request: { fetch: axiosFetch },
};
if (ghesApiUrl) {
ocktokitOptions.baseUrl = ghesApiUrl;
Expand All @@ -32,12 +30,12 @@ export async function createOctoClient(token: string, ghesApiUrl = ''): Promise<
return new CustomOctokit({
...ocktokitOptions,
throttle: {
onRateLimit: (options: { method: string; url: string }) => {
onRateLimit: (retryAfter: number, options: Required<EndpointDefaults>) => {
logger.warn(
`GitHub rate limit: Request quota exhausted for request ${options.method} ${options.url}. Requested `,
);
},
onSecondaryRateLimit: (options: { method: string; url: string }) => {
onSecondaryRateLimit: (retryAfter: number, options: Required<EndpointDefaults>) => {
logger.warn(`GitHub rate limit: SecondaryRateLimit detected for request ${options.method} ${options.url}`);
},
},
Expand Down Expand Up @@ -82,12 +80,7 @@ async function createAuth(installationId: number | undefined, ghesApiUrl: string
if (ghesApiUrl) {
authOptions.request = request.defaults({
baseUrl: ghesApiUrl,
request: {
fetch: axiosFetch,
},
});
} else {
authOptions.request = request.defaults({ request: { fetch: axiosFetch } });
}
return createAppAuth(authOptions);
}
2 changes: 1 addition & 1 deletion lambdas/functions/gh-agent-syncer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"@aws-sdk/lib-storage": "^3.623.0",
"@aws-sdk/types": "^3.609.0",
"@middy/core": "^4.7.0",
"@octokit/rest": "20.0.2",
"@octokit/rest": "20.1.1",
"@terraform-aws-github-runner/aws-powertools-util": "*",
"axios": "^1.7.2"
},
Expand Down
2 changes: 1 addition & 1 deletion lambdas/functions/webhook/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"dependencies": {
"@aws-sdk/client-sqs": "^3.623.0",
"@middy/core": "^4.7.0",
"@octokit/rest": "20.0.2",
"@octokit/rest": "20.1.1",
"@octokit/types": "^13.5.0",
"@octokit/webhooks": "^12.2.0",
"@terraform-aws-github-runner/aws-powertools-util": "*",
Expand Down
Loading

0 comments on commit 6ac19e6

Please sign in to comment.