Skip to content

chore: fix lint issues #56

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.

Sign up for GitHub

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

Merged
DeepSpace2 merged 3 commits into from
Jun 27, 2026
Merged

chore: fix lint issues #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37bb34d
chore: fix lint errors
DeepSpace2 Jun 21, 2026
913c0e9
Merge branch 'fix-lint-issues' of github.com:DeepSpace2/plugnpin into…
DeepSpace2 Jun 26, 2026
062faea
fix more lint errors
DeepSpace2 Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
version: "2"

run:
build-tags:
- e2e
- unit

output:
formats:
tab:
path: stdout
sort-order:
- file
- severity
- linter
49 changes: 27 additions & 22 deletions e2e_tests/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestMain(m *testing.M) {
if err == nil {
logger.Info("Performing global image cleanup")
for image := range globalImagesToRemove {
dockerCli.ImageRemove(ctx, image, imageApi.RemoveOptions{
_, _ = dockerCli.ImageRemove(ctx, image, imageApi.RemoveOptions{
Force: true,
})
}
Expand Down Expand Up @@ -121,11 +121,11 @@ func getEnvVars() (*config, error) {
if err != nil {
return nil, err
}
var config config
if err := env.Parse(&config); err != nil {
var cfg config
if err := env.Parse(&cfg); err != nil {
return nil, err
}
return &config, nil
return &cfg, nil
}

func pullRequiredImages(t *testing.T, ctx context.Context, dockerApi *dockerApi.Client, containers []Container) {
Expand All @@ -147,7 +147,7 @@ func pullRequiredImages(t *testing.T, ctx context.Context, dockerApi *dockerApi.
}

if err := g.Wait(); err != nil {
cleanup(t, dockerApi, containers, nil)
cleanup(dockerApi, containers, nil)
t.Fatal(err)
}
}
Expand Down Expand Up @@ -209,7 +209,7 @@ func startRequiredContainers(t *testing.T, ctx context.Context, dockerCli *docke
})
}
if err := g.Wait(); err != nil {
cleanup(t, dockerCli, containers, nil)
cleanup(dockerCli, containers, nil)
t.Fatal(err)
}
}
Expand All @@ -233,22 +233,22 @@ func setClients(t *testing.T, containers []Container) (*docker.Client, *pihole.C
}

piholeClient := pihole.NewClient(piholeURL, "password")
logger.Info("Waiting for Pi-hole to be ready...")
logger.Info("Waiting for Pi-Hole to be ready...")
piholeLoginTimeout := time.After(60 * time.Second)
piholeLoginTicker := time.NewTicker(3 * time.Second)
defer piholeLoginTicker.Stop()
PiholeLoginLoop:
for {
select {
case <-piholeLoginTimeout:
t.Fatalf("Timed out waiting for Pi-hole to be ready at %s", piholeURL)
t.Fatalf("Timed out waiting for Pi-Hole to be ready at %s", piholeURL)
case <-piholeLoginTicker.C:
err = piholeClient.Login()
if err == nil {
logger.Info("Successfully logged into Pi-hole")
logger.Info("Successfully logged into Pi-Hole")
break PiholeLoginLoop
}
logger.Error("Pi-hole not ready, retrying...", "error", err)
logger.Error("Pi-Hole not ready, retrying...", "error", err)
}
}

Expand Down Expand Up @@ -284,7 +284,7 @@ func setup(t *testing.T, ctx context.Context, dockerCli *dockerApi.Client, conta
return dockerClient, piholeClient, npmClient, adguardHomeClient
}

func cleanup(t *testing.T, dockerCli *dockerApi.Client, containers []Container, npmClient *npm.Client) {
func cleanup(dockerCli *dockerApi.Client, containers []Container, npmClient *npm.Client) {
logger.Info("In cleanup")

// Use background context for cleanup to ensure it completes even if test is canceled
Expand All @@ -293,7 +293,7 @@ func cleanup(t *testing.T, dockerCli *dockerApi.Client, containers []Container,
if npmClient != nil {
npmProxyHosts, err := npmClient.GetProxyHosts()
if err == nil {
npmClient.DeleteProxyHosts(slices.Collect(maps.Keys(npmProxyHosts)))
_, _ = npmClient.DeleteProxyHosts(slices.Collect(maps.Keys(npmProxyHosts)))
}
Comment on lines 293 to 297

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

}
var wg sync.WaitGroup
Expand Down Expand Up @@ -360,7 +360,7 @@ func TestE2E(t *testing.T) {
dockerClient, piholeClient, npmClient, adguardHomeClient := setup(t, ctx, dockerCli, containers)

t.Cleanup(func() {
cleanup(t, dockerCli, containers, npmClient)
cleanup(dockerCli, containers, npmClient)
})

time.Sleep(2 * time.Second)
Expand Down Expand Up @@ -408,9 +408,14 @@ func TestE2E(t *testing.T) {
}

// Deleting to assert delete functionality
piholeClient.DeleteDnsRecords(urls)
npmClient.DeleteProxyHosts(urls)
adguardHomeClient.DeleteDnsRewrites(urls, npmClient.GetIP())
_, 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")
Comment on lines +411 to +418

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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")
[review_comment_end]
📝 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.


piholeDnsRecords, err := piholeClient.GetDnsRecords()
if err != nil {
Expand Down Expand Up @@ -461,7 +466,7 @@ func TestE2E_CreateOnHealthy(t *testing.T) {
dockerClient, piholeClient, npmClient, adguardHomeClient := setup(t, ctx, dockerCli, infraContainers)

t.Cleanup(func() {
cleanup(t, dockerCli, infraContainers, npmClient)
cleanup(dockerCli, infraContainers, npmClient)
})

proc := processor.New(
Expand Down Expand Up @@ -502,7 +507,7 @@ func TestE2E_CreateOnHealthy(t *testing.T) {
startRequiredContainers(t, ctx, dockerCli, testContainers)

t.Cleanup(func() {
dockerCli.ContainerRemove(context.Background(), testContainers[0].id, containerApi.RemoveOptions{Force: true})
_ = dockerCli.ContainerRemove(context.Background(), testContainers[0].id, containerApi.RemoveOptions{Force: true})
})

// Assert entry DOES NOT exist while unhealthy
Expand All @@ -521,8 +526,8 @@ func TestE2E_CreateOnHealthy(t *testing.T) {
// Wait for Docker to mark it healthy
logger.Info("Waiting for Docker to mark container as healthy")
require.Eventually(t, func() bool {
inspect, err := dockerCli.ContainerInspect(ctx, testContainers[0].id)
if err != nil {
inspect, inspectErr := dockerCli.ContainerInspect(ctx, testContainers[0].id)
if inspectErr != nil {
return false
}
return inspect.State.Health != nil && inspect.State.Health.Status == "healthy"
Expand Down Expand Up @@ -577,7 +582,7 @@ func TestE2E_CreateOnHealthy_NoHealthcheck(t *testing.T) {
dockerClient, _, npmClient, adguardHomeClient := setup(t, ctx, dockerCli, infraContainers)

t.Cleanup(func() {
cleanup(t, dockerCli, infraContainers, npmClient)
cleanup(dockerCli, infraContainers, npmClient)
})

proc := processor.New(
Expand Down Expand Up @@ -614,7 +619,7 @@ func TestE2E_CreateOnHealthy_NoHealthcheck(t *testing.T) {
startRequiredContainers(t, ctx, dockerCli, testContainers)

t.Cleanup(func() {
dockerCli.ContainerRemove(context.Background(), testContainers[0].id, containerApi.RemoveOptions{Force: true})
_ = dockerCli.ContainerRemove(context.Background(), testContainers[0].id, containerApi.RemoveOptions{Force: true})
})

// Assert entry NEVER exists
Expand Down
5 changes: 2 additions & 3 deletions e2e_tests/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@ func pullImage(ctx context.Context, dockerCli *dockerCliClient.Client, img strin
return err
}

defer reader.Close()
defer reader.Close() //nolint:errcheck
termFd, isTerm := term.GetFdInfo(os.Stderr)
jsonmessage.DisplayJSONMessagesStream(reader, os.Stderr, termFd, isTerm, nil)
return nil
return jsonmessage.DisplayJSONMessagesStream(reader, os.Stderr, termFd, isTerm, nil)
}

func markContainerHealthy(ctx context.Context, dockerCli *dockerCliClient.Client, containerID string) error {
Expand Down
8 changes: 4 additions & 4 deletions pkg/clients/adguardhome/adguardhome.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,9 @@ import (
"net/http"

"github.com/deepspace2/plugnpin/pkg/clients/common"
"github.com/deepspace2/plugnpin/pkg/logging"
"github.com/deepspace2/plugnpin/pkg/metrics"
)

var log = logging.GetLogger("adguardhome")

type Client struct {
http.Client
baseURL string
Expand Down Expand Up @@ -42,7 +39,10 @@ func (ad *Client) GetDnsRewrites() (DnsRewrites, error) {
return nil, err
}
var resp []DnsRewrite
json.Unmarshal([]byte(dnsRewritesResponseString), &resp)
err = json.Unmarshal([]byte(dnsRewritesResponseString), &resp)
if err != nil {
return nil, err
}

dnsRewrites := DnsRewrites{}
for _, rawDnsRewrite := range resp {
Expand Down
12 changes: 6 additions & 6 deletions pkg/clients/adguardhome/adguardhome_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestAddDnsRewrite(t *testing.T) {

if r.URL.Path == "/control/rewrite/list" && r.Method == http.MethodGet {
w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `[{"domain": "one.com", "answer": "1.1.1.1"}]`)
_, _ = fmt.Fprint(w, `[{"domain": "one.com", "answer": "1.1.1.1"}]`)
return
}

Expand All @@ -46,7 +46,7 @@ func TestAddDnsRewrite(t *testing.T) {
assert.True(t, payload.Enabled)

w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `{}`)
_, _ = fmt.Fprint(w, `{}`)
return
}

Expand Down Expand Up @@ -81,13 +81,13 @@ func TestDeleteDnsRewrite(t *testing.T) {
assert.True(t, payload.Enabled)

w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `{}`)
_, _ = fmt.Fprint(w, `{}`)
return
}

if r.URL.Path == "/control/rewrite/list" && r.Method == http.MethodGet {
w.WriteHeader(http.StatusOK)
fmt.Fprint(w, `[]`)
_, _ = fmt.Fprint(w, `[]`)
return
}

Expand Down Expand Up @@ -116,7 +116,7 @@ func TestWrongCredentials(t *testing.T) {
expectedAuth := "Basic " + base64.StdEncoding.EncodeToString([]byte("testuser:testpass"))
if auth != expectedAuth {
w.WriteHeader(http.StatusUnauthorized)
fmt.Fprint(w, `{}`)
_, _ = fmt.Fprint(w, `{}`)
}
return
}
Expand All @@ -126,7 +126,7 @@ func TestWrongCredentials(t *testing.T) {
expectedAuth := "Basic " + base64.StdEncoding.EncodeToString([]byte("testuser:testpass"))
if auth != expectedAuth {
w.WriteHeader(http.StatusUnauthorized)
fmt.Fprint(w, `{}`)
_, _ = fmt.Fprint(w, `{}`)
}
return
}
Expand Down
30 changes: 23 additions & 7 deletions pkg/clients/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func setHeaders(req *http.Request, headers map[string]string) {
}
}

func Post(client *http.Client, path string, headers map[string]string, data *string) (string, int, error) {
func Post(client *http.Client, path string, headers map[string]string, data *string) (bodyStr string, statusCode int, err error) {
req, err := http.NewRequest(
http.MethodPost,
path,
Expand All @@ -68,7 +68,11 @@ func Post(client *http.Client, path string, headers map[string]string, data *str
return "", 0, err
}

defer resp.Body.Close()
defer func() {
if cerr := resp.Body.Close(); cerr != nil && err == nil {
err = cerr
}
}()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", 0, err
Expand All @@ -77,7 +81,7 @@ func Post(client *http.Client, path string, headers map[string]string, data *str
return string(body), resp.StatusCode, nil
}

func Get(client *http.Client, path string, headers map[string]string) (string, int, error) {
func Get(client *http.Client, path string, headers map[string]string) (bodyStr string, statusCode int, err error) {
req, err := http.NewRequest(
http.MethodGet,
path,
Expand All @@ -94,15 +98,19 @@ func Get(client *http.Client, path string, headers map[string]string) (string, i
return "", 0, err
}

defer resp.Body.Close()
defer func() {
if cerr := resp.Body.Close(); cerr != nil && err == nil {
err = cerr
}
}()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", 0, err
}
return string(body), resp.StatusCode, nil
}

func Patch(client *http.Client, path string, headers map[string]string, data string) (string, int, error) {
func Patch(client *http.Client, path string, headers map[string]string, data string) (bodyStr string, statusCode int, err error) {
req, err := http.NewRequest(
http.MethodPatch,
path,
Expand All @@ -119,7 +127,11 @@ func Patch(client *http.Client, path string, headers map[string]string, data str
return "", 0, err
}

defer resp.Body.Close()
defer func() {
if cerr := resp.Body.Close(); cerr != nil && err == nil {
err = cerr
}
}()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", 0, err
Expand All @@ -135,7 +147,11 @@ func doDeleteRequest(req *http.Request, client *http.Client, headers map[string]
return "", 0, err
}

defer resp.Body.Close()
defer func() {
if cerr := resp.Body.Close(); cerr != nil && err == nil {
err = cerr
}
}()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", 0, err
Expand Down
Loading
Loading