chore: fix lint issues#56
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughLint settings were updated, shared HTTP and image-pull helpers now propagate close/display errors, and AdGuard Home, NPM, and Pi-hole clients now validate JSON decoding and retry/error paths. E2E tests and a config test were adjusted to use the updated cleanup and return-value handling. ChangesClient and test error-handling updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/clients/common/common.go (2)
142-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
doDeleteRequestuse named returns, or this defer never surfaces close errors.Lines 150-154 assign to the local
err, butdoDeleteRequeststill has unnamed return values. By Line 160 the function has already committednilas the returned error, so a failingresp.Body.Close()is silently dropped forDelete/DeleteWithContextcallers.Proposed fix
-func doDeleteRequest(req *http.Request, client *http.Client, headers map[string]string) (string, int, error) { +func doDeleteRequest(req *http.Request, client *http.Client, headers map[string]string) (bodyStr string, statusCode int, err error) { setHeaders(req, headers) resp, err := client.Do(req) if err != nil { return "", 0, err @@ body, err := io.ReadAll(resp.Body) if err != nil { return "", 0, err } return string(body), resp.StatusCode, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/common/common.go` around lines 142 - 160, Update doDeleteRequest to use named return values so the deferred resp.Body.Close() error can be propagated back to Delete and DeleteWithContext callers. Keep the existing defer logic, but ensure the function’s err return is the named one that the defer can update when io.ReadAll succeeds and Close fails.
113-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn the request-construction error from
Patch.Line 120 returns
nilwhenhttp.NewRequestfails. That turns a malformed or misconfigured URL into a fake success path, and downstream callers likepkg/clients/pihole/pihole.gocan report records added/deleted even though no PATCH request was sent.Proposed fix
if err != nil { - return "", 0, nil + return "", 0, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/common/common.go` around lines 113 - 120, Fix the Patch helper in common.Patch so request निर्माण failures are propagated instead of masked: when http.NewRequest returns an error, return that error from Patch rather than nil. Keep the change localized to the Patch function in common.go so callers such as pihole-related code can correctly detect and handle malformed or misconfigured paths.
🧹 Nitpick comments (1)
pkg/clients/pihole/pihole_test.go (1)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
/configdecode regression test for the new schema contract.
GetDnsRecords/getCNameRecordsnow depend on unmarshalling the fullconfigResponse, but these fixtures still only exercisehosts/cnameRecords. Please add at least one case that includes the newdns.domainobject shape, plus one malformed/configpayload case, so future Pi-hole schema drift fails in tests instead of at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clients/pihole/pihole_test.go` at line 126, Add regression coverage in the Pi-hole tests around config decoding: the current fixtures only cover the old hosts/cnameRecords shape, so extend the /config test setup used by GetDnsRecords and getCNameRecords to include a valid configResponse payload with the new dns.domain object shape, and add a malformed /config payload case that should fail during unmarshal. Use the existing test helpers and the configResponse/GetDnsRecords/getCNameRecords symbols to locate the relevant fixture and assertions, and make sure the new cases verify schema drift is caught in tests rather than at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e_tests/e2e_test.go`:
- Around line 293-297: The NPM teardown in t.Cleanup is swallowing
DeleteProxyHosts failures, so cleanup errors never surface and stale npm-data
can leak into later runs. Update the npmClient teardown path in e2e_test.go to
handle the error returned by DeleteProxyHosts instead of discarding it, and use
the existing cleanup flow around npmClient.GetProxyHosts and t.Cleanup to either
log the failure or return/aggregate it so the test can fail when cleanup does.
- Around line 411-418: The delete-step assertions in e2e_test.go only check for
errors, so a no-op delete can still pass; update the cleanup assertions around
DeleteDnsRecords, DeleteProxyHosts, and DeleteDnsRewrites to verify actual
deletion results. Use the returned deleted counts from
piholeClient.DeleteDnsRecords and adguardHomeClient.DeleteDnsRewrites, and the
boolean/removal result from npmClient.DeleteProxyHosts, so the test fails unless
entries were really removed.
In `@pkg/clients/pihole/pihole.go`:
- Around line 101-102: Stop mutating the shared package-level headers map with
the session token in the Pi-hole client request path; this creates cross-client
contamination when multiple Client instances call the API concurrently. Update
the request flow around Client and common.Get usage to build a fresh per-request
header map from an immutable base set, or store the auth header on Client and
merge it when sending requests. Apply the same fix anywhere the X-FTL-SID header
is set, including the other occurrence referenced in the review.
---
Outside diff comments:
In `@pkg/clients/common/common.go`:
- Around line 142-160: Update doDeleteRequest to use named return values so the
deferred resp.Body.Close() error can be propagated back to Delete and
DeleteWithContext callers. Keep the existing defer logic, but ensure the
function’s err return is the named one that the defer can update when io.ReadAll
succeeds and Close fails.
- Around line 113-120: Fix the Patch helper in common.Patch so request निर्माण
failures are propagated instead of masked: when http.NewRequest returns an
error, return that error from Patch rather than nil. Keep the change localized
to the Patch function in common.go so callers such as pihole-related code can
correctly detect and handle malformed or misconfigured paths.
---
Nitpick comments:
In `@pkg/clients/pihole/pihole_test.go`:
- Line 126: Add regression coverage in the Pi-hole tests around config decoding:
the current fixtures only cover the old hosts/cnameRecords shape, so extend the
/config test setup used by GetDnsRecords and getCNameRecords to include a valid
configResponse payload with the new dns.domain object shape, and add a malformed
/config payload case that should fail during unmarshal. Use the existing test
helpers and the configResponse/GetDnsRecords/getCNameRecords symbols to locate
the relevant fixture and assertions, and make sure the new cases verify schema
drift is caught in tests rather than at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 721f228a-1b90-4489-a6b0-a1980a44ce97
📒 Files selected for processing (12)
.golangci.ymle2e_tests/e2e_test.goe2e_tests/utils.gopkg/clients/adguardhome/adguardhome.gopkg/clients/adguardhome/adguardhome_test.gopkg/clients/common/common.gopkg/clients/npm/npm.gopkg/clients/npm/npm_test.gopkg/clients/pihole/pihole.gopkg/clients/pihole/pihole_test.gopkg/clients/pihole/types.gopkg/config/config_test.go
Comment on lines
293
to
297
| if npmClient != nil { | ||
| npmProxyHosts, err := npmClient.GetProxyHosts() | ||
| if err == nil { | ||
| npmClient.DeleteProxyHosts(slices.Collect(maps.Keys(npmProxyHosts))) | ||
| _, _ = npmClient.DeleteProxyHosts(slices.Collect(maps.Keys(npmProxyHosts))) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't suppress NPM teardown failures.
If DeleteProxyHosts fails here, the bind-mounted npm-data state survives container removal and can poison later E2E runs, but this path now hides that completely. Please surface the error at least as a log, or return/aggregate it so the t.Cleanup caller can fail the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_tests/e2e_test.go` around lines 293 - 297, The NPM teardown in t.Cleanup
is swallowing DeleteProxyHosts failures, so cleanup errors never surface and
stale npm-data can leak into later runs. Update the npmClient teardown path in
e2e_test.go to handle the error returned by DeleteProxyHosts instead of
discarding it, and use the existing cleanup flow around npmClient.GetProxyHosts
and t.Cleanup to either log the failure or return/aggregate it so the test can
fail when cleanup does.
Comment on lines
+411
to
+418
| _, err = piholeClient.DeleteDnsRecords(urls) | ||
| require.NoError(t, err, "Failed to delete Pi-Hole DNS records") | ||
|
|
||
| _, err = npmClient.DeleteProxyHosts(urls) | ||
| require.NoError(t, err, "Failed to delete NPM proxy hosts") | ||
|
|
||
| _, err = adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP()) | ||
| require.NoError(t, err, "Failed to delete AdGuard Home DNS rewrites") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the delete operations actually deleted entries.
These checks only prove the calls didn't error. DeleteDnsRecords / DeleteDnsRewrites return deleted counts, and DeleteProxyHosts returns whether anything was removed, so a no-op delete would still let the "delete" half of this test pass.
Suggested assertion shape
- _, err = piholeClient.DeleteDnsRecords(urls)
+ deletedDNS, err := piholeClient.DeleteDnsRecords(urls)
require.NoError(t, err, "Failed to delete Pi-Hole DNS records")
+ require.Equal(t, len(urls), deletedDNS, "Expected Pi-Hole to delete all requested records")
- _, err = npmClient.DeleteProxyHosts(urls)
+ deletedProxyHosts, err := npmClient.DeleteProxyHosts(urls)
require.NoError(t, err, "Failed to delete NPM proxy hosts")
+ require.True(t, deletedProxyHosts, "Expected NPM to delete a proxy host")
- _, err = adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP())
+ deletedRewrites, err := adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP())
require.NoError(t, err, "Failed to delete AdGuard Home DNS rewrites")
+ require.Equal(t, len(urls), deletedRewrites, "Expected AdGuard Home to delete all requested rewrites")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _, err = piholeClient.DeleteDnsRecords(urls) | |
| require.NoError(t, err, "Failed to delete Pi-Hole DNS records") | |
| _, err = npmClient.DeleteProxyHosts(urls) | |
| require.NoError(t, err, "Failed to delete NPM proxy hosts") | |
| _, err = adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP()) | |
| require.NoError(t, err, "Failed to delete AdGuard Home DNS rewrites") | |
| deletedDNS, err := piholeClient.DeleteDnsRecords(urls) | |
| require.NoError(t, err, "Failed to delete Pi-Hole DNS records") | |
| require.Equal(t, len(urls), deletedDNS, "Expected Pi-Hole to delete all requested records") | |
| deletedProxyHosts, err := npmClient.DeleteProxyHosts(urls) | |
| require.NoError(t, err, "Failed to delete NPM proxy hosts") | |
| require.True(t, deletedProxyHosts, "Expected NPM to delete a proxy host") | |
| deletedRewrites, err := adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP()) | |
| require.NoError(t, err, "Failed to delete AdGuard Home DNS rewrites") | |
| require.Equal(t, len(urls), deletedRewrites, "Expected AdGuard Home to delete all requested rewrites") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e_tests/e2e_test.go` around lines 411 - 418, The delete-step assertions in
e2e_test.go only check for errors, so a no-op delete can still pass; update the
cleanup assertions around DeleteDnsRecords, DeleteProxyHosts, and
DeleteDnsRewrites to verify actual deletion results. Use the returned deleted
counts from piholeClient.DeleteDnsRecords and
adguardHomeClient.DeleteDnsRewrites, and the boolean/removal result from
npmClient.DeleteProxyHosts, so the test fails unless entries were really
removed.
Comment on lines
101
to
102
| headers["X-FTL-SID"] = p.sid | ||
| configResponseString, _, err := common.Get(&p.Client, p.baseURL+"/config", headers) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stop storing the session ID in the package-level headers map.
These writes mutate shared global state, so two Client instances can overwrite each other’s X-FTL-SID and send the wrong session token on concurrent requests. Build per-request headers from an immutable base map, or keep the auth header on Client instead of the package variable.
Also applies to: 265-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/clients/pihole/pihole.go` around lines 101 - 102, Stop mutating the
shared package-level headers map with the session token in the Pi-hole client
request path; this creates cross-client contamination when multiple Client
instances call the API concurrently. Update the request flow around Client and
common.Get usage to build a fresh per-request header map from an immutable base
set, or store the auth header on Client and merge it when sending requests.
Apply the same fix anywhere the X-FTL-SID header is set, including the other
occurrence referenced in the review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores