Skip to content
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

feat: v21 #413

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

feat: v21 #413

wants to merge 24 commits into from

Conversation

wolfy1339
Copy link
Member

BREAKING CHANGE: package is now ESM

@wolfy1339 wolfy1339 added Type: Feature New feature or request Type: Breaking change Used to note any change that requires a major version bump labels Mar 4, 2024
Copy link
Contributor

github-actions bot commented Mar 4, 2024

👋 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
Copy link
Member Author

wolfy1339 commented Mar 18, 2024

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
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member

oscard0m commented Apr 10, 2024

@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 octokit/endpoint.js#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.

Footnotes

  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 oscard0m self-assigned this Apr 10, 2024
@oscard0m
Copy link
Member

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.

r0ps3c added a commit to r0ps3c/fork-sync that referenced this pull request Apr 11, 2024
- Remove throttling module, as it was unused
- Update to latest compatible where possible (e.g. excepting
octokit/rest.js#413)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants