Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR consistently replaces the "cluster" concept with "fabricAddr" across CLI commands, client connection code, login/logout flows, agent/debug/mcp components, and related tests—updating struct fields, function signatures, flag bindings, and call sites without altering core control flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/pkg/cli/connect.go (1)
15-17: Debug message still references "cluster" after parameter rename.The parameter was renamed to
fabricAddr, but the debug message on line 17 still says"cluster %q". Consider updating for consistency:func Connect(fabricAddr string, requestedTenant types.TenantNameOrID) *client.GrpcClient { host := client.NormalizeHost(fabricAddr) - term.Debugf("Using tenant %q for cluster %q", requestedTenant, host) + term.Debugf("Using tenant %q for fabric %q", requestedTenant, host)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/connect.go` around lines 15 - 17, Update the debug message in the Connect function to reflect the renamed parameter: replace the outdated "cluster %q" wording with something like "fabric %q" or "fabric address %q". Locate the term.Debugf call in Connect (variables requestedTenant and host) and modify the format string so it reads "Using tenant %q for fabric %q" (or similar) while keeping the same arguments.src/pkg/agent/tools/setConfig_test.go (1)
97-103: Consider renaming the test constant for consistency.The test struct field was renamed to
fabricAddr, but the constant is still namedtestCluster. This is a minor inconsistency.const ( - testCluster = "test-cluster" + testFabricAddr = "test-fabric" testConfigName = "test-config" testValue = "test-value" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/agent/tools/setConfig_test.go` around lines 97 - 103, Rename the test constant testCluster to a name consistent with the renamed test struct field (e.g., testFabricAddr or testFabric) and update all usages in the setConfig_test.go file so the constant name matches the struct field fabricAddr; ensure testConfigName and testValue remain unchanged and adjust any test case initializers that refer to testCluster to use the new constant name (look for the constants block and the tests []struct initializer).src/pkg/mcp/mcp_server.go (1)
34-34: Consider renaming analytics property key for consistency.You now pass
t.fabricAddrunder the"cluster"property key. If analytics consumers already migrated, consider using afabricAddrkey to avoid semantic drift.Possible cleanup
- track.Evt("MCP Tool Called", track.P("tool", name), track.P("client", t.client), track.P("cluster", t.fabricAddr), track.P("provider", *t.providerId)) + track.Evt("MCP Tool Called", track.P("tool", name), track.P("client", t.client), track.P("fabricAddr", t.fabricAddr), track.P("provider", *t.providerId)) - track.Evt("MCP Tool Done", track.P("tool", name), track.P("client", t.client), track.P("cluster", t.fabricAddr), track.P("provider", *t.providerId), track.P("error", err)) + track.Evt("MCP Tool Done", track.P("tool", name), track.P("client", t.client), track.P("fabricAddr", t.fabricAddr), track.P("provider", *t.providerId), track.P("error", err))Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/mcp/mcp_server.go` at line 34, Rename the analytics property key so it consistently reflects the underlying value: replace the use of the "cluster" property when calling track.Evt with a "fabricAddr" property (e.g., change track.Evt(..., track.P("cluster", t.fabricAddr), ...) to track.P("fabricAddr", t.fabricAddr)). Update every occurrence in this file where t.fabricAddr is passed under "cluster" (including the other call around line 41) to use "fabricAddr" instead so analytics payloads remain semantically consistent.src/cmd/cli/command/commands_test.go (1)
113-117: Make the test helper scheme-agnostic for future HTTPS test servers.The current helper strips only
http://. If a test later uses TLS (https://), the--clusterargument may include a scheme unexpectedly.♻️ Suggested tweak
func testCommand(t *testing.T, args []string, fabricAddr string) error { t.Helper() if fabricAddr != "" { - args = append(args, "--cluster", strings.TrimPrefix(fabricAddr, "http://")) + addr := strings.TrimPrefix(strings.TrimPrefix(fabricAddr, "http://"), "https://") + args = append(args, "--cluster", addr) } RootCmd.SetArgs(args) return RootCmd.ExecuteContext(context.Background()) // t.Context() only works for first test? }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/commands_test.go` around lines 113 - 117, In testCommand, the helper currently strips only the "http://" prefix from fabricAddr which fails for "https://" test servers; update the logic in testCommand to be scheme-agnostic by parsing fabricAddr (e.g., using url.Parse or equivalent) and extracting only the Host (or Hostname:Port) before appending the "--cluster" arg so both "http://" and "https://" (and any other schemes) are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/globals.go`:
- Line 150: The ToMap serialization uses the wrong key "DEFANG_CLUSTER" for the
FabricAddr field, causing config round-trip drift; update the map assignment in
ToMap to use "DEFANG_FABRIC" instead of "DEFANG_CLUSTER" so FabricAddr
(global.FabricAddr) is serialized under the same key the initializer reads,
ensuring consistent load/save behavior.
---
Nitpick comments:
In `@src/cmd/cli/command/commands_test.go`:
- Around line 113-117: In testCommand, the helper currently strips only the
"http://" prefix from fabricAddr which fails for "https://" test servers; update
the logic in testCommand to be scheme-agnostic by parsing fabricAddr (e.g.,
using url.Parse or equivalent) and extracting only the Host (or Hostname:Port)
before appending the "--cluster" arg so both "http://" and "https://" (and any
other schemes) are handled correctly.
In `@src/pkg/agent/tools/setConfig_test.go`:
- Around line 97-103: Rename the test constant testCluster to a name consistent
with the renamed test struct field (e.g., testFabricAddr or testFabric) and
update all usages in the setConfig_test.go file so the constant name matches the
struct field fabricAddr; ensure testConfigName and testValue remain unchanged
and adjust any test case initializers that refer to testCluster to use the new
constant name (look for the constants block and the tests []struct initializer).
In `@src/pkg/cli/connect.go`:
- Around line 15-17: Update the debug message in the Connect function to reflect
the renamed parameter: replace the outdated "cluster %q" wording with something
like "fabric %q" or "fabric address %q". Locate the term.Debugf call in Connect
(variables requestedTenant and host) and modify the format string so it reads
"Using tenant %q for fabric %q" (or similar) while keeping the same arguments.
In `@src/pkg/mcp/mcp_server.go`:
- Line 34: Rename the analytics property key so it consistently reflects the
underlying value: replace the use of the "cluster" property when calling
track.Evt with a "fabricAddr" property (e.g., change track.Evt(...,
track.P("cluster", t.fabricAddr), ...) to track.P("fabricAddr", t.fabricAddr)).
Update every occurrence in this file where t.fabricAddr is passed under
"cluster" (including the other call around line 41) to use "fabricAddr" instead
so analytics payloads remain semantically consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/compose_test.gosrc/cmd/cli/command/debug.gosrc/cmd/cli/command/generate.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/init.gosrc/cmd/cli/command/login.gosrc/cmd/cli/command/logout.gosrc/cmd/cli/command/mcp.gosrc/cmd/cli/command/session.gosrc/cmd/cli/command/whoami.gosrc/cmd/cli/command/workspace.gosrc/pkg/agent/agent.gosrc/pkg/agent/tools/auth.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/destroy_test.gosrc/pkg/agent/tools/estimate_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/agent/tools/listConfig_test.gosrc/pkg/agent/tools/removeConfig_test.gosrc/pkg/agent/tools/services_test.gosrc/pkg/agent/tools/setConfig_test.gosrc/pkg/cli/client/cluster.gosrc/pkg/cli/composeUp_test.gosrc/pkg/cli/configSet_test.gosrc/pkg/cli/connect.gosrc/pkg/cli/getServices_test.gosrc/pkg/cli/logout.gosrc/pkg/cli/logout_test.gosrc/pkg/debug/debug.gosrc/pkg/login/login.gosrc/pkg/login/login_test.gosrc/pkg/mcp/mcp_server.gosrc/pkg/mcp/tools/tools.gosrc/pkg/setup/setup.go
jordanstephens
approved these changes
Feb 27, 2026
edwardrf
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Best reviewed per commit. I've renamed "cluster" to "fabricAddr".
Linked Issues
Fixes #1886
Checklist
Summary by CodeRabbit