fix: remove deprecated creation http client and usage of it#183
fix: remove deprecated creation http client and usage of it#183
Conversation
|
|
nnicora
changed the title
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR modernizes the HTTP client configuration by removing deprecated MTLS fields from the HTTPClient struct and introducing new authentication methods (APIToken, BasicAuth, OAuth2Auth, MTLS) with transport-level attributes. The deprecated NewClient constructor is removed, keeping NewHTTPClient as the primary factory. Default timeout increases from 10s to 30s. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/commonhttp/client_test.go (1)
45-45: Consider consolidating duplicate constructor tests.Lines 45 and 107 correctly switched to
commonhttp.NewHTTPClient, but this also makesTestNewClientandTestNewHTTPClientheavily overlapping. Keeping a single table-driven suite will reduce maintenance noise.Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/commonhttp/client_test.go` at line 45, Tests TestNewClient and TestNewHTTPClient are now duplicative because both call commonhttp.NewHTTPClient; consolidate them into a single table-driven test (e.g., TestNewHTTPClient) that enumerates all existing cases from both tests, reference commonhttp.NewHTTPClient in each case, remove the redundant TestNewClient, and ensure each table entry includes name, input args and expected error/behavior so assertion logic remains shared and clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/commoncfg/config.go`:
- Line 371: Add a regression test that loads the config via the existing loader
(which calls defaults.Set()) and verifies the HTTPClient.Timeout default is
applied when omitted: create a test case that unmarshals a config YAML/JSON that
includes an HTTPClient entry without the Timeout field, call the config loader
used in loader.go (ensure defaults.Set() runs), then assert the resulting
HTTPClient.Timeout equals 30*time.Second (matching the Timeout tag on the
Timeout field in pkg/commoncfg/config.go). Target the same test package and use
the same loader helper used by other tests so the defaults behavior is
exercised.
---
Nitpick comments:
In `@pkg/commonhttp/client_test.go`:
- Line 45: Tests TestNewClient and TestNewHTTPClient are now duplicative because
both call commonhttp.NewHTTPClient; consolidate them into a single table-driven
test (e.g., TestNewHTTPClient) that enumerates all existing cases from both
tests, reference commonhttp.NewHTTPClient in each case, remove the redundant
TestNewClient, and ensure each table entry includes name, input args and
expected error/behavior so assertion logic remains shared and clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92b8b351-be53-46d8-b11d-b5894b53c300
📒 Files selected for processing (3)
pkg/commoncfg/config.gopkg/commonhttp/client.gopkg/commonhttp/client_test.go
💤 Files with no reviewable changes (1)
- pkg/commonhttp/client.go
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements