refactor: use globalping-go & cleanup #180
Conversation
WalkthroughThis pull request replaces the local globalping implementation with the external globalping-go library and introduces a new api package that exposes a Client interface and concrete implementation. It propagates context throughout command handlers and measurement flows, centralizes token handling using storage.Token with storage-backed persistence, and converts many client methods to context-aware signatures. The globalping package files (models, client, measurements, limits) and many legacy mocks were removed; mocks were reorganized into api/utils/view packages. Viewer APIs were restructured (OutputDefault, OutputJSON, OutputLatency, OutputLive) and command flows now use a handleMeasurement pathway. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
storage/config_test.go (1)
24-30: FixToken.ExpiryJSON tag to avoid breaking config compatibilityThe new
storage.Tokentype (seestorage/config.go) defines:Expiry time.Time `json:"expiry,omitzero"`
encoding/jsondoes not recognizeomitzeroas an option, so the JSON field name becomes"expiry,omitzero". That has two concrete side effects:
- Existing config files that stored
"expiry"(with the old token type) will no longer populateExpirywhen unmarshaled.- New configs will write an odd
"expiry,omitzero"field name, diverging from the expected"expiry"and making manual inspection/editing confusing.Given this test is asserting persistence of
Tokenthrough JSON, this is effectively a regression in the on-disk format.I’d recommend changing the tag in
storage/config.goto use the standardomitemptyoption instead:- Expiry time.Time `json:"expiry,omitzero"` + Expiry time.Time `json:"expiry,omitempty"`This restores the
"expiry"field name and keeps zero values out of the JSON.cmd/ping.go (1)
145-207: Pipeline failure:Test_Execute_Ping_Infinitepanics onGetMeasurementcall.The test fails because the infinite ping loop calls
GetMeasurement(ctx, id)but the test mock doesn't have expectations set for it. Thepingfunction iterates and callsGetMeasurementmultiple times, which isn't covered by the test setup.Ensure the ping infinite test sets up
GetMeasurementexpectations. Since the loop runs until cancelled, useAnyTimes()or a specific count with proper return values:gbMock.EXPECT().GetMeasurement(gomock.Any(), gomock.Any()).Return(expectedMeasurement, nil).AnyTimes()api/auth.go (1)
168-183: HTTP response body not closed - connection leak.The response body must be closed to allow HTTP connection reuse. This pattern repeats in
refreshToken,introspection, andRevokeTokenas well.resp, err := c.http.Do(req) if err != nil { return nil, &AuthorizeError{ ErrorType: ErrTypeExchangeFailed, Description: err.Error(), } } + defer resp.Body.Close() if resp.StatusCode != http.StatusOK {Apply similar fix at lines 305, 376, and 417.
♻️ Duplicate comments (2)
view/share_test.go (2)
41-41: Same viewer-construction duplication concern as aboveThis
NewViewerinvocation has the same shape and maintenance concern as the one noted earlier on Line 21; it could also be covered by a shared test helper.
66-66: Same viewer-construction duplication concern as aboveThis call site can likewise benefit from the shared test helper suggested for Line 21.
🧹 Nitpick comments (19)
view/share_test.go (1)
21-21: Reduce duplication by introducing a small test helper for viewer creationAll three subtests build a
Viewerin similar ways; this will become brittle ifNewViewergains new dependencies again. Consider introducing a small helper likenewTestViewer(ctx *Context, w, errw *bytes.Buffer) Viewerand using it in these tests to centralize construction and keep tests easier to adapt to future signature changes.utils/config.go (1)
8-9: Consider usingconstinstead ofvarfor immutability.
ShareURLis initialized to a fixed string and doesn't appear to be modified at runtime. Declaring it as a constant would prevent accidental mutation and better express the intent.Apply this diff:
-var ShareURL = "https://globalping.io?measurement=" - +const ShareURL = "https://globalping.io?measurement="mocks/api/mock_probe.go (1)
2-16: Keep mockgen configuration as single source of truthThis file is generated; if the source path, destination, or package name needs to change again, make the change in the mockgen invocation (and any helper scripts) rather than editing the generated file directly, so it doesn’t get reverted on regeneration.
view/default.go (1)
11-59: Defensive nil checks foropts.Options.Requestto avoid panics
OutputDefaultdereferencesopts.Options.Request.Methodwithout any guards (Lines 55 and 59). If a future caller ever passes a nilopts(oropts.Options/opts.Options.Request), the CLI will panic instead of degrading gracefully.Consider guarding these accesses and treating missing options as “non‑GET”:
- v.printer.ErrPrintln(result.Result.RawHeaders) - if opts.Options.Request.Method == "GET" { + v.printer.ErrPrintln(result.Result.RawHeaders) + if opts != nil && opts.Options != nil && opts.Options.Request.Method == "GET" { v.printer.ErrPrintln() v.printer.Println(strings.TrimSpace(result.Result.RawBody)) } - } else if opts.Options.Request.Method == "GET" { + } else if opts != nil && opts.Options != nil && opts.Options.Request.Method == "GET" { v.printer.Println(strings.TrimSpace(result.Result.RawBody)) } else { v.printer.Println(strings.TrimSpace(result.Result.RawOutput)) }This keeps current behavior when options are present while avoiding hard crashes if a nil slips through later.
cmd/utils_test.go (1)
30-56: Preferglobalping.MeasurementType*constants over raw string literalsThese helpers now take
cmd globalping.MeasurementType, but theswitch cmdblocks still match against untyped string literals like"ping","traceroute", etc. This couples the tests to the literal wire values and makes it easier for them to drift from the official enum values inglobalping-go(e.g.,globalping.MeasurementTypePing).(github.com)To make the tests more robust and self‑documenting, consider:
- Switching the case labels to the library’s typed constants (e.g.,
case globalping.MeasurementTypePing:etc.).- Optionally centralizing the mapping from
MeasurementType→ protocol/port in a small helper to avoid duplicating the same switch logic across helpers.Also applies to: 60-99
mocks/utils/mock_utils.go (1)
6-17: Generated mock: ensure generator command is the canonical sourceSince this mock is generated, make sure the
mockgeninvocation (and any generation script) is updated to match the new destination/package so future regenerations won’t revert or conflict with these header changes.README.md (1)
21-35: Keep the TOC in sync with the new “Globalping API package” sectionYou’ve added a
## Globalping API packagesection at the end, but it isn’t listed in the Table of Contents block. For consistency, either:
- Add a bullet for “Globalping API package” to the TOC, or
- Regenerate the entire TOC with the same tool that produced the comment markers so it picks up the new heading automatically.
Also applies to: 427-429
storage/storage.go (1)
61-75: Surface unexpectedLoadConfigerrors fromInitinstead of ignoring themIn
Init, onlyos.IsNotExist(err)is handled specially; any other error froms.LoadConfig()is effectively ignored, andInitproceeds tos.Migrate()and returnsnil:_, err = s.LoadConfig() if err != nil { if os.IsNotExist(err) { s.config = &Config{...} err = s.SaveConfig() if err != nil { return err } } } s.Migrate() return nilIf
LoadConfigfails due to corrupt JSON, permission problems, or other I/O issues, this will silently succeed from the caller’s perspective, potentially withs.configin an invalid state and migrations running against it.Consider tightening this logic so unexpected errors are surfaced, for example:
- _, err = s.LoadConfig() - if err != nil { - if os.IsNotExist(err) { - s.config = &Config{ - Profile: "default", - Profiles: make(map[string]*Profile), - LastMigration: len(s.migrations), - } - err = s.SaveConfig() - if err != nil { - return err - } - } - } + _, err = s.LoadConfig() + if err != nil { + if os.IsNotExist(err) { + s.config = &Config{ + Profile: "default", + Profiles: make(map[string]*Profile), + LastMigration: len(s.migrations), + } + if err := s.SaveConfig(); err != nil { + return err + } + } else { + // For non-NotExist errors, fail fast so callers can react. + return err + } + }This makes config initialization failures explicit instead of silently continuing with a potentially bad state.
view/viewer.go (1)
8-16: Inconsistent error return patterns across Output methods.
OutputLatencyandOutputInfinitereturnerror, whileOutputDefault,OutputJSON, andOutputLivedo not. If the non-error-returning methods can fail (e.g., write errors), consider unifying the signatures for consistent error handling by callers.storage/config.go (1)
27-31: Race condition acknowledged but not addressed.The TODO on line 27 correctly identifies the need for a locking mechanism. The current implementation caches config in
s.configwithout synchronization, which could cause data races ifLoadConfigis called concurrently. Consider usingsync.Oncefor initialization or async.RWMutexfor the cached config access.Would you like me to open an issue to track implementing the locking mechanism?
cmd/common.go (1)
48-51: Redundant condition.At line 48,
r.ctx.CIModeis always true when reached—ifToLatencyorToJSONwere true, we'd have already returned. This could simply be an unconditional block or removed entirely since the early returns cover the other cases.- if r.ctx.CIMode { - r.viewer.OutputDefault(id, res, opts) - return nil - } + r.viewer.OutputDefault(id, res, opts) + return nil }cmd/root.go (1)
91-91: Parameter name is misleading.The parameter
globalpingClientis typed asapi.Client, notglobalping.Client. This could cause confusion when reading the code sinceglobalpingrefers to the external library.func NewRoot( printer *view.Printer, ctx *view.Context, viewer view.Viewer, utils utils.Utils, - globalpingClient api.Client, + apiClient api.Client, globalpingProbe probe.Probe, localStorage *storage.LocalStorage, ) *Root {view/output.go (1)
36-38: Panic is harsh for terminal size edge case.If the terminal is extremely small (e.g., width ≤ 4 or height ≤ 4), this panics. While rare, this could occur in edge cases like embedded terminals or CI environments with unusual configurations.
Consider returning an error or gracefully handling this:
func trimOutput(output *strings.Builder, terminalW, terminalH int) *string { maxW := terminalW - 4 // 4 extra chars to be safe from overflow maxH := terminalH - 4 // 4 extra lines to be safe from overflow if maxW <= 0 || maxH <= 0 { - panic("terminal width / height too limited to display results") + // Terminal too small, return truncated message + txt := "..." + return &txt }api/measurements_test.go (3)
22-28: Variable shadowing: local variables shadow imported package aliases.
utilsMockandglobalpingMocklocal variables shadow the imported package aliases of the same name. This compiles but is confusing and error-prone.- utilsMock := utilsMock.NewMockUtils(ctrl) + mockUtils := utilsMock.NewMockUtils(ctrl) utilsMock.EXPECT().Now().Return(defaultCurrentTime).AnyTimes() + mockUtils.EXPECT().Now().Return(defaultCurrentTime).AnyTimes() - globalpingMock := globalpingMock.NewMockClient(ctrl) + mockGlobalping := globalpingMock.NewMockClient(ctrl)Alternatively, rename the imports (e.g.,
gpMockfor globalping,uMockfor utils) to avoid collision with local variable naming conventions.
175-182: UncheckedWritereturn value in test handler.Line 178 ignores the error from
w.Write(). While acceptable in tests, consider using a helper or_, _ = w.Write(...)to silence linters.- w.Write([]byte(`{"error": "invalid_grant", "error_description": "Invalid refresh token."}`)) + _, _ = w.Write([]byte(`{"error": "invalid_grant", "error_description": "Invalid refresh token."}`))
225-228: Same uncheckedWritepattern.- w.Write([]byte(`{"error": {"type": "unauthorized", "message": "Unauthorized."}}`)) + _, _ = w.Write([]byte(`{"error": {"type": "unauthorized", "message": "Unauthorized."}}`))mocks/globalping/mock_globalping.go (1)
6-10: Package namelibis inconsistent with directory path.The generated mock is in
mocks/globalping/but the package is namedlib. This could cause import confusion. Consider regenerating with a more descriptive package name likeglobalpingmocks:mockgen -destination mocks/globalping/mock_globalping.go -package globalpingmocks github.com/jsdelivr/globalping-go Clientapi/client.go (1)
98-98: Redundant mutex initialization.The zero value of
sync.RWMutexis ready to use; explicit initialization is unnecessary.- mu: sync.RWMutex{},api/auth.go (1)
226-232: Redundant token field copying.The token
tis already a*storage.Token. You can pass it directly tosaveTokeninstead of creating a new instance with identical fields:- c.saveToken(&storage.Token{ - AccessToken: t.AccessToken, - TokenType: t.TokenType, - RefreshToken: t.RefreshToken, - ExpiresIn: t.ExpiresIn, - Expiry: t.Expiry, - }) + c.saveToken(t)Same applies at lines 247-253 and 279-285.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
.gitignore(1 hunks)README.md(2 hunks)api/auth.go(15 hunks)api/auth_test.go(9 hunks)api/client.go(1 hunks)api/limits.go(1 hunks)api/measurements.go(1 hunks)api/measurements_test.go(1 hunks)api/utils_test.go(1 hunks)cmd/auth.go(6 hunks)cmd/auth_test.go(7 hunks)cmd/common.go(5 hunks)cmd/dns.go(4 hunks)cmd/dns_test.go(9 hunks)cmd/history_test.go(5 hunks)cmd/http.go(4 hunks)cmd/http_test.go(9 hunks)cmd/install_probe.go(1 hunks)cmd/install_probe_test.go(3 hunks)cmd/limits.go(3 hunks)cmd/limits_test.go(7 hunks)cmd/mtr.go(4 hunks)cmd/mtr_test.go(9 hunks)cmd/ping.go(6 hunks)cmd/ping_test.go(33 hunks)cmd/root.go(4 hunks)cmd/traceroute.go(4 hunks)cmd/traceroute_test.go(9 hunks)cmd/utils_test.go(4 hunks)cmd/version_test.go(1 hunks)globalping/client.go(0 hunks)globalping/limits.go(0 hunks)globalping/limits_test.go(0 hunks)globalping/measurements.go(0 hunks)globalping/measurements_test.go(0 hunks)globalping/models.go(0 hunks)globalping/utils_test.go(0 hunks)go.mod(2 hunks)mocks/api/mock_client.go(2 hunks)mocks/api/mock_probe.go(1 hunks)mocks/gen_mocks.sh(1 hunks)mocks/globalping/mock_globalping.go(1 hunks)mocks/mock_viewer.go(0 hunks)mocks/utils/mock_utils.go(1 hunks)mocks/view/mock_viewer.go(1 hunks)storage/config.go(2 hunks)storage/config_test.go(1 hunks)storage/history.go(2 hunks)storage/storage.go(1 hunks)utils/config.go(1 hunks)view/default.go(3 hunks)view/default_test.go(12 hunks)view/history.go(1 hunks)view/infinite.go(2 hunks)view/infinite_test.go(13 hunks)view/json.go(1 hunks)view/json_test.go(1 hunks)view/latency.go(1 hunks)view/latency_test.go(7 hunks)view/output.go(2 hunks)view/output_test.go(1 hunks)view/share_test.go(3 hunks)view/summary_test.go(3 hunks)view/utils_test.go(1 hunks)view/viewer.go(1 hunks)
💤 Files with no reviewable changes (8)
- mocks/mock_viewer.go
- globalping/limits_test.go
- globalping/utils_test.go
- globalping/measurements_test.go
- globalping/limits.go
- globalping/measurements.go
- globalping/models.go
- globalping/client.go
🧰 Additional context used
🧬 Code graph analysis (25)
api/utils_test.go (2)
utils/utils.go (1)
Utils(11-14)storage/storage.go (2)
LocalStorage(16-27)NewLocalStorage(29-34)
cmd/history_test.go (1)
mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
storage/history.go (1)
utils/config.go (1)
ShareURL(8-8)
view/share_test.go (2)
view/viewer.go (1)
NewViewer(24-34)view/printer.go (1)
NewPrinter(62-72)
mocks/gen_mocks.sh (1)
api/client.go (1)
Client(20-57)
cmd/traceroute.go (1)
view/context.go (1)
Context(8-48)
cmd/common.go (3)
cmd/root.go (1)
Root(24-34)view/context.go (1)
Context(8-48)api/measurements.go (1)
StatusUnauthorizedWithTokenRefreshed(23-23)
cmd/dns.go (1)
view/context.go (1)
Context(8-48)
view/infinite_test.go (3)
mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)view/viewer.go (1)
NewViewer(24-34)view/printer.go (1)
NewPrinter(62-72)
cmd/root.go (3)
api/client.go (3)
Client(20-57)NewClient(92-152)Config(59-74)storage/config.go (2)
Token(9-15)Config(21-25)api/probe/probe.go (1)
NewProbe(18-20)
api/measurements_test.go (4)
storage/config.go (2)
Token(9-15)Config(21-25)api/client.go (2)
NewClient(92-152)Config(59-74)utils/utils.go (1)
Utils(11-14)api/measurements.go (1)
StatusUnauthorizedWithTokenRefreshed(23-23)
cmd/http.go (1)
view/context.go (1)
Context(8-48)
cmd/ping_test.go (5)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)view/context.go (1)
Context(8-48)mocks/view/mock_viewer.go (1)
NewMockViewer(31-35)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
cmd/auth_test.go (4)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)storage/config.go (1)
Token(9-15)api/auth.go (2)
IntrospectionResponse(347-363)AuthorizeResponse(38-41)
cmd/dns_test.go (5)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)view/context.go (1)
Context(8-48)mocks/view/mock_viewer.go (1)
NewMockViewer(31-35)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
cmd/traceroute_test.go (5)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)view/context.go (1)
Context(8-48)mocks/view/mock_viewer.go (1)
NewMockViewer(31-35)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
cmd/http_test.go (5)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)view/context.go (1)
Context(8-48)mocks/view/mock_viewer.go (1)
NewMockViewer(31-35)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
storage/config_test.go (1)
storage/config.go (1)
Token(9-15)
cmd/limits_test.go (3)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)api/auth.go (2)
IntrospectionResponse(347-363)AuthorizeError(28-32)
cmd/auth.go (3)
storage/config.go (1)
Token(9-15)api/auth.go (2)
AuthorizeError(28-32)ErrTypeNotAuthorized(25-25)cmd/root.go (1)
Root(24-34)
api/auth.go (1)
storage/config.go (1)
Token(9-15)
view/viewer.go (2)
view/context.go (1)
Context(8-48)view/printer.go (1)
Printer(53-60)
view/summary_test.go (2)
view/viewer.go (1)
NewViewer(24-34)view/printer.go (1)
NewPrinter(62-72)
view/output.go (1)
utils/config.go (1)
ShareURL(8-8)
cmd/install_probe_test.go (1)
mocks/api/mock_probe.go (1)
NewMockProbe(31-35)
🪛 GitHub Actions: Tests
cmd/ping.go
[error] 154-154: Test ping: GetMeasurement invocation caused a panic in gomock during Test_Execute_Ping_Infinite; test failed leading to process exit.
mocks/api/mock_client.go
[error] 92-92: gomock: Controller.Call panic propagated from GetMeasurement during test.
🔍 Remote MCP
Since the DeepWiki sources are not available for this repository, let me search for relevant information about the technologies and patterns being used in this refactor.## Summary of Additional Context Found
Based on the PR changes and supporting research, here are the key findings relevant to reviewing this pull request:
Architecture & Design Pattern Changes
The PR implements proper context propagation following Go best practices, where client libraries accept a Context from calling code to establish a common interface for request-scoped data and cancellation, making it easier for package developers to share code for creating scalable services.
The refactor demonstrates several key architectural improvements:
-
API Abstraction Layer: The new
apipackage creates an abstraction over theglobalping-golibrary, allowing the CLI to work with context-aware interfaces and managing token lifecycle separately from the underlying API client. -
Context Propagation: The PR correctly implements Go's context guidelines by passing Context explicitly to each function that needs it, with Context as the first parameter, typically named ctx, rather than storing contexts in structs.
-
Token Management Refactor: The PR moves from a callback-based token refresh model (
OnTokenRefresh) to explicit storage-backed token management via the newstorage.Tokentype, which aligns with stateless API design.
Critical Observations for Review
Public API Changes:
- All measurement and auth-related client methods now require a
context.Contextas the first parameter - The
OnTokenRefreshcallback mechanism has been removed entirely from public Config - The viewer interface has been significantly restructured with new methods:
OutputDefault,OutputJSON,OutputLatency,OutputLivereplacing the previous genericOutputmethod
Dependency Management:
- Old inline API client implementation (globalping package) is completely removed
- New dependency on
github.com/jsdelivr/globalping-go(v0.1.1) provides the base measurement client - Token types now unified under
storage.Tokeninstead of scattered between packages
Testing Impact:
- Mock organization restructured into
mocks/api/,mocks/utils/, andmocks/view/packages - New mock for the globalping-go
Clientinterface added atmocks/globalping/mock_globalping.go - Tests updated to use
t.Context()instead ofcontext.TODO()for proper test lifecycle management
Potential Review Concerns:
- Error Handling: The PR consolidates error handling in the
apipackage layer with custom status codes (e.g.,StatusUnauthorizedWithTokenRefreshed = 1000), requiring careful review of edge cases - Token Expiry: The new storage-based token uses
time.TimeforExpiryfield with JSON tags - verify serialization/deserialization correctness - Background Goroutine: The
NewClientconstructor spawns a background goroutine for periodicCacheClean()- verify proper cleanup on client shutdown - Viewer API Breakage: The viewer interface changes significantly affect all command implementations and test coverage
🔇 Additional comments (44)
.gitignore (1)
44-44: LGTM!Adding
.DS_Storeto gitignore is a standard best practice for macOS users.view/utils_test.go (1)
7-7: I'll help you verify the compatibility of the externalglobalping-golibrary with the test file. Let me gather evidence about the types and constants used.
<function_calls>
#!/bin/bashCheck go.mod for globalping-go version and list the files in the test directory
echo "=== Checking go.mod for globalping-go version ==="
grep "globalping-go" go.mod 2>/dev/null || echo "Not found in go.mod"echo -e "\n=== Listing view directory files ==="
ls -la view/ | head -20echo -e "\n=== Reading view/utils_test.go to see the types used ==="
wc -l view/utils_test.go
</function_calls>Now let me examine the test file to see what types and constants are being used:
<function_calls>
#!/bin/bashRead the test file to see imports and usage
cat -n view/utils_test.go | head -50
</function_calls>Let me search for all usages of the types in the test file:
<function_calls>
#!/bin/bashSearch for the types being used
echo "=== Types and constants used in view/utils_test.go ==="
rg "globalping.(Measurement|ProbeMeasurement|ProbeDetails|ProbeResult|StatusFinished)" view/utils_test.go -n
</function_calls>Now let me check if there's a go.sum file and verify the globalping-go dependency version:
<function_calls>
#!/bin/bashCheck go.mod and go.sum
echo "=== Full go.mod file ==="
cat go.mod | grep -A5 -B5 "globalping" || echo "No globalping found in go.mod"echo -e "\n=== go.sum entries for globalping ==="
cat go.sum 2>/dev/null | grep globalping || echo "go.sum not available or no globalping entries"
</function_calls>Let me also check the full content of the test file to understand the structure better:
<function_calls>
#!/bin/bashRead the complete test file
cat -n view/utils_test.go
</function_calls>Now let me search for the globalping-go repository to verify the exported types:
<function_calls>
github.com/jsdelivr/globalping-go v0.1.1 types Measurement ProbeMeasurement
</function_calls>Let me also check if there's any documentation or README about this library:
<function_calls>
globalping-go library exported types structure
</function_calls>budget:token_budget200000</budget:token_budget>
mocks/gen_mocks.sh (1)
7-7: I'll help you verify the package name for the globalping mock. Let me start by examining the mock generation script and how the generated mock is used in the codebase.
<function_calls>
{"code": "#!/bin/bash\n# First, let's see the full context of gen_mocks.sh\ncat -n mocks/gen_mocks.sh"}
</function_calls>
10Let me also check how the globalping mock is imported and used in test files:
<function_calls>
{"code": "#!/bin/bash\n# Search for imports and usage of the globalping mock\nrg -n "mocks/globalping" --type go -C3"
</function_calls>
10Let me also search for any references to the "lib" package from the mocks directory:
<function_calls>
{"code": "#!/bin/bash\n# Look for imports of lib package from mocks\nrg -n 'import.*mocks.*lib|from.*mocks.*lib' --type go -C2"
</function_calls>
10Let me check if there are any test files that might reference the globalping mock:
<function_calls>
{"code": "#!/bin/bash\n# Find test files and check for globalping references\nfd -e _test.go | head -20\necho "---"\nrg -n "globalping" --type go -C2"
</function_calls>
10go.mod (1)
7-7: Verify the globalping-go dependency version with the maintainers.The dependency uses a pseudo-version (
v0.1.1-0.20251125110013-2813a850a61f) pointing to a specific commit rather than a released semantic version. While pseudo-versions are a valid Go feature for unreleased code, automated verification of available releases for this dependency was inconclusive. Confirm with the globalping-go maintainers that:
- This pseudo-version represents stable, tested functionality
- A proper semantic release is not available or planned
- This pinning is intentional for your use case
view/json.go (1)
4-11: No changes requested for this segmentNothing stands out here that warrants modification; leaving this implementation as-is.
view/infinite.go (1)
26-38: No changes requested for this segmentThe updated
OutputInfinitesignature and its use ofmeasurementintegrate cleanly with existing helpers; no additional adjustments seem necessary here.view/output_test.go (1)
7-47: No changes requested for this segmentThe updated import and usage of
globalping.ProbeMeasurementin these tests don’t introduce issues that need adjustment.view/json_test.go (1)
11-31: LGTM!The simplified test directly exercises the
OutputJSONpath without unnecessary mock setup. Thenilutils parameter is appropriate since JSON output doesn't depend on utility functions.view/infinite_test.go (2)
9-10: LGTM!Import updates correctly reference the reorganized mock package structure and the new
globalping-golibrary.
19-28: LGTM!Mock creation and
NewViewercalls correctly updated to match the new signatures.cmd/dns.go (2)
73-74: LGTM!Context correctly captured from the command and propagated through the measurement workflow.
114-136: LGTM!The context-aware
CreateMeasurementcall and delegation tohandleMeasurementfollows the established pattern across command handlers.cmd/http.go (2)
90-90: LGTM!Context captured at the command entry point, consistent with other command handlers.
124-146: LGTM!Context-aware measurement creation and output handling follows the established pattern.
cmd/mtr.go (2)
62-63: LGTM!Context correctly captured and propagated through the MTR measurement workflow.
103-125: LGTM!Context-aware
CreateMeasurementandhandleMeasurementcalls are consistent with the other command handlers.cmd/limits.go (1)
20-28: LGTM!Context propagation is correctly implemented -
ctxis obtained from the Cobra command and properly passed to bothTokenIntrospectionandLimitscalls. The intentional silencing of the introspection error (line 23) is reasonable since authentication status shouldn't block displaying rate limits.view/latency_test.go (1)
12-68: LGTM!Test simplification is well done - using concrete
globalping.Measurementobjects instead of mocks makes these tests more readable and maintainable. The test coverage for ping latency output with multiple probe results is thorough.cmd/http_test.go (1)
36-44: LGTM!The mock setup correctly follows the new context-aware API patterns. The test properly verifies the full HTTP measurement flow including creation, await, and output.
cmd/dns_test.go (1)
59-59: Yes, please fetch the exact "go X.Y" line from globalping-cli's go.mod file.storage/config.go (1)
9-15: The review comment is technically accurate regardingomitzerosupport.The web search confirms that
omitzerowas indeed introduced in Go 1.24 (released February 11, 2025), making the review comment's claim factually correct. The concern about ensuring Go 1.24+ targeting is valid—zerotime.Timevalues will be serialized in Go versions prior to 1.24 since the tag will be silently ignored.However, I cannot complete full verification because the repository cannot be cloned to check the project's actual
go.modfile. The concern raised is legitimate and actionable; a developer should verify that the project'sgo.modspecifiesgo 1.24or higher.view/default_test.go (1)
1-387: LGTM!Tests are properly updated to use the new
globalping-gotypes and the refactoredOutputDefaultmethod. The TLS field naming fix (CipherNameinstead of what was likely a typo) aligns with the external library's schema.cmd/root.go (1)
54-78: LGTM!Clean initialization flow separating the globalping client from the api wrapper. Token setup from environment variable is handled correctly with appropriate expiry values.
view/output.go (1)
13-30: LGTM!Clean implementation of
OutputLivethat properly iterates over results and conditionally displays body or raw output based on HTTP GET detection.api/auth_test.go (1)
1-403: LGTM!Tests are well-structured with proper mocking, context usage, and coverage of edge cases (token refresh, no token, no refresh token scenarios). The use of
createDefaultTestStoragehelper keeps tests clean.cmd/traceroute_test.go (3)
8-15: LGTM!The import reorganization and switch to the modular mock packages (
apiMocks,utilsMocks,viewMocks) aligns with the PR's restructuring. The addition ofglobalping-goas a direct import is correct for accessing types likeIPVersion4andIPVersion6.
28-38: LGTM!Context propagation through
t.Context()toCreateMeasurementandAwaitMeasurementis correctly implemented. The viewer expectation properly updated toOutputDefaultwith the new signature(measurementID, measurement, opts).
157-177: LGTM!The negative test case correctly handles the scenario where no API mock is needed (nil values passed to
NewRoot), and properly validates error handling for invalid protocol input.cmd/ping_test.go (4)
11-19: LGTM!Import changes align with the mock reorganization. The direct import of
globalping-gois needed for types likeIPVersion4,IPVersion6, andMeasurementError.
29-36: LGTM!Context-aware API calls and the new
OutputDefaultsignature are correctly wired. The expected measurement is properly passed through the mock chain.
302-326: LGTM!The infinite ping test properly chains
CreateMeasurementandGetMeasurementcalls with ordered expectations using.After(). The context propagation viat.Context()is consistent throughout.
504-510: LGTM!The
MeasurementErrorstructure is correctly used for the rate limit test case with properStatusCode,Type, andMessagefields.cmd/limits_test.go (3)
8-14: LGTM!Import changes correctly bring in the new
apipackage forIntrospectionResponseandAuthorizeErrortypes, while retainingglobalping-goforLimitsResponse.
20-26: LGTM!Context-aware
TokenIntrospection(t.Context(), "")andLimits(t.Context())calls properly align with the new API surface. Return typeapi.IntrospectionResponseis correct.
117-120: LGTM!The unauthorized case correctly uses
api.AuthorizeErrorfor the error return fromTokenIntrospection, reflecting the new error type structure.cmd/auth_test.go (4)
11-18: LGTM!Import changes correctly include
apifor response types andstoragefor theTokentype, alongside the reorganized mock packages.
35-46: LGTM!The token setup uses
storage.Tokencorrectly, and the context-awareTokenIntrospectionandRevokeTokencalls properly passt.Context().
83-87: LGTM!The
Authorizemock correctly uses context and returnsapi.AuthorizeResponse. The callback pattern usingDo()to trigger SIGINT is preserved.
126-142: LGTM!The logout test correctly uses context-aware
Logout(t.Context())call with proper assertion on output.cmd/mtr_test.go (1)
17-79: LGTM!Test correctly migrated to use the new mock packages and context-aware API signatures. The expectations for
CreateMeasurement,AwaitMeasurement, andOutputDefaultare properly configured.cmd/auth.go (1)
52-91: Unable to complete automated verification due to repository access failure.I attempted to verify the context capture concern through two approaches:
- Shell scripts to examine the
Authorizeimplementation and how the callback is invoked- External knowledge sources for documentation on context propagation patterns
Both approaches failed—the repository could not be cloned, and no relevant documentation was found in external sources.
The concern raised in the review comment is technically valid: capturing
ctxin a closure that may be invoked asynchronously after the command context expires is a real concurrency pattern risk. However, definitive verification requires examining:
- Whether
Authorizetruly invokes the callback asynchronously- The lifecycle of the context relative to callback execution
- Whether
RevokeTokengracefully handles cancelled contextsSince this cannot be verified automatically, manual code inspection is necessary to determine if the context is actually at risk of cancellation when
RevokeTokenis called, or if the implementation already handles this correctly.mocks/view/mock_viewer.go (1)
1-128: Auto-generated mock file - no manual review needed.This file is generated by MockGen and should not be manually edited. The generation command is documented in the header.
mocks/api/mock_client.go (2)
1-175: Auto-generated mock file - no manual review needed.This file is generated by MockGen. The mock signatures correctly reflect the context-aware API surface. Ensure the mock is regenerated if the
api/client.gointerface changes.
90-102: Unfortunately, I'm unable to access the repository to verify the claims in the review comment. The repository clone operation has failed, which prevents me from:
- Examining the test files that call
GetMeasurement- Verifying whether
EXPECT()setups exist in the tests- Confirming the actual pipeline failure details
- Validating the specific scenarios that trigger the panic
Unable to verify the mock panic claim—repository access unavailable.
The review comment suggests that
GetMeasurementmock calls fail due to missingEXPECT()setup in tests. While this is a plausible explanation for gomock panics, I cannot confirm:
- Whether test files actually call
GetMeasurementwithout proper expectations- What the exact pipeline failure logs show
- If the suggested fix pattern applies to all affected call sites
Recommend manually reviewing your test files for any
GetMeasurementcalls that lack correspondingEXPECT().GetMeasurement(...)setups, particularly in the test setup/initialization code.
MartinKolarik
left a comment
MartinKolarik
left a comment
There was a problem hiding this comment.
Please resolve the rabbit comments as needed and revert the readme change completely, then rerequest review from me.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/auth.go (1)
143-204: Close HTTP response bodies to prevent connection and file descriptor leaks.The
exchange()method readshttp.Response.Body(viajson.Decoder) at multiple points but never closes it. Adddefer resp.Body.Close()immediately after the successfulc.http.Do(req)call:resp, err := c.http.Do(req) if err != nil { return nil, &AuthorizeError{ ErrorType: ErrTypeExchangeFailed, Description: err.Error(), } } + defer resp.Body.Close() if resp.StatusCode != http.StatusOK {Apply the same fix to
refreshToken(),introspection(), andRevokeToken()methods.
♻️ Duplicate comments (2)
api/auth_test.go (1)
69-69:t.Context()requires Go 1.24.This was flagged in a previous review. The project's
go.modspecifiesgo 1.23, butt.Context()was introduced in Go 1.24. Either updatego.modtogo 1.24or replace allt.Context()calls withcontext.Background().cmd/ping_test.go (1)
48-48:t.Context()usage requires Go 1.24.Same issue as in
api/auth_test.go- this file extensively usest.Context()which requires Go 1.24.
🧹 Nitpick comments (2)
api/client.go (1)
96-108: Guard againstnilGlobalpingclient before starting the cache-clean ticker.
NewClientdoesn’t validateconfig.Globalping, yet the background goroutine unconditionally callsc.globalping.CacheClean(). IfGlobalpingis ever passed asnil, the goroutine will panic.Consider either:
- Validating
config.GlobalpinginNewClient(and failing fast, e.g., via panic or documented precondition), or- Providing a sensible default client when
config.Globalping == nil, and/or- Skipping ticker setup when
globalpingisnil.This helps avoid hard-to-diagnose panics originating from the background goroutine.
Also applies to: 149-156
api/auth.go (1)
43-104: Consider using caller context (with timeout) forserver.ShutdowninAuthorize.
Authorizecurrently callsserver.Shutdown(context.Background()), ignoring the caller’sctxand with no timeout. If shutdown ever blocks (e.g., slow handler or stuck connection), this goroutine could hang longer than intended.Consider something like:
shutdownCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() if err := server.Shutdown(shutdownCtx); err != nil { // optionally log via c.printer }This ties the auth server’s lifetime more closely to the invoking context and bounds shutdown time.
Also applies to: 64-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(2 hunks)api/auth.go(14 hunks)api/auth_test.go(9 hunks)api/client.go(1 hunks)api/measurements.go(1 hunks)cmd/ping.go(6 hunks)cmd/ping_test.go(32 hunks)cmd/root.go(4 hunks)mocks/api/mock_client.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/measurements.go
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/root.go (4)
api/client.go (3)
Client(20-60)NewClient(96-164)Config(62-77)storage/config.go (2)
Token(9-15)Config(21-25)utils/config.go (1)
Config(10-15)api/probe/probe.go (1)
NewProbe(18-20)
cmd/ping_test.go (5)
mocks/api/mock_client.go (1)
NewMockClient(33-37)mocks/globalping/mock_globalping.go (1)
NewMockClient(32-36)view/context.go (1)
Context(8-48)mocks/view/mock_viewer.go (1)
NewMockViewer(31-35)mocks/utils/mock_utils.go (1)
NewMockUtils(31-35)
cmd/ping.go (3)
view/context.go (1)
Context(8-48)cmd/root.go (1)
Root(24-34)view/history.go (1)
HistoryItem(14-21)
api/auth.go (1)
storage/config.go (1)
Token(9-15)
mocks/api/mock_client.go (2)
mocks/globalping/mock_globalping.go (2)
MockClient(21-24)MockClientMockRecorder(27-29)api/auth.go (2)
AuthorizeResponse(38-41)IntrospectionResponse(352-368)
🔇 Additional comments (11)
README.md (1)
22-35: LGTM!TOC reformatting is clean and consistent.
api/auth_test.go (1)
209-238: LGTM!Token refresh test correctly verifies storage persistence of the new token with proper time assertions using
defaultCurrentTime.cmd/ping_test.go (2)
29-36: LGTM!Mock setup correctly updated to use new context-aware API signatures with
apiMocks.NewMockClientandOutputDefault.
504-533: LGTM!Rate-limiting error test correctly validates the error message format and propagation.
cmd/root.go (2)
63-80: LGTM!Client composition correctly separates the low-level
globalping.Client(for direct API calls) from the higher-levelapi.Client(which wraps it with auth/token management). Thedefer apiClient.Close()ensures proper cleanup.
55-62: LGTM!Static token from environment variable correctly set with far-future expiry for "never expires" semantics.
cmd/ping.go (3)
71-122: LGTM!Context propagation correctly implemented in
RunPing. The context from cobra command is properly threaded through topingInfinite,createMeasurement, andhandleMeasurement.
145-206: LGTM!The infinite ping loop correctly passes context to all API calls. Context cancellation will propagate through
GetMeasurementandCreateMeasurementcalls, causing them to return errors when the context is canceled.
209-230: LGTM!
createMeasurementcorrectly updated to accept and propagate context to the API call.api/auth.go (1)
206-239: Clear in-memory token oninvalid_grantingetTokento matchtryToRefreshToken.On refresh failure with
ErrTypeInvalidGrant,getTokenonly callsc.saveToken(nil)but leavesc.tokennon-nil. This means subsequentgetTokencalls will keep trying to refresh with a known-invalid refresh token, unliketryToRefreshToken, which clears both in-memory and persisted tokens.Align
getTokenwithtryToRefreshTokenby clearingc.tokenas well:if err != nil { e, ok := err.(*AuthorizeError) if ok && e.ErrorType == ErrTypeInvalidGrant { + c.token = nil c.saveToken(nil) } return nil, err }This ensures once a refresh token is invalid, the client stops hammering the auth server with doomed refresh attempts and clearly transitions to an "unauthorized" state. Also applies to: 261-293
api/client.go (1)
79-87: Makeclient.Closeidempotent to avoid panics on multiple calls.
Closedirectly closesc.done. IfCloseis called more than once (easy in tests or with multiple deferred closes), this will panic with "close of closed channel". GivenClient.Close()is part of the public interface, making it idempotent is safer.Guard the close with a
sync.Once:type client struct { utils utils.Utils storage *storage.LocalStorage printer *view.Printer http *http.Client globalping globalping.Client mu sync.RWMutex done chan struct{} + closeOnce sync.Once func (c *client) Close() { - close(c.done) + c.closeOnce.Do(func() { + close(c.done) + }) }Also review existing call sites (including tests) for potential multiple
Close()invocations.Also applies to: 96-104, 149-161, 166-168
2 participants
Use the new go package for the api and general cleanup/refactor.
Closes #177