fix: allow set rejectUnauthorized = true on urllib.request options #561
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #561 +/- ##
==========================================
+ Coverage 93.27% 94.79% +1.51%
==========================================
Files 11 11
Lines 1235 1267 +32
Branches 286 305 +19
==========================================
+ Hits 1152 1201 +49
+ Misses 79 62 -17
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/HttpClient.test.ts (1)
463-469: Verify the usage of thehostnamevariableThe variable
hostnameis initialized and assigned within thecheckAddresscallback. Ensure that this variable is used later in the test to validate the hostname check functionality.If
hostnameis intended for assertion purposes, consider adding assertions to utilize this variable effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
package.json(1 hunks)src/HttpClient.ts(1 hunks)src/index.ts(2 hunks)test/HttpClient.test.ts(4 hunks)test/fixtures/server.ts(1 hunks)test/options.rejectUnauthorized-false.test.ts(1 hunks)test/options.timeout.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/HttpClient.ts
- test/fixtures/server.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/index.ts
[warning] 40-46: src/index.ts#L40-L46
Added lines #L40 - L46 were not covered by tests
🔇 Additional comments (6)
src/index.ts (1)
39-46: Lines 39-46 are not covered by tests
According to static analysis, the branch handling allowH2 when rejectUnauthorized is not false lacks test coverage. This may lead to untested behaviors in production.
Consider adding unit tests to cover these lines to ensure that the allowH2 logic functions correctly when rejectUnauthorized is true or undefined. This will increase confidence in the functionality and prevent potential issues.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 40-46: src/index.ts#L40-L46
Added lines #L40 - L46 were not covered by tests
test/HttpClient.test.ts (2)
121-125: Updated to use selfsigned library for certificate generation
The change from pem to selfsigned for generating self-signed certificates seems appropriate and aligns with the updated dependencies.
179-183: Consistent use of selfsigned in tests
The test setup now consistently uses selfsigned to generate SSL certificates. This ensures uniformity across tests.
test/options.rejectUnauthorized-false.test.ts (1)
1-70: Added tests for rejectUnauthorized: false
The new test cases effectively verify that setting rejectUnauthorized to false allows connections to servers with self-signed certificates, both in HTTP/1.1 and HTTP/2.
package.json (1)
75-75: Updated selfsigned dependency version and removed https-pem
The selfsigned library has been updated to ^2.4.1, and https-pem has been removed from devDependencies.
Please verify that the new version of selfsigned is compatible and does not introduce any breaking changes that could affect certificate generation in tests. Reviewing the changelog for selfsigned is recommended.
test/options.timeout.test.ts (1)
4-4: Replaced pem with selfsigned for SSL certificates in tests
The tests now use selfsigned to generate SSL certificates, ensuring consistency across the test suite.
Also applies to: 49-53
[skip ci] ## [4.6.6](v4.6.5...v4.6.6) (2024-12-07) ### Bug Fixes * allow set rejectUnauthorized = true on urllib.request options ([#561](#561)) ([88785e1](88785e1))
closes #531
Summary by CodeRabbit
Release Notes
New Features
rejectUnauthorizedset to false.Bug Fixes
Documentation
HttpClientclass.Chores