chore(staticcheck): resolve SA1019 in libvirt and generated protobuf#148
chore(staticcheck): resolve SA1019 in libvirt and generated protobuf#148jackthepunished wants to merge 2 commits intopoyrazK:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 28 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe libvirt client initialization in the adapter is refactored to use a custom Unix-socket dialer with timeout support instead of pre-dialing a connection. The initial connection is now explicitly closed, delegating socket management to the libvirt client through Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to clean up Staticcheck SA1019 deprecation warnings by updating libvirt client construction and suppressing SA1019 in generated protobuf output.
Changes:
- Switch libvirt adapter initialization from the deprecated
libvirt.New(conn)pattern tolibvirt.NewWithDialer(...)using a custom UNIX dialer. - Add a file-level Staticcheck ignore directive for SA1019 in a generated
*.pb.goprotobuf file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/repositories/libvirt/adapter.go | Replaces deprecated libvirt constructor usage with a dialer-based client initialization. |
| internal/autoscaler/protos/externalgrpc.pb.go | Adds a file-level Staticcheck SA1019 suppression comment to generated protobuf code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ = c.Close() | ||
| l := libvirt.NewWithDialer(&libvirtUnixDialer{uri: uri, timeout: 2 * time.Second}) |
There was a problem hiding this comment.
NewLibvirtAdapter now opens a UNIX socket connection (c) only to close it immediately before creating the libvirt client via NewWithDialer. This creates an extra connection attempt at startup and duplicates the timeout value in multiple places. Consider removing the eager DialTimeout/Close() (or moving the system/session fallback selection into the dialer) and centralizing the timeout into a constant to avoid divergence.
| //lint:file-ignore SA1019 Generated protobuf compatibility shims may reference deprecated APIs. | ||
|
|
There was a problem hiding this comment.
This file is marked as generated (DO NOT EDIT), but the PR adds a linter directive inside it. That change will be lost the next time protobufs are regenerated and can create recurring diff churn. Prefer suppressing SA1019 for generated *.pb.go under internal/autoscaler/protos/ via .golangci.yml (issues.exclude-rules / linters-settings.staticcheck checks) rather than editing generated output.
| //lint:file-ignore SA1019 Generated protobuf compatibility shims may reference deprecated APIs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/repositories/libvirt/adapter.go`:
- Line 125: The probe socket Close() call currently discards any error with “_ =
c.Close()”; change this to capture the returned error from c.Close() (e.g., err
:= c.Close()) and handle it instead of ignoring it — at minimum call the
adapter's logger (or standard log) to record the failure (e.g., logger.Errorf /
log.Printf with context like "probe socket close failed") and, if appropriate
for the surrounding function (e.g., in the adapter setup/cleanup code where c is
closed), propagate the error or wrap it before returning so init-time socket
issues are not lost.
- Around line 25-39: The call to c.Close() (the probe connection close) is
silently ignoring its error; change the code that currently does `_ = c.Close()`
to explicitly handle the returned error from Close() (e.g., capture err :=
c.Close() and either log it via your logger or return/wrap it so callers can act
on it) or move the close into a small helper (e.g., closeConn(conn) that
documents best-effort cleanup and logs a non-nil error) so the intent is
explicit; additionally consider replacing the custom libvirtUnixDialer type and
its Dial() implementation with the upstream dialers.NewLocal(...) and its
WithSocket/WithLocalTimeout options to remove the custom Dialer implementation
if you prefer the upstream API.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eb4858a-0dab-42a7-b684-8794b1b1d76e
⛔ Files ignored due to path filters (1)
internal/autoscaler/protos/externalgrpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
internal/repositories/libvirt/adapter.go
| libvirtsocket "github.com/digitalocean/go-libvirt/socket" | ||
| "github.com/google/uuid" | ||
| "github.com/poyrazk/thecloud/internal/core/ports" | ||
| ) | ||
|
|
||
| type libvirtUnixDialer struct { | ||
| uri string | ||
| timeout time.Duration | ||
| } | ||
|
|
||
| var _ libvirtsocket.Dialer = (*libvirtUnixDialer)(nil) | ||
|
|
||
| func (d *libvirtUnixDialer) Dial() (net.Conn, error) { | ||
| return net.DialTimeout("unix", d.uri, d.timeout) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Pinned go-libvirt dependency entries:"
rg -n --no-heading 'github.com/digitalocean/go-libvirt' go.mod go.sum
echo
echo "New libvirt APIs referenced by this patch:"
sed -n '20,140p' internal/repositories/libvirt/adapter.go | rg -n 'NewWithDialer|go-libvirt/socket'Repository: poyrazK/thecloud
Length of output: 636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the exact pinned version
echo "=== Pinned go-libvirt version ==="
rg 'github.com/digitalocean/go-libvirt' go.mod | head -1
# Get the full context around lines 120-130 to see the silent Close() issue
echo ""
echo "=== Code around line 125-126 (silent failure context) ==="
sed -n '115,135p' internal/repositories/libvirt/adapter.go
# Search for all usages of libvirtUnixDialer to understand its scope
echo ""
echo "=== All references to libvirtUnixDialer ==="
rg -n 'libvirtUnixDialer' internal/repositories/libvirt/
# Check imports in the file
echo ""
echo "=== Current imports in adapter.go ==="
sed -n '1,35p' internal/repositories/libvirt/adapter.goRepository: poyrazK/thecloud
Length of output: 2113
🌐 Web query:
go-libvirt socket.Dialer dialers.NewLocal WithSocket WithLocalTimeout API documentation
💡 Result:
The go-libvirt library (github.com/digitalocean/go-libvirt) provides a socket dialing API in the socket/dialers subpackage for connecting to libvirt daemons via UNIX sockets, TCP, TLS, SSH, etc. Key components: - socket.Dialer is the interface: type Dialer interface { Dial (net.Conn, error) } - dialers.NewLocal(opts ...LocalOption) creates a local (UNIX socket) dialer. It is the default for connecting to a locally running libvirt socket. - dialers.WithSocket(socket string) LocalOption: Specifies a custom UNIX socket path (e.g., dialers.NewLocal(dialers.WithSocket("/var/run/libvirt/virtqemud-sock")) for modular daemons). - dialers.WithLocalTimeout(timeout time.Duration) LocalOption: Sets the dial timeout for the local connection. Usage example from sources: dialer := dialers.NewLocal(dialers.WithSocket("/path/to/socket"), dialers.WithLocalTimeout(5*time.Second)) lv := libvirt.NewWithDialer(dialer) lv.Connect This is used internally by libvirt.ConnectToURI for "unix" transports, parsing ?socket= from the URI. Official documentation: pkg.go.dev/github.com/digitalocean/go-libvirt/socket/dialers (updated Feb 2026). Source code examples in github.com/digitalocean/go-libvirt/tree/main/socket/dialers and connect_uri.go.
Citations:
- 1: https://pkg.go.dev/github.com/digitalocean/go-libvirt/socket/dialers
- 2: https://github.com/digitalocean/go-libvirt/blob/main/socket/socket.go
- 3: https://github.com/digitalocean/go-libvirt/blob/main/connect_uri.go
- 4: Support for modular libvirt daemons digitalocean/go-libvirt#171
Handle the error from Close() at line 125; do not silently ignore it.
The probe connection must have its error handled explicitly. Currently _ = c.Close() violates the guideline prohibiting silent failures via blank identifier assignment.
If the error is intentionally ignored (e.g., best-effort cleanup before reconnection), capture it explicitly or wrap the close in a helper that documents the intent.
Optional: Replace the custom libvirtUnixDialer with the upstream local dialer.
The pinned go-libvirt version (v0.0.0-20260106181248-56be673cd6a5) includes dialers.NewLocal(...) and its options (WithSocket, WithLocalTimeout), which provide the same functionality more idiomatically:
Suggested refactoring
- libvirtsocket "github.com/digitalocean/go-libvirt/socket"
+ "github.com/digitalocean/go-libvirt/socket/dialers"
@@
-type libvirtUnixDialer struct {
- uri string
- timeout time.Duration
-}
-
-var _ libvirtsocket.Dialer = (*libvirtUnixDialer)(nil)
-
-func (d *libvirtUnixDialer) Dial() (net.Conn, error) {
- return net.DialTimeout("unix", d.uri, d.timeout)
-}
@@
- l := libvirt.NewWithDialer(&libvirtUnixDialer{uri: uri, timeout: 2 * time.Second})
+ l := libvirt.NewWithDialer(dialers.NewLocal(
+ dialers.WithSocket(uri),
+ dialers.WithLocalTimeout(2*time.Second),
+ ))This removes the custom type and aligns with the upstream library's design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repositories/libvirt/adapter.go` around lines 25 - 39, The call to
c.Close() (the probe connection close) is silently ignoring its error; change
the code that currently does `_ = c.Close()` to explicitly handle the returned
error from Close() (e.g., capture err := c.Close() and either log it via your
logger or return/wrap it so callers can act on it) or move the close into a
small helper (e.g., closeConn(conn) that documents best-effort cleanup and logs
a non-nil error) so the intent is explicit; additionally consider replacing the
custom libvirtUnixDialer type and its Dial() implementation with the upstream
dialers.NewLocal(...) and its WithSocket/WithLocalTimeout options to remove the
custom Dialer implementation if you prefer the upstream API.
|
|
||
| //nolint:staticcheck | ||
| l := libvirt.New(c) | ||
| _ = c.Close() |
There was a problem hiding this comment.
Don't silently drop the probe socket Close() failure.
Line 125 discards the only signal that the preflight socket cleanup failed. At least log it so init-time socket issues do not disappear during adapter setup.
🛠️ Suggested fix
- _ = c.Close()
+ if err := c.Close(); err != nil {
+ logger.Warn("failed to close preflight libvirt socket", "uri", uri, "error", err)
+ }As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
📝 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.
| _ = c.Close() | |
| if err := c.Close(); err != nil { | |
| logger.Warn("failed to close preflight libvirt socket", "uri", uri, "error", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repositories/libvirt/adapter.go` at line 125, The probe socket
Close() call currently discards any error with “_ = c.Close()”; change this to
capture the returned error from c.Close() (e.g., err := c.Close()) and handle it
instead of ignoring it — at minimum call the adapter's logger (or standard log)
to record the failure (e.g., logger.Errorf / log.Printf with context like "probe
socket close failed") and, if appropriate for the surrounding function (e.g., in
the adapter setup/cleanup code where c is closed), propagate the error or wrap
it before returning so init-time socket issues are not lost.
Summary
Validation
Scoped staticcheck cleanup against main.