fix: suggest to check logs if no services are found #1714
Conversation
WalkthroughReplaces local Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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: 0
🧹 Nitpick comments (2)
src/pkg/agent/tools/services.go (1)
44-55: Prefer returningnoServicesErr.Error()(stable user-facing text) instead oferr.Error()(may include wrappers).
Right now, if upstream wrapsErrNoServices, the user may see extra prefix text even though you successfullyerrors.As-matched the typed error.- if errors.As(err, &noServicesErr) { - return err.Error(), nil - } + if errors.As(err, &noServicesErr) { + return noServicesErr.Error(), nil + }src/pkg/agent/tools/services_test.go (1)
190-204: Add an assertion for the new “check logs for deployment status” guidance.
Current expectation only checks the prefix, so a regression could drop the guidance without failing tests.- resultTextContains: "no services found in project", + resultTextContains: "no services found in project", + // consider also asserting: "check logs for deployment status"(If you prefer keeping one field, you could add a second
assert.Contains(t, result, "...")in the test body for this case.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/cmd/cli/command/globals.go(0 hunks)src/cmd/cli/command/globals_test.go(0 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/services_test.go(1 hunks)src/pkg/cli/getServices.go(2 hunks)src/pkg/mcp/deployment_info/deployment_info.go(2 hunks)src/pkg/migrate/heroku.go(1 hunks)
💤 Files with no reviewable changes (2)
- src/cmd/cli/command/globals.go
- src/cmd/cli/command/globals_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/mcp/deployment_info/deployment_info.go (1)
src/pkg/cli/getServices.go (1)
ErrNoServices(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: nix-shell-test
- GitHub Check: go-playground-test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
src/pkg/migrate/heroku.go (1)
336-343: Doc comment spacing change only; no functional impact. The added blank comment lines read fine and won’t affect behavior.src/pkg/cli/getServices.go (1)
3-25: UpdatedErrNoServicesmessage is clear and consistent (includes “check logs…” and quotes project).
No concerns with thefmt.Sprintf(...%q...)change.src/pkg/mcp/deployment_info/deployment_info.go (1)
3-34: No import cycle detected. The search confirmed thatsrc/pkg/clidoes not importsrc/pkg/mcp/deployment_info(directly or indirectly), so the new dependency is safe and will not break builds.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pkg/cli/getServices_test.go (1)
73-79: Exercise the intendedlongpath in “no services long” testThe “no services long” subtest still calls
PrintServices(..., false)(Line 108), so it’s not actually covering the long-output variant.- err := PrintServices(ctx, "empty", &provider, false) + err := PrintServices(ctx, "empty", &provider, true)src/pkg/cli/deploymentinfo.go (1)
17-50: Defensive nil-check forserviceInfo.Serviceto avoid panicSince
ServiceInfo.Serviceoriginates from RPC responses, consider guardingserviceInfo.Service == nilbefore accessing.Name.
🧹 Nitpick comments (2)
src/pkg/cli/getServices_test.go (1)
73-79: Nit: update failure strings referencingGetServices()A few
t.Fatalf(...)messages still sayGetServices()even though the test now callsPrintServices().Also applies to: 81-87, 107-112, 115-121
src/pkg/agent/tools/services_test.go (1)
10-13: Avoid importing the same package twice (clivsdefangcli)This is the same module path imported under two names; consider keeping one alias to reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/cmd/cli/command/compose.go(1 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/services_test.go(5 hunks)src/pkg/cli/client/byoc/aws/byoc.go(1 hunks)src/pkg/cli/client/byoc/aws/domain_integration_test.go(0 hunks)src/pkg/cli/client/byoc/baseclient.go(1 hunks)src/pkg/cli/client/byoc/do/byoc.go(1 hunks)src/pkg/cli/client/byoc/gcp/byoc.go(1 hunks)src/pkg/cli/deploymentinfo.go(4 hunks)src/pkg/cli/getServices.go(3 hunks)src/pkg/cli/getServices_test.go(4 hunks)src/pkg/mcp/deployment_info/deployment_info.go(0 hunks)
💤 Files with no reviewable changes (2)
- src/pkg/cli/client/byoc/aws/domain_integration_test.go
- src/pkg/mcp/deployment_info/deployment_info.go
🧰 Additional context used
🧬 Code graph analysis (7)
src/pkg/cli/getServices_test.go (1)
src/pkg/cli/getServices.go (1)
PrintServices(44-58)
src/pkg/cli/client/byoc/baseclient.go (1)
src/pkg/utils.go (1)
Ensure(135-139)
src/cmd/cli/command/compose.go (1)
src/pkg/cli/getServices.go (1)
PrintServices(44-58)
src/pkg/agent/tools/default_tool_cli.go (3)
src/pkg/cli/getServices.go (2)
GetServices(28-42)UpdateServiceStates(60-103)src/pkg/cli/deploymentinfo.go (2)
Service(8-15)GetServiceStatesAndEndpoints(17-53)src/pkg/cli/compose/loader.go (1)
Services(29-29)
src/pkg/agent/tools/services_test.go (2)
src/pkg/cli/deploymentinfo.go (1)
Service(8-15)src/pkg/cli/getServices.go (1)
GetServices(28-42)
src/pkg/cli/deploymentinfo.go (2)
src/protos/io/defang/v1/fabric.pb.go (6)
ServiceInfo(2065-2088)ServiceInfo(2101-2101)ServiceInfo(2116-2118)Service(3816-3821)Service(3834-3834)Service(3849-3851)src/pkg/term/table.go (1)
Table(11-13)
src/pkg/cli/getServices.go (2)
src/protos/io/defang/v1/fabric.pb.go (16)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)GetServicesResponse(3593-3600)GetServicesResponse(3613-3613)GetServicesResponse(3628-3630)GetServicesRequest(4143-4148)GetServicesRequest(4161-4161)GetServicesRequest(4176-4178)ServiceInfo(2065-2088)ServiceInfo(2101-2101)ServiceInfo(2116-2118)Service(3816-3821)Service(3834-3834)Service(3849-3851)src/pkg/cli/client/provider.go (1)
Provider(43-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (12)
src/pkg/cli/client/byoc/do/byoc.go (1)
99-99: LGTM: Delegation to centralized path helper.The refactoring to use
GetProjectUpdatePathsuccessfully centralizes path construction logic across BYOC providers.src/pkg/cli/client/byoc/baseclient.go (1)
295-299: LGTM: Helper centralizes path construction.The new helper method successfully consolidates duplicate path logic from multiple BYOC providers. The validation using
pkg.Ensureis appropriate for an internal precondition check.src/pkg/cli/client/byoc/gcp/byoc.go (1)
960-960: LGTM: Consistent with path helper refactoring.The change aligns with the centralized path construction pattern applied across all BYOC providers.
src/pkg/cli/client/byoc/aws/byoc.go (1)
494-494: LGTM: Completes path helper refactoring.The change successfully centralizes path construction logic, ensuring consistency across all BYOC provider implementations.
src/pkg/cli/getServices_test.go (1)
73-105: PrintServices migration in tests looks consistentSwitching the test coverage to
PrintServices(...)matches the new split between data retrieval and presentation.src/cmd/cli/command/compose.go (1)
593-611: Correctly routespsthroughcli.PrintServicesCaller remains responsible for handling
cli.ErrNoServices, while printing stays encapsulated in the cli package.src/pkg/agent/tools/default_tool_cli.go (1)
72-83: GetServices tool wiring matches new cli service modelImplementation is consistent with the refactor (fetch → health update → convert-to-view).
src/pkg/agent/tools/services_test.go (1)
59-67: Tests updated cleanly to[]*cli.Serviceand new no-services messageAlso applies to: 190-205, 233-253
src/pkg/agent/tools/interfaces.go (1)
15-34: Interface updates align with the new cli types (Service,TailOptions)src/pkg/cli/deploymentinfo.go (1)
55-76: Nice separation: data shaping vs printingsrc/pkg/cli/getServices.go (2)
21-26: ErrNoServices message matches the PR goal (“check logs…”)
28-58: Good split: GetServices (data) vs PrintServices (presentation)
Description
Change error message: when no services were found, suggest to check logs for deployment status.
Summary by CodeRabbit
Bug Fixes
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.