refactor: remove trivial deps and dead code - #21991
Conversation
TimeToOffset used tkuchiki/go-timezone to look up UTC offsets by timezone abbreviation. This is both unnecessary and less accurate than the stdlib: time.Time.Zone() already returns the exact offset, while the library re-derives it from the abbreviation and returns 0 on ambiguous names (e.g. "CST" maps to multiple offsets). Removes one direct dependency (tkuchiki/go-timezone) and its transitive deps from go.mod. Partially generated with AI assistance (Claude Code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
go.uber.org/atomic is deprecated by Uber in favor of stdlib sync/atomic, which gained equivalent method-based types (Int32, Int64, etc.) in Go 1.19. API mapping: NewInt32(0) → new(atomic.Int32), .Inc() → .Add(1), .Dec() → .Add(-1). .Load()/.Store() are identical. Partially generated with AI assistance (Claude Code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The refresh token cookie used gorilla/schema (a reflection-based struct-to-url.Values encoder/decoder) for a 4-field string struct. Replaced with direct url.Values access — simpler, no reflection, no external dependency for this trivial case. All 5 existing round-trip tests pass unchanged. Partially generated with AI assistance (Claude Code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The log-convert subcommand was disabled in PR #21301 (central-entrypoint roxctl removal) but the package and command registration were left behind. Remove the dead code and its import. Generated with AI assistance.
Removes gorilla/schema and tkuchiki/go-timezone from direct deps. Demotes go.uber.org/atomic from direct to indirect (still needed transitively by zap). Generated with AI assistance.
Single use in restore progress display. The exponentially weighted moving average with age=30 is: value += (2/31) * (sample - value). Three lines of arithmetic vs a whole dependency. Generated with AI assistance.
These schema:"..." tags were left behind when gorilla/schema was replaced with direct url.Values encoding. No code reads them now. Generated with AI assistance.
Generated with AI assistance.
|
Skipping CI for Draft Pull Request. |
github-actions
Bot
added
area/roxctl
area/auth
area/central
ai-review
coderabbit-review
labels
📝 WalkthroughWalkthroughThe changes replace selected third-party utilities with standard-library or local implementations, modernize restore progress tracking, migrate cron validation, simplify cookie and timezone handling, update operator and provider parsing, and remove the hidden ChangesRuntime dependency replacements
Restore transfer progress
Validation and test parsing
Other command and data decoupling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Restorer as v2Restorer
participant ProgressBar as transfer.ProgressBar
participant Transfer as transfer
Restorer->>ProgressBar: CreateProgressBar
Restorer->>Transfer: performHTTPRequest
Transfer->>ProgressBar: ProxyReader and count bytes
Restorer->>ProgressBar: Read current progress
ProgressBar-->>Restorer: Render transfer status and ETA
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
🚀 Build Images ReadyImages are ready for commit 510db81. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-626-g510db81bf7 |
Replace vbauerster/mpb multi-progress-bar library with plain fmt.Fprintf status lines, matching the approach used by kubectl and other Go CLI tools. Removes the mpb dep and its statusline.go filler implementation. This also removes the indirect deps on VividCortex/ewma and acarl005/stripansi which were only needed by mpb. Generated with AI assistance.
Three modules fully eliminated from the dependency tree. Generated with AI assistance.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@roxctl/central/db/transfer/progress_bar.go`:
- Line 96: Update FormatSize so its unit lookup supports sizes beyond GiB,
extending the unit sequence to include at least terabytes and continuing through
petabytes before indexing with exp. Preserve the existing scaling and formatting
behavior while preventing out-of-bounds access for larger byte values.
In `@roxctl/central/db/transfer/transfer.go`:
- Around line 36-40: Update the read-progress handling in the transfer method
around r.reader.Read and use count > 0, rather than err == nil, as the condition
for updating r.lastActivity and incrementing r.bar. Preserve the returned error
while counting bytes delivered alongside non-nil errors such as io.EOF.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ad44b03-bdfc-4255-aa8d-0c02cebf15cb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modroxctl/central/db/restore/statusline.goroxctl/central/db/restore/v2_restorer.goroxctl/central/db/transfer/progress_bar.goroxctl/central/db/transfer/transfer.go
💤 Files with no reviewable changes (2)
- roxctl/central/db/restore/statusline.go
- go.mod
| div *= unit | ||
| exp++ | ||
| } | ||
| return fmt.Sprintf("%.1f %ciB", float64(b)/float64(div), "KMG"[exp]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching progress_bar.go:"
fd -a 'progress_bar\.go$' . || true
file="$(fd 'progress_bar\.go$' . | head -n1 || true)"
if [ -n "$file" ]; then
echo "--- outline ---"
ast-grep outline "$file" || true
echo "--- relevant lines ---"
nl -ba "$file" | sed -n '1,150p'
fi
echo "--- FormatSize usages in repo ---"
rg -n "FormatSize|progress_bar|transfer" -S . --glob '!vendor/**' --glob '!*.sum' | head -n 200Repository: stackrox/stackrox
Length of output: 1079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/roxctl/central/db/transfer/progress_bar.go"
echo "--- progress_bar.go lines 1-140 ---"
awk '{printf "%4d\t%s\n", NR, $0}' "$file" | sed -n '1,140p'
echo "--- FormatSize references ---"
grep -RIn "FormatSize" . --exclude-dir=.git || true
echo "--- unit divider boundary values (read-only simulation) ---"
python3 - <<'PY'
divs = [1024, 1048576, 1073741824, 1099511627776, 1125899906842624, 1152921504606846976]
exp = 0
for bytes in [1023, 1024, 1048575, 1048576, 1073741823, 1073741824, 1099511627775, 1099511627776]:
div = divs[exp]
k = "KMG"
print(bytes, "div=", div, "exp=", exp, "index=", bytes // div, "next_exp_when_exceed_div=", exp + (bytes // div >= 2))
if bytes // div >= len(k):
print(" would overflow KMG at", bytes)
if bytes >= divs[exp+1]:
exp += 1
PYRepository: stackrox/stackrox
Length of output: 4377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/roxctl/central/db/transfer/progress_bar.go"
echo "--- exact diff/stat ---"
git diff -- roxctl/central/db/transfer/progress_bar.go || true
echo "--- local static bounds check for "KMG" slice ---"
python3 - <<'PY'
path = "/home/jailuser/git/roxctl/central/db/transfer/progress_bar.go"
text = open(path).read()
start = text.index("\nfunc FormatSize(b int64) string {\n")
end = text.index("\n}\ntype countingReader", start)
fn = text[start:end]
print(fn)
print("\nContains 'KMGTPE' unit suffix:", "KMGTPE" in fn)
print("Contains 'KMG' literal:", "'KMG'" in fn or '"KMG"' in fn)
# Static loop invariant based on the shown source:
# "KMG" has indices 0..2, so exp >= 3 overflows. The loop increments
# exp starting at 0 while n = b / 1024 stays >= 1024. Hence any
# b >= 1024**4 causes exp == 3 at return.
units = [1024, 1024**2, 1024**3, 1024**4, 1024**5, 1024**6]
suffixes = "KMG"
for b in units:
if b < 1024:
continue
div, exp = 1024, 0
n = b // 1024
while n >= 1024:
div *= 1024
exp += 1
print(b, "expected unit", suffixes[min(exp, len(suffixes)-1)], "final exp", exp, "overflow=", exp >= len(suffixes))
PYRepository: stackrox/stackrox
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- source references containing KMG/KMGTPE/FormatSize ---"
grep -RInE "FormatSize|KMG|KMGTPE" roxctl/central/db --exclude-dir=.git || true
echo "--- bounds of byte literals in format function with line numbers ---"
python3 - <<'PY'
from pathlib import Path
p = Path("roxctl/central/db/transfer/progress_bar.go")
lines = p.read_text().splitlines()
for i in range(85, 97):
print(f"{i}: {lines[i-1]}")
slice_literal = '"KMG"'
print(f"KMG literal line =", next((i+1 for i,l in enumerate(lines) if slice_literal in l), None))
print("contains KMGTPE literal:", "KMGTPE" in p.read_text())
PYRepository: stackrox/stackrox
Length of output: 1381
Handle sizes beyond GiB.
FormatSize uses exp to index "KMG" after div grows past 1 GiB; at 1 TiB exp becomes 3, causing a bounds panic from the progress-render goroutine. Support larger units such as KMGTPE before indexing.
🤖 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 `@roxctl/central/db/transfer/progress_bar.go` at line 96, Update FormatSize so
its unit lookup supports sizes beyond GiB, extending the unit sequence to
include at least terabytes and continuing through petabytes before indexing with
exp. Preserve the existing scaling and formatting behavior while preventing
out-of-bounds access for larger byte values.
| count, err := r.reader.Read(p) | ||
| if err == nil { | ||
| r.lastActivity.StoreAtomic(timestamp.Now()) | ||
| r.progressBar.IncrBy(len(p)) | ||
| r.bar.IncrBy(count) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching transfer.go:"
fd -a 'transfer.go$' . || true
echo
echo "Relevant file outline and contents:"
if [ -f roxctl/central/db/transfer/transfer.go ]; then
wc -l roxctl/central/db/transfer/transfer.go
ast-grep outline roxctl/central/db/transfer/transfer.go --view expanded || true
cat -n roxctl/central/db/transfer/transfer.go
else
echo "roxctl/central/db/transfer/transfer.go not found"
fi
echo
echo "Search for progressWatchReader usages and Close semantics:"
rg -n "progressWatchReader|IncrBy|lastActivity|Read\\(p\\)|Read\\(" roxctl/central/db/transfer roxctl || trueRepository: stackrox/stackrox
Length of output: 10586
🌐 Web query:
Go io.Reader documentation returned bytes and error may be non-nil
💡 Result:
In Go, the io.Reader interface's Read method is explicitly designed to allow returning both data (n > 0) and an error (err!= nil) in the same call [1][2]. This behavior occurs when a reader encounters an error or end-of-file condition after successfully reading some bytes [1][3]. For example, a reader might return the final bytes of a stream along with an io.EOF error simultaneously [1][4]. Because of this, callers must always process the returned bytes (n > 0) before considering the error (err) [1][5]. The standard, correct pattern for handling io.Reader is: n, err:= r.Read(p) if n > 0 { // Process the n bytes of data first process(p[:n]) } if err!= nil { // Handle the error (e.g., check for io.EOF) handle(err) } If you follow the common but incorrect pattern of checking the error first (e.g., if err!= nil { return err }), you risk discarding valid data returned by the final read operation [6][7]. Following the established pattern ensures that you correctly handle both partial reads and the various allowed behaviors of EOF [1][4].
Citations:
- 1: https://pkg.go.dev/io
- 2: https://go.dev/src/io/io.go?m=text
- 3: https://pkg.go.dev/io@go1.26.3
- 4: https://go.dev/src/io/io.go
- 5: https://tip.golang.org/pkg/io/?m=old
- 6: https://groups.google.com/g/golang-nuts/c/qRidokUs8Hw
- 7: https://groups.google.com/g/golang-nuts/c/5fntsH8fSVA/m/eSq5M4zhvL0J
Count bytes returned even when an error accompanies them.
io.Reader can legally return n > 0 with a non-nil error, including io.EOF. The current err == nil gate skips those transferred bytes and leaves the watchdog activity stale, so transfers can report less progress than actually read. Update the progress/activity timestamp when count > 0, independently of err.
Proposed fix
count, err := r.reader.Read(p)
- if err == nil {
+ if count > 0 {
r.lastActivity.StoreAtomic(timestamp.Now())
r.bar.IncrBy(count)
}📝 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.
| count, err := r.reader.Read(p) | |
| if err == nil { | |
| r.lastActivity.StoreAtomic(timestamp.Now()) | |
| r.progressBar.IncrBy(len(p)) | |
| r.bar.IncrBy(count) | |
| } | |
| count, err := r.reader.Read(p) | |
| if count > 0 { | |
| r.lastActivity.StoreAtomic(timestamp.Now()) | |
| r.bar.IncrBy(count) | |
| } |
🤖 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 `@roxctl/central/db/transfer/transfer.go` around lines 36 - 40, Update the
read-progress handling in the transfer method around r.reader.Read and use count
> 0, rather than err == nil, as the condition for updating r.lastActivity and
incrementing r.bar. Preserve the returned error while counting bytes delivered
alongside non-nil errors such as io.EOF.
gronx (513 stars, 1474 lines) was only used for IsValid() — cron expression syntax checking. robfig/cron.v2 (14k stars, 806 lines) is already in go.mod for Central's backup scheduler and can validate by parsing with a "0 " prefix to convert 5-field to 6-field format. One fewer module in the supply chain; robfig/cron is strictly more popular and actually smaller. Generated with AI assistance.
Generated with AI assistance.
|
/test pull-ci-stackrox-stackrox-master-gke-qa-e2e-tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@sensor/kubernetes/complianceoperator/utils.go`:
- Around line 195-198: Update both cron validation paths in
sensor/kubernetes/complianceoperator/utils.go at lines 195-198 and 214-217,
which call cronlib.Parse after prepending “0 ”, to support the documented
six-field cron expressions with seconds. Use cronlib’s explicit optional-seconds
or known-format parser consistently in both locations, or restrict and document
callers to five-field input if six-field schedules are not supported.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d864f550-c504-4561-983c-571bb35a3017
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
central/complianceoperator/v2/compliancemanager/manager_impl.gogo.modsensor/kubernetes/complianceoperator/utils.go
💤 Files with no reviewable changes (1)
- go.mod
| if req.GetCron() != "" { | ||
| cron := gronx.New() | ||
| if !cron.IsValid(req.GetCron()) { | ||
| if _, err := cronlib.Parse("0 " + req.GetCron()); err != nil { | ||
| errList.AddStrings("schedule is not valid") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether callers, tests, or documentation use six-field, descriptor, or extended cron syntax.
rg -n -C2 \
-e 'GetCron\(\)' \
-e '@(daily|hourly|weekly|monthly|every)' \
-e '[LW#]' \
--glob '*.go' --glob '*.md' --glob '*.proto' .Repository: stackrox/stackrox
Length of output: 50374
Use the correct label for the major cron validation issue.
cronlib.Parse defaults to standard five-field cron, but supported six-field cron specs including seconds should remain valid. Since these validation paths unconditionally add 0 before GetCron(), six-field expressions become seven-field and fail during apply/update validation. If six-field cron is supported, parse with explicit optional-seconds support or a known format; otherwise document/update callers to only send five-field cron.
📍 Affects 1 file
sensor/kubernetes/complianceoperator/utils.go#L195-L198(this comment)sensor/kubernetes/complianceoperator/utils.go#L214-L217
🤖 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 `@sensor/kubernetes/complianceoperator/utils.go` around lines 195 - 198, Update
both cron validation paths in sensor/kubernetes/complianceoperator/utils.go at
lines 195-198 and 214-217, which call cronlib.Parse after prepending “0 ”, to
support the documented six-field cron expressions with seconds. Use cronlib’s
explicit optional-seconds or known-format parser consistently in both locations,
or restrict and document callers to five-field input if six-field schedules are
not supported.
Sources: Path instructions, MCP tools
The flatten library was imported for a single function used in 3 test files: recursively flattening a nested map to dot-separated keys. Replaced with a 15-line FlattenMap helper in the existing operator test utilities package. Generated with AI assistance.
Generated with AI assistance.
|
/test gke-nongroovy-e2e-tests |
The go-junit library was used in a single test file to round-trip verify JUnit XML output. Replaced with 20 lines of encoding/xml struct definitions and xml.Unmarshal. Generated with AI assistance.
Generated with AI assistance.
The stackrox/external-network-pusher module (466 transitive deps!) was imported for 4 string constants and 4 simple JSON struct definitions. Inline them directly — zero behavioral change, massive dep tree reduction. Generated with AI assistance.
This module brought 466 transitive dependencies for 4 string constants and 4 JSON struct definitions. Generated with AI assistance.
The entire cloud.google.com/go/artifactregistry module (275 transitive deps) was imported in 2 files solely for DefaultAuthScopes(), which returns two OAuth scope URL strings. Inline them. Generated with AI assistance.
275 transitive dependencies eliminated for two OAuth scope strings. Generated with AI assistance.
|
/test gke-nongroovy-e2e-tests |
1 similar comment
|
/test gke-nongroovy-e2e-tests |
|
/retest |
|
@davdhacs: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
Description
Remove or replace several direct Go dependencies with stdlib equivalents or already-imported alternatives, reducing supply-chain attack surface. Each removal is a separate atomic commit.
Deps fully removed from go.mod (8 modules eliminated):
tkuchiki/go-timezone— replaced with stdlibtime.Time.Zone()(the offset is authoritative; the library re-derives it from ambiguous abbreviations)gorilla/schema— replaced with directurl.Valuesencoding/decoding for a 4-field string structVividCortex/ewma— replaced with 3 lines of inline EWMA math (single use in a progress bar speed display)vbauerster/mpb/v4— replaced with simple line-based progress output (matching kubectl's approach)acarl005/stripansi— removed (was only needed by mpb)adhocore/gronx— consolidated with existingrobfig/cron.v2(14k stars vs 513, actually smaller source)jeremywohl/flatten— replaced with 15-line recursive map flattener in test helperjoshdk/go-junit— replaced with stdlibencoding/xmlstructs in testMajor dep tree reductions:
stackrox/external-network-pusher— 466 transitive deps eliminated by inlining 4 string constants and 4 JSON struct definitionscloud.google.com/go/artifactregistry— 275 transitive deps eliminated by inlining 2 OAuth scope URL stringsDeps demoted from direct to indirect:
go.uber.org/atomic— replaced with stdlibsync/atomic(Uber deprecated it; stays indirect via zap until they drop it)Dead code removed:
roxctl/logconvertsubcommand (disabled in PR chore(central): remove dead roxctl log-convert from startup #21301 but package left behind)Net result: 10 modules fully eliminated from go.mod, 1 demoted to indirect, 1 dead code package deleted.
User-facing documentation
No user-facing behavior changes. The
roxctl central db restoreprogress display changes from a fancy multi-bar to simple line-based output (same information, less visual flair).Testing and quality
Automated testing
Existing tests for refresh_token_cookie (5 tests), reprocessor, logging, operator translation (deployment defaults + securedcluster), and JUnit printer continue to pass. The logconvert subcommand had no tests. Cron validation covered by existing integration tests.
How I validated my change
go build ./...passes with all changes appliedgo test ./pkg/auth/authproviders/...— 5 tests passgo test ./central/reprocessor/...— tests passgo test ./pkg/logging/...— tests pass (pre-existing flake in TestRateLimitedLogger unrelated)go test ./operator/internal/central/values/translation/...— tests passgo test ./operator/internal/securedcluster/values/translation/...— tests passgo test ./pkg/printers/... -run TestJunit— 2 tests passgo test ./pkg/networkgraph/defaultexternalsrcs/...— tests passgo mod tidyconfirms no orphaned depsrobfig/cron.v2.Parse("0 " + expr)correctly validates standard 5-field cron expressions