-
Notifications
You must be signed in to change notification settings - Fork 51
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.
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
[BUG]: Update 6.1.0 -> 6.1.1 results in runtime error in AWS #634
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
This seems more like an issue with AWS then our code. It seems some platforms are using a non-standard fetch implementation. All of our tests pass, and there haven't been any issues filed with standard nodejs environments... All of the dependencies use Are you able to post a complete error stack trace? |
Can you also dedupe your lock file? You have multiple versions of |
I assume that the error message refers to the following feature that was added octokit/request.js#648 |
Can you see if using Instructions on patching out fetch can be found here: https://github.com/node-fetch/node-fetch?tab=readme-ov-file#providing-global-access Don't forget to use the --no-experimental-fetch flag |
Thanks for the pointers. We indeed override some time back the default fetch implementation. Due to the fact tracing in Node 18 was not working with AWS powertools. Resulting in that we could not trace calls to GitHub. This problem is now resolved in the AWS XRay sdk, and will be solved when lambda (AWS) moves to Node 22 (nov 24). Thanks for the quick feedback! |
## 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
What happened?
Upgrading from 6.1.0 to 6.1.1 results in a runtime (type) error
response.json(...).catch is not a function
The cause seems to be one of the transivive dependencies updated in the patch release as well.Versions
6.1.1
Relevant log output
Runtime we catch the following generic error. Which seems related to an underlying type issue:
See here the PR containing the version update and related packages bumped in the lock file.: https://github.com/philips-labs/terraform-aws-github-runner/pull/4042
Code of Conduct
The text was updated successfully, but these errors were encountered: