octokit / rest.js

GitHub REST API client for JavaScript
https://octokit.github.io/rest.js
MIT License
548 stars 63 forks source link

feat: v21 #413

Closed wolfy1339 closed 3 months ago

wolfy1339 commented 7 months ago

BREAKING CHANGE: package is now ESM

github-actions[bot] commented 7 months ago

👋 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 labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

wolfy1339 commented 6 months ago

It seems that the test is failing because of a trailing slash (/) missing, after modifying @octokit/fixtures-server to account for the missing trailing slash it now fails with

 HttpError: Unknown error: {"error":"Nock: No match for request","detail":{"expected":{"method":"GET","url":"https://fixtures-xrae14px8de.api.github.com/repos/octokit-fixture-org/hello-world/contents","headers":{"accept-encoding":"gzip, deflate","sec-fetch-mode":"cors","accept-language":"*","authorization":"token 0000000000000000000000000000000000000001","user-agent":"octokit-rest.js/0.0.0-development octokit-core.js/6.0.1 Node.js/20.10.0 (linux; x64)","accept":"application/vnd.github.v3+json","connection":"close","host":"fixtures-xrae14px8de.api.github.com"}}}}
wolfy1339 commented 5 months ago

@octokit/js Is anyone able to figure out how to get the test passing?

Is there a way to get @octokit/endpoint to ignore the trailing / for the GET /repos/{owner}/{repo}/contents/{path} endpoint, if path is empty

oscard0m commented 5 months ago

@octokit/js Is anyone able to figure out how to get the test passing?

Is there a way to get @octokit/endpoint to ignore the trailing / for the GET /repos/{owner}/{repo}/contents/{path} endpoint, if path is empty

I'm going to take a look into this. I'll keep you posted.

oscard0m commented 5 months ago

@octokit/js Is anyone able to figure out how to get the test passing? Is there a way to get @octokit/endpoint to ignore the trailing / for the GET /repos/{owner}/{repo}/contents/{path} endpoint, if path is empty

I'm going to take a look into this. I'll keep you posted.

TL;DR (in test/scenarios/get-content.test.ts)

There is a logic in place in @octokit/endpoint.js which is removing the ending / and creating a mismatch in tests.


The Problem (in test/scenarios/get-content.test.ts)

I'm focusing on the tests failing for test/scenarios/get-content.test.ts. I will add a new comment for the other test failing.

The error prompted is the following one (complete logs here):

HttpError: Unknown error: {"error":"GET /repos/octokit-fixture-org/hello-world/contents does not match next fixture: GET /repos/octokit-fixture-org/hello-world/contents/","detail":{"pending":["GET [https://fixtures-987ffyc5v8n.api.github.com:443/repos/octokit-fixture-org/hello-world/contents/](https://fixtures-987ffyc5v8n.api.github.com/repos/octokit-fixture-org/hello-world/contents/)","GET [https://fixtures-987ffyc5v8n.api.github.com:443/repos/octokit-fixture-org/hello-world/contents/README.md](https://fixtures-987ffyc5v8n.api.github.com/repos/octokit-fixture-org/hello-world/contents/README.md)"]}}

The root cause

The line swallowing the ending / for the URL requested is this one https://github.com/octokit/endpoint.js/blob/fdbc2643b75d7f291eba3b1cda6428946093d778/src/util/url-template.ts#L197

This change was introduced in https://github.com/octokit/endpoint.js/pull/455 to cover use cases like:

Request Response
/repos/OWNER/REPO/branches 200
/repos/OWNER/REPO/branches/ 404

But, /repos/OWNER/REPO/contents/PATH works with and without /, so we should probably support both [^1].

Request Response
/repos/octokit/endpoint.js/contents 200
/repos/octokit/endpoint.js/contents/ 200
/repos/octokit/endpoint.js/contents/test 200
/repos/octokit/endpoint.js/contents/test/ 200

This mismatch comes when we compare the requested URL (with no ending /) and the expected from @octokit/fixtures (with ending /): https://github.com/octokit/fixtures/blob/5aa66e46e3f3475c4c1eb521cd402dfd85091b52/scenarios/api.github.com/get-content/normalized-fixture.json#L5

We should probably improve the logic we have in place in @octokit/endpoints, but I would like to hear your thoughts on this @octokit/js @octokit/extensibility-sdks. I'm discarding the option of fixing it in @octokit/fixtures because those are auto-generated.

[^1]: Apparently in the past the trailing / was not working for /repos/{org}/{repo}/contents endpoint but seems that it was escalated and now it's working.

oscard0m commented 5 months ago

The Problem (in test/scenarios/get-archive.test.ts)

The error prompted is the following one (complete logs here):

HttpError: Unknown error: {"error":"Nock: No match for request","detail":{"expected":{"method":"GET","url":"https://fixtures-r9di1toxolb.api.github.com/repos/octokit-fixture-org/get-archive/tarball/main","headers":{"accept-encoding":"gzip, deflate","sec-fetch-mode":"cors","accept-language":"*","authorization":"token 0000000000000000000000000000000000000001","user-agent":"octokit-rest.js/0.0.0-development octokit-core.js/6.1.2 Node.js/20.12.1 (darwin; x64)","accept":"application/vnd.github.v3+json","connection":"close","host":"fixtures-r9di1toxolb.api.github.com"}}}}

The root cause

I've been debugging a bit but did not found the root cause yet. I will keep checking. It's the only fixture which uses 302 as status response. Maybe it has something to do with that. What I can say is the issue is unrelated to the one with get-content.

wolfy1339 commented 3 months ago

I have skipped the tests that are failing for now. This PR should be ready to go

github-actions[bot] commented 3 months ago

:tada: This PR is included in version 21.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: