Skip to content

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.

wolfy1339 added a commit that referenced this pull request Feb 26, 2025
…, bump `devDependencies` (#487)

This aims to resolve #486 `npm vulnerabilities with the 20.x branch

Should resolve:

GHSA-2p57-rm9w-gvfp
GHSA-3xgq-45jj-v275
GHSA-67mh-4wv8-2f99
GHSA-78xj-cgh5-2h22
GHSA-952p-6rrq-rcjv
GHSA-9qxr-qj54-h672
GHSA-9wv6-86v2-598j
GHSA-c2qf-rxjj-qqgw
GHSA-c76h-2ccp-4975
GHSA-c7qv-q95q-8v27
GHSA-f5x3-32g6-xq36
GHSA-grv7-fg5c-xmjg
GHSA-h5c3-5r3r-rr8q
GHSA-m4v8-wqvr-p9f7
GHSA-m6fv-jmcg-4jfg
GHSA-pxg6-pf52-xh8x
GHSA-qwcr-r2fm-qrc7
GHSA-rhx6-c78j-4q9w
GHSA-rmvr-2pp2-xj38
GHSA-xx4v-prfh-6cgc


----

### Before the change?
<!-- Please describe the current behavior that you are modifying. -->

> 31 vulnerabilities (3 low, 18 moderate, 10 high)

![CleanShot 2025-02-21 at 12 06
39](https://github.com/user-attachments/assets/02abda17-8aee-46e3-b808-764672a18475)


### After the change?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

> 9 moderate severity vulnerabilities

![CleanShot 2025-02-21 at 12 12
49](https://github.com/user-attachments/assets/10d593d8-9de5-478e-8cde-b5fb81762706)

**Important note**: the remaining reported 'moderate' vulnerabilities
for `@octokit/request` and `@octokit/plugin-paginate-rest` for
GHSA-h5c3-5r3r-rr8q and
GHSA-rmvr-2pp2-xj38 are actually mitigated
already; npm audit isn't taking the minor versions properly into account
as:

- @octokit/plugin-paginate-rest is patched in `9.2.2` (applied)
- @octokit/request is patched in `8.4.1` (applied)

This is a reporting issue: npm/cli#8125


### Pull request checklist

**Important note**: this PR reduces updates (reduces :() test coverage
due to the same challenges discovered in
#413 (comment)

- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been reviewed and added / updated if needed (for bug
fixes / features)

### Does this introduce a breaking change?
<!-- If this introduces a breaking change make sure to note it here any
what the impact might be -->

Please see our docs on [breaking
changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md)
to help!

- [ ] Yes
- [x] No

----

---------

Co-authored-by: wolfy1339 <[email protected]>
wolfy1339 added a commit that referenced this pull request Feb 26, 2025
…, bump `devDependencies` (#487)

This aims to resolve #486 `npm vulnerabilities with the 20.x branch

Should resolve:

GHSA-2p57-rm9w-gvfp
GHSA-3xgq-45jj-v275
GHSA-67mh-4wv8-2f99
GHSA-78xj-cgh5-2h22
GHSA-952p-6rrq-rcjv
GHSA-9qxr-qj54-h672
GHSA-9wv6-86v2-598j
GHSA-c2qf-rxjj-qqgw
GHSA-c76h-2ccp-4975
GHSA-c7qv-q95q-8v27
GHSA-f5x3-32g6-xq36
GHSA-grv7-fg5c-xmjg
GHSA-h5c3-5r3r-rr8q
GHSA-m4v8-wqvr-p9f7
GHSA-m6fv-jmcg-4jfg
GHSA-pxg6-pf52-xh8x
GHSA-qwcr-r2fm-qrc7
GHSA-rhx6-c78j-4q9w
GHSA-rmvr-2pp2-xj38
GHSA-xx4v-prfh-6cgc

----

<!-- Please describe the current behavior that you are modifying. -->

> 31 vulnerabilities (3 low, 18 moderate, 10 high)

![CleanShot 2025-02-21 at 12 06
39](https://github.com/user-attachments/assets/02abda17-8aee-46e3-b808-764672a18475)

<!-- Please describe the behavior or changes that are being added by
this PR. -->

> 9 moderate severity vulnerabilities

![CleanShot 2025-02-21 at 12 12
49](https://github.com/user-attachments/assets/10d593d8-9de5-478e-8cde-b5fb81762706)

**Important note**: the remaining reported 'moderate' vulnerabilities
for `@octokit/request` and `@octokit/plugin-paginate-rest` for
GHSA-h5c3-5r3r-rr8q and
GHSA-rmvr-2pp2-xj38 are actually mitigated
already; npm audit isn't taking the minor versions properly into account
as:

- @octokit/plugin-paginate-rest is patched in `9.2.2` (applied)
- @octokit/request is patched in `8.4.1` (applied)

This is a reporting issue: npm/cli#8125

**Important note**: this PR reduces updates (reduces :() test coverage
due to the same challenges discovered in
#413 (comment)

- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been reviewed and added / updated if needed (for bug
fixes / features)

<!-- If this introduces a breaking change make sure to note it here any
what the impact might be -->

Please see our docs on [breaking
changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md)
to help!

- [ ] Yes
- [x] No

----

---------

Co-authored-by: wolfy1339 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants