feat(connectivity): add connector and connector-instance commands - #174
feat(connectivity): add connector and connector-instance commands#174Dav-14 wants to merge 21 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds a stack-authenticated Connectivity API client, models, CLI command tree, connector and connector-instance workflows, schema-aware configuration parsing, bounded shell completion, context propagation, and debug-body redaction. ChangesConnectivity API and authentication
Connector-instance workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConnectivityCommand
participant ClientFactory
participant ConnectivityAPI
participant Renderer
User->>ConnectivityCommand: run connector or instance command
ConnectivityCommand->>ClientFactory: create authenticated client
ClientFactory->>ConnectivityAPI: authenticate and resolve stack access
ConnectivityCommand->>ConnectivityAPI: list, retrieve, create, patch, or delete resource
ConnectivityAPI-->>ConnectivityCommand: return validated response
ConnectivityCommand->>Renderer: store and render result
Renderer-->>User: display table, JSON, or confirmation
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
🛑 Changes requested — automated reviewThe patch can expose inline configuration secrets in structured output and has several correctness issues around no-op updates, error reporting, and debug serialization. |
![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
🧹 Nitpick comments (10)
cmd/connectivity/root_test.go (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover public command aliases in this test.
The test checks only canonical command names. Add representative alias paths such as
plugin list,instances ls, andinstances deleteso alias regressions fail in CI. The alias contract is defined indocs/superpowers/specs/2026-08-07-connectivity-cli-design.md.🤖 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 `@cmd/connectivity/root_test.go` around lines 13 - 25, Extend the command paths table in the connectivity command test to include representative public aliases, specifically plugin list, instances ls, and instances delete. Keep the existing Find and name assertions so each alias path is resolved and validated alongside canonical commands.internal/connectivityclient/client.go (1)
208-233: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider bounding the response read.
decodeResponsecallsio.ReadAllon the response body without a limit. A large or hostile response can exhaust memory in the CLI process. Wrap the body inio.LimitReaderwith a generous cap.🤖 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 `@internal/connectivityclient/client.go` around lines 208 - 233, Update decodeResponse to read through io.LimitReader with a generous maximum response size before calling io.ReadAll, preserving the existing decoding and validation behavior while preventing unbounded response-body memory usage.internal/connectivityclient/integration_test.go (1)
74-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not call
requireinside the HTTP handler goroutine.
require.*callst.FailNow, andtestingdocuments thatFailNowmust run on the goroutine that runs the test function. Thehttptesthandler runs on a server goroutine. On failure the handler exits throughruntime.Goexitwithout writing a response, so the client reports an unrelated EOF error and the real assertion message is lost.Use
assert.*in the handler, or capture the decoded body and assert after the call returns.🛠️ Proposed change
- require.Equal(t, http.MethodPatch, req.Method) - require.Equal(t, "application/merge-patch+json", req.Header.Get("Content-Type")) + assert.Equal(t, http.MethodPatch, req.Method) + assert.Equal(t, "application/merge-patch+json", req.Header.Get("Content-Type")) var patch map[string]any - require.NoError(t, json.NewDecoder(req.Body).Decode(&patch)) - spec, ok := patch["spec"].(map[string]any) - require.True(t, ok) - require.NotContains(t, spec, "config") + if !assert.NoError(t, json.NewDecoder(req.Body).Decode(&patch)) { + return + } + spec, _ := patch["spec"].(map[string]any) + assert.NotContains(t, spec, "config")The same change applies to the
require.Equalcall at line 20 and line 27 and line 33 in the lifecycle handler.🤖 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 `@internal/connectivityclient/integration_test.go` around lines 74 - 94, Replace all require.* assertions inside the httptest HTTP handler(s), including the lifecycle handler, with assert.* so failures do not terminate the server goroutine; keep require assertions in the test goroutine after the client call where needed.pkg/clients.go (1)
636-637: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStoring the context is acceptable here; document why.
oauth2.TokenSource.Token()takes no context, so the storedctxis the only way to bound refresh and fetch. Go guidance normally discourages a context field. Add a short comment on the field to record the constraint and to prevent a future removal.🛠️ Proposed addition
type stackTokenSource struct { mu sync.Mutex + // ctx bounds Refresh and FetchStackToken. oauth2.TokenSource.Token + // accepts no context, so the command context is captured here. ctx context.ContextAlso applies to: 675-675, 689-689, 711-736
🤖 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 `@pkg/clients.go` around lines 636 - 637, Add a concise comment to the stored ctx field in each affected client struct explaining that oauth2.TokenSource.Token() accepts no context, so the field is required to bound token refresh and fetch operations; keep the existing context usage unchanged.pkg/http.go (1)
56-80: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRedaction depends on container key names.
isConfigContainerrecognizesconfig,defaults,env, andfiles. A secret that the server returns outside these containers, for example inside an errordetailsobject that echoes a submitted value, still prints in debug output. The current Connectivity models place inline secrets only in those containers, so the coverage matches today's contract. Track this coupling if the wire contract gains a new secret-bearing container.🤖 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 `@pkg/http.go` around lines 56 - 80, Update the redaction logic around isConfigContainer and redactInlineConfigValues to recognize every server response container that may hold inline secret values, including error details objects that echo submitted values. Extend the container-key coverage as the wire contract adds secret-bearing containers, while preserving redaction of value fields within the existing config, defaults, env, and files paths.cmd/connectivity/internal/client.go (1)
85-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm
fctl.Dialoghas only theInfomethod.
silentDialogimplementsInfoalone. IfDialoggains another method, the non-interactive path breaks at compile time in a distant package. Add a compile-time assertion to localize the failure.🛠️ Proposed addition
type silentDialog struct{} +var _ fctl.Dialog = silentDialog{} + func (silentDialog) Info(string, ...any) {}🤖 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 `@cmd/connectivity/internal/client.go` around lines 85 - 87, Add a compile-time interface assertion near silentDialog confirming it implements fctl.Dialog, while preserving its existing Info method implementation; use the repository’s fctl.Dialog symbol so any future interface-method mismatch fails at this local declaration.pkg/clients_test.go (1)
42-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the handler signal idempotent and widen the timing margins.
Two robustness problems exist in these deadline tests.
- The handler calls
close(tokenStarted)directly. If the transport issues the request twice, for example after a redirect or a retry, the second call panics on a closed channel and crashes the test binary. Guard the close withsync.Once.- The test uses a 75 ms deadline and a 300 ms assertion budget. A loaded CI runner can exceed 300 ms of scheduling delay, which produces a flaky failure. Increase both values, keeping the deadline well below the budget.
The same pattern exists at lines 100 and 145 in
TestStackTokenSourceCancellationBoundsStackTokenRefresh.🛠️ Proposed change
tokenStarted := make(chan struct{}) releaseToken := make(chan struct{}) + var startOnce sync.Once var server *httptest.Server server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { switch req.URL.Path { case "/api/auth/.well-known/openid-configuration": w.Header().Set("Content-Type", "application/json") _, _ = fmt.Fprintf(w, `{"token_endpoint":%q}`, server.URL+"/token") case "/token": - close(tokenStarted) + startOnce.Do(func() { close(tokenStarted) }) select { case <-req.Context().Done(): case <-releaseToken: }- ctx, cancel := context.WithTimeout(context.Background(), 75*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)- case <-time.After(300 * time.Millisecond): + case <-time.After(2 * time.Second):Also applies to: 54-55, 85-89
🤖 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 `@pkg/clients_test.go` around lines 42 - 47, The deadline tests use non-idempotent token-start signaling and timing margins that are too tight. In the handlers for the affected cases in TestStackTokenSourceCancellationBoundsStackTokenRefresh and the related test, guard each close(tokenStarted) with sync.Once, and increase both the 75 ms deadline and 300 ms assertion budget while keeping the deadline shorter than the budget.cmd/connectivity/instances/config_test.go (1)
285-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWrap each case in
t.Runfor named failures.The loop asserts two assignments without a subtest. A failure does not identify which assignment failed. Other table tests in this file use
t.Run.♻️ Proposed change
for _, assignment := range tests { - _, err := BuildInstallConfig( - &cobra.Command{}, - pluginWithSchemaAndDefaults(), - InputOptions{SetValues: []string{assignment}}, - mapReadFile(nil), - ) - - require.Error(t, err) - require.NotContains(t, err.Error(), assignment) - require.NotContains(t, err.Error(), secretSentinel) + t.Run(assignment, func(t *testing.T) { + _, err := BuildInstallConfig( + &cobra.Command{}, + pluginWithSchemaAndDefaults(), + InputOptions{SetValues: []string{assignment}}, + mapReadFile(nil), + ) + + require.Error(t, err) + require.NotContains(t, err.Error(), assignment) + require.NotContains(t, err.Error(), secretSentinel) + }) }🤖 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 `@cmd/connectivity/instances/config_test.go` around lines 285 - 296, Wrap each assignment iteration in a named t.Run subtest, using assignment as the subtest name, and move the BuildInstallConfig call plus all related assertions into that subtest so failures identify the specific input while preserving the existing checks.cmd/connectivity/instances/config.go (1)
259-259: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider raising the scanner buffer limit.
bufio.Scanneruses a 64 KiB maximum token size by default. A dotenv line that holds an inline certificate or key can exceed this limit. The scan then fails withbufio.Scanner: token too long, which does not explain the cause.♻️ Proposed change
scanner := bufio.NewScanner(strings.NewReader(contents)) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) line := 0🤖 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 `@cmd/connectivity/instances/config.go` at line 259, Increase the buffer capacity and maximum token size for the bufio.Scanner initialized in the dotenv parsing flow before scanning contents, so long certificate or key lines are accepted while preserving the existing parsing behavior.cmd/connectivity/instances/install.go (1)
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the internal version completion helper.
cmd/connectivity/instances/configure.go:53callscompleteVersions(...)while the exported helper isCompleteVersions(...). The unexported helper is only a wrapper, so give it a distinct name such ascompleteVersionsByNameor inline the wrapper to avoid confusion at call sites.🤖 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 `@cmd/connectivity/instances/install.go` around lines 70 - 72, Rename the unexported version-completion wrapper called by configure.go’s completeVersions(...) to a distinct name such as completeVersionsByName, and update all its call sites consistently while preserving the exported CompleteVersions(...) helper.
🤖 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 `@cmd/connectivity/instances/configure.go`:
- Around line 103-122: Update the config-change flow around BuildConfigureConfig
and specPatch["config"] to detect when the resolved InstanceConfig contains no
environment values and no files, using the existing configHasData helper from
install.go if applicable. Prevent an empty configuration from being added as a
successful no-op patch, and preserve applying non-empty configuration changes.
In `@cmd/connectivity/instances/show.go`:
- Line 65: Redact inline configuration values before storing instances for JSON
output, without mutating the API response model: update show.go line 65 to store
a copied redacted view preserving the inline source while removing
EnvValue.Value and FileMount.Value, and apply the same transformation to each
instance at list.go line 84. Update show_test.go lines 93-109 and list_test.go
lines 50-74 to assert JSON omits inline values; these tests require direct
changes.
In `@cmd/connectivity/plugins/list.go`:
- Line 80: Redact inline default values before storing output models: in
cmd/connectivity/plugins/list.go:80, map each response item to an output-safe
plugin model before assigning c.store.Plugins; in
cmd/connectivity/plugins/show.go:63, apply the same conversion before assigning
c.store.Plugin. Update the JSON tests to verify inline values are redacted while
configuration keys and references remain available.
In `@internal/connectivityclient/client.go`:
- Around line 286-301: Update decodeAPIError so the APIError Message falls back
to response.Status whenever payload.Message is empty, including successfully
decoded null or non-object-compatible payloads; preserve the existing
decode-error fallback and other payload fields.
- Around line 95-115: Update adaptInstancePatchForPinnedAPI so that after
validating configObject, it explicitly sets spec["env"] and spec["files"] to nil
when their corresponding keys are absent, while preserving provided values.
Ensure both sections are cleared before returning the patch.
In `@pkg/http.go`:
- Around line 43-54: Update redactDebugBody to decode JSON with a decoder
configured with UseNumber, preserving numeric text during redaction and
re-marshal. Also update printBody’s map[string]any decoding path to use the same
number-preserving decoder so colorized output retains large integer values.
---
Nitpick comments:
In `@cmd/connectivity/instances/config_test.go`:
- Around line 285-296: Wrap each assignment iteration in a named t.Run subtest,
using assignment as the subtest name, and move the BuildInstallConfig call plus
all related assertions into that subtest so failures identify the specific input
while preserving the existing checks.
In `@cmd/connectivity/instances/config.go`:
- Line 259: Increase the buffer capacity and maximum token size for the
bufio.Scanner initialized in the dotenv parsing flow before scanning contents,
so long certificate or key lines are accepted while preserving the existing
parsing behavior.
In `@cmd/connectivity/instances/install.go`:
- Around line 70-72: Rename the unexported version-completion wrapper called by
configure.go’s completeVersions(...) to a distinct name such as
completeVersionsByName, and update all its call sites consistently while
preserving the exported CompleteVersions(...) helper.
In `@cmd/connectivity/internal/client.go`:
- Around line 85-87: Add a compile-time interface assertion near silentDialog
confirming it implements fctl.Dialog, while preserving its existing Info method
implementation; use the repository’s fctl.Dialog symbol so any future
interface-method mismatch fails at this local declaration.
In `@cmd/connectivity/root_test.go`:
- Around line 13-25: Extend the command paths table in the connectivity command
test to include representative public aliases, specifically plugin list,
instances ls, and instances delete. Keep the existing Find and name assertions
so each alias path is resolved and validated alongside canonical commands.
In `@internal/connectivityclient/client.go`:
- Around line 208-233: Update decodeResponse to read through io.LimitReader with
a generous maximum response size before calling io.ReadAll, preserving the
existing decoding and validation behavior while preventing unbounded
response-body memory usage.
In `@internal/connectivityclient/integration_test.go`:
- Around line 74-94: Replace all require.* assertions inside the httptest HTTP
handler(s), including the lifecycle handler, with assert.* so failures do not
terminate the server goroutine; keep require assertions in the test goroutine
after the client call where needed.
In `@pkg/clients_test.go`:
- Around line 42-47: The deadline tests use non-idempotent token-start signaling
and timing margins that are too tight. In the handlers for the affected cases in
TestStackTokenSourceCancellationBoundsStackTokenRefresh and the related test,
guard each close(tokenStarted) with sync.Once, and increase both the 75 ms
deadline and 300 ms assertion budget while keeping the deadline shorter than the
budget.
In `@pkg/clients.go`:
- Around line 636-637: Add a concise comment to the stored ctx field in each
affected client struct explaining that oauth2.TokenSource.Token() accepts no
context, so the field is required to bound token refresh and fetch operations;
keep the existing context usage unchanged.
In `@pkg/http.go`:
- Around line 56-80: Update the redaction logic around isConfigContainer and
redactInlineConfigValues to recognize every server response container that may
hold inline secret values, including error details objects that echo submitted
values. Extend the container-key coverage as the wire contract adds
secret-bearing containers, while preserving redaction of value fields within the
existing config, defaults, env, and files paths.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba339659-a6d5-49a8-beb2-da1f914d5f45
📒 Files selected for processing (40)
cmd/connectivity/instances/completion.gocmd/connectivity/instances/completion_test.gocmd/connectivity/instances/config.gocmd/connectivity/instances/config_test.gocmd/connectivity/instances/configure.gocmd/connectivity/instances/configure_test.gocmd/connectivity/instances/install.gocmd/connectivity/instances/install_test.gocmd/connectivity/instances/list.gocmd/connectivity/instances/list_test.gocmd/connectivity/instances/root.gocmd/connectivity/instances/show.gocmd/connectivity/instances/show_test.gocmd/connectivity/instances/test_helpers_test.gocmd/connectivity/instances/uninstall.gocmd/connectivity/instances/uninstall_test.gocmd/connectivity/internal/client.gocmd/connectivity/internal/client_test.gocmd/connectivity/plugins/completion.gocmd/connectivity/plugins/completion_test.gocmd/connectivity/plugins/list.gocmd/connectivity/plugins/list_test.gocmd/connectivity/plugins/root.gocmd/connectivity/plugins/show.gocmd/connectivity/plugins/show_test.gocmd/connectivity/plugins/test_helpers_test.gocmd/connectivity/root.gocmd/connectivity/root_test.gocmd/root.gocmd/root_test.godocs/superpowers/plans/2026-08-07-connectivity-cli.mddocs/superpowers/specs/2026-08-07-connectivity-cli-design.mdinternal/connectivityclient/client.gointernal/connectivityclient/client_test.gointernal/connectivityclient/integration_test.gointernal/connectivityclient/models.gopkg/clients.gopkg/clients_test.gopkg/http.gopkg/http_test.go
| configChanged := cmd.Flags().Changed(configFlag) || cmd.Flags().Changed(envFileFlag) || cmd.Flags().Changed(setFlag) | ||
| if configChanged { | ||
| envFiles, err := cmd.Flags().GetStringArray(envFileFlag) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| setValues, err := cmd.Flags().GetStringArray(setFlag) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| config, err := BuildConfigureConfig(cmd, plugin, instance.Spec.Config, InputOptions{ | ||
| ConfigFile: fctl.GetString(cmd, configFlag), | ||
| EnvFiles: envFiles, | ||
| SetValues: setValues, | ||
| }, c.read) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| specPatch["config"] = config | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Empty configuration input produces a no-op patch that reports success.
BuildConfigureConfig always returns a non-nil *InstanceConfig from cloneConfig. If the current instance has no configuration and the user passes an empty --config file, config holds an empty Env map and no files. InstanceConfig marks both fields omitempty, so the patch serializes as "config": {}. The server applies nothing, and the command still prints Instance "…" configured.
install.go guards this case with configHasData at lines 131-133. Apply the same guard here, or fail when the user requested a configuration change that resolves to no data.
♻️ Proposed change
if err != nil {
return nil, err
}
- specPatch["config"] = config
+ if configHasData(config) {
+ specPatch["config"] = config
+ }
}📝 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.
| configChanged := cmd.Flags().Changed(configFlag) || cmd.Flags().Changed(envFileFlag) || cmd.Flags().Changed(setFlag) | |
| if configChanged { | |
| envFiles, err := cmd.Flags().GetStringArray(envFileFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| setValues, err := cmd.Flags().GetStringArray(setFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| config, err := BuildConfigureConfig(cmd, plugin, instance.Spec.Config, InputOptions{ | |
| ConfigFile: fctl.GetString(cmd, configFlag), | |
| EnvFiles: envFiles, | |
| SetValues: setValues, | |
| }, c.read) | |
| if err != nil { | |
| return nil, err | |
| } | |
| specPatch["config"] = config | |
| } | |
| configChanged := cmd.Flags().Changed(configFlag) || cmd.Flags().Changed(envFileFlag) || cmd.Flags().Changed(setFlag) | |
| if configChanged { | |
| envFiles, err := cmd.Flags().GetStringArray(envFileFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| setValues, err := cmd.Flags().GetStringArray(setFlag) | |
| if err != nil { | |
| return nil, err | |
| } | |
| config, err := BuildConfigureConfig(cmd, plugin, instance.Spec.Config, InputOptions{ | |
| ConfigFile: fctl.GetString(cmd, configFlag), | |
| EnvFiles: envFiles, | |
| SetValues: setValues, | |
| }, c.read) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if configHasData(config) { | |
| specPatch["config"] = config | |
| } | |
| } |
🤖 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 `@cmd/connectivity/instances/configure.go` around lines 103 - 122, Update the
config-change flow around BuildConfigureConfig and specPatch["config"] to detect
when the resolved InstanceConfig contains no environment values and no files,
using the existing configHasData helper from install.go if applicable. Prevent
an empty configuration from being added as a successful no-op patch, and
preserve applying non-empty configuration changes.
| if instance == nil { | ||
| return nil, fmt.Errorf("show connectivity instance %q: empty response", args[0]) | ||
| } | ||
| c.store.Instance = *instance |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Redact inline configuration values before JSON serialization.
ShowStore and ListStore retain raw EnvValue.Value and FileMount.Value fields. Therefore, --output json exposes inline secrets. This conflicts with the PR redaction requirement.
cmd/connectivity/instances/show.go#L65-L65: Store a redacted view that preserves theinlinesource without retaining the inline value.cmd/connectivity/instances/list.go#L84-L84: Store the same redacted view for every listed instance.cmd/connectivity/instances/show_test.go#L93-L109: Assert that JSON omits the inline value.cmd/connectivity/instances/list_test.go#L50-L74: Assert that JSON omits the inline value.
Do not mutate the API response in place. A caller can reuse that model after rendering.
📍 Affects 4 files
cmd/connectivity/instances/show.go#L65-L65(this comment)cmd/connectivity/instances/list.go#L84-L84cmd/connectivity/instances/show_test.go#L93-L109cmd/connectivity/instances/list_test.go#L50-L74
🤖 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 `@cmd/connectivity/instances/show.go` at line 65, Redact inline configuration
values before storing instances for JSON output, without mutating the API
response model: update show.go line 65 to store a copied redacted view
preserving the inline source while removing EnvValue.Value and FileMount.Value,
and apply the same transformation to each instance at list.go line 84. Update
show_test.go lines 93-109 and list_test.go lines 50-74 to assert JSON omits
inline values; these tests require direct changes.
| return nil, fmt.Errorf("list connectivity plugins: empty response") | ||
| } | ||
|
|
||
| c.store.Plugins = response.Items |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Redact inline default values before storing output models.
Both commands store raw connectivityclient.Plugin models for JSON output. Plugin.Spec.Defaults can contain inline secret values. JSON output can expose these values, despite the PR redaction requirement.
cmd/connectivity/plugins/list.go#L80-L80: map each response item to an output-safe plugin model before assigningc.store.Plugins.cmd/connectivity/plugins/show.go#L63-L63: map the response to an output-safe plugin model before assigningc.store.Plugin.
Update the JSON tests to assert that inline values are redacted while configuration keys and references remain available.
📍 Affects 2 files
cmd/connectivity/plugins/list.go#L80-L80(this comment)cmd/connectivity/plugins/show.go#L63-L63
🤖 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 `@cmd/connectivity/plugins/list.go` at line 80, Redact inline default values
before storing output models: in cmd/connectivity/plugins/list.go:80, map each
response item to an output-safe plugin model before assigning c.store.Plugins;
in cmd/connectivity/plugins/show.go:63, apply the same conversion before
assigning c.store.Plugin. Update the JSON tests to verify inline values are
redacted while configuration keys and references remain available.
| config, exists := spec["config"] | ||
| if !exists { | ||
| return wirePatch, nil | ||
| } | ||
| delete(spec, "config") | ||
| if config == nil { | ||
| spec["env"] = nil | ||
| spec["files"] = nil | ||
| return wirePatch, nil | ||
| } | ||
| configObject, ok := config.(map[string]any) | ||
| if !ok { | ||
| return nil, fmt.Errorf("reshape connectivity instance patch: spec.config must be an object") | ||
| } | ||
| if env, ok := configObject["env"]; ok { | ||
| spec["env"] = env | ||
| } | ||
| if files, ok := configObject["files"]; ok { | ||
| spec["files"] = files | ||
| } | ||
| return wirePatch, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how configure builds the instance config and whether empty env/files are reachable.
fd -t f 'config.go' cmd/connectivity/instances --exec cat -n {}Repository: formancehq/fctl
Length of output: 21619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate connectivityclient files =="
fd -t f . internal/connectivityclient
echo "== client.go around reshape function =="
wc -l internal/connectivityclient/client.go
sed -n '1,180p' internal/connectivityclient/client.go | cat -n
echo "== models / config types =="
sed -n '1,220p' internal/connectivityclient/models.go | cat -n
echo "== usages of buildConfig/BuildConfigureConfig/config files =="
rg -n "BuildConfigureConfig|BuildInstallConfig|buildConfig|InstanceConfig|Env|Files|config" cmd/connectivity/instances internal/connectivityclient -S
echo "== behavioral probe: JSON/YAML marshaling and merge-patch key presence =="
python3 - <<'PY'
import json
import copy
current = {"spec": {"config": {"env": {"OLD": "value"}, "files": [{"path": "/old"}]}}}
for new in [
{},
{"env": {}},
{"files": []},
{"env": {}, "files": []},
{"env": {"NEW": "value"}, "files": [{"path": "/new"}]},
]:
next_ = copy.deepcopy(current)
config = new.get("config")
if config is not None:
del next_["spec"]["config"]
if config is not None:
if "env" in config:
next_["spec"]["env"] = config["env"]
if "files" in config:
next_["spec"]["files"] = config["files"]
print(json.dumps({"new": new, "result_spec_keys": sorted(next_["spec"].keys()), "env": next_["spec"].get("env"), "files": next_["spec"].get("files")}))
PYRepository: formancehq/fctl
Length of output: 50371
Clear configured env/files when spec.config has none.
BuildConfigureConfig preserves the current configuration by cloning it, so a configure request that sets no new input leaves config.env and/or config.files absent. In adaptInstancePatchForPinnedAPI, absent keys are not copied into the merge patch, so previous spec.env or spec.files remain on the instance. Write explicit nil values for missing sections before returning the patch.
🤖 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 `@internal/connectivityclient/client.go` around lines 95 - 115, Update
adaptInstancePatchForPinnedAPI so that after validating configObject, it
explicitly sets spec["env"] and spec["files"] to nil when their corresponding
keys are absent, while preserving provided values. Ensure both sections are
cleared before returning the patch.
| func decodeAPIError(response *http.Response) error { | ||
| payload := struct { | ||
| Code string `json:"code"` | ||
| Message string `json:"message"` | ||
| Details map[string]any `json:"details"` | ||
| }{} | ||
| if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { | ||
| return &APIError{StatusCode: response.StatusCode, Message: response.Status} | ||
| } | ||
| return &APIError{ | ||
| StatusCode: response.StatusCode, | ||
| Code: payload.Code, | ||
| Message: payload.Message, | ||
| Details: payload.Details, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle non-object error payloads.
json.Decode succeeds for a body such as null or "text" is rejected only for type mismatch; null decodes into the zero struct. The resulting APIError then carries an empty Code and Message, which hides the HTTP status text. Fall back to response.Status when payload.Message is empty.
🛠️ Proposed fix
if err := json.NewDecoder(response.Body).Decode(&payload); err != nil {
return &APIError{StatusCode: response.StatusCode, Message: response.Status}
}
+ if payload.Message == "" {
+ payload.Message = response.Status
+ }
return &APIError{📝 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.
| func decodeAPIError(response *http.Response) error { | |
| payload := struct { | |
| Code string `json:"code"` | |
| Message string `json:"message"` | |
| Details map[string]any `json:"details"` | |
| }{} | |
| if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { | |
| return &APIError{StatusCode: response.StatusCode, Message: response.Status} | |
| } | |
| return &APIError{ | |
| StatusCode: response.StatusCode, | |
| Code: payload.Code, | |
| Message: payload.Message, | |
| Details: payload.Details, | |
| } | |
| } | |
| func decodeAPIError(response *http.Response) error { | |
| payload := struct { | |
| Code string `json:"code"` | |
| Message string `json:"message"` | |
| Details map[string]any `json:"details"` | |
| }{} | |
| if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { | |
| return &APIError{StatusCode: response.StatusCode, Message: response.Status} | |
| } | |
| if payload.Message == "" { | |
| payload.Message = response.Status | |
| } | |
| return &APIError{ | |
| StatusCode: response.StatusCode, | |
| Code: payload.Code, | |
| Message: payload.Message, | |
| Details: payload.Details, | |
| } | |
| } |
🤖 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 `@internal/connectivityclient/client.go` around lines 286 - 301, Update
decodeAPIError so the APIError Message falls back to response.Status whenever
payload.Message is empty, including successfully decoded null or
non-object-compatible payloads; preserve the existing decode-error fallback and
other payload fields.
| func redactDebugBody(data []byte) []byte { | ||
| var body any | ||
| if err := json.Unmarshal(data, &body); err != nil { | ||
| return append([]byte(nil), data...) | ||
| } | ||
| redactInlineConfigValues(body, false) | ||
| redacted, err := json.Marshal(body) | ||
| if err != nil { | ||
| return append([]byte(nil), data...) | ||
| } | ||
| return redacted | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Debug output loses numeric fidelity after the re-marshal.
redactDebugBody decodes into any, so every JSON number becomes a float64. The re-marshal then prints large integers in an altered form. A 64-bit identifier such as 1234567890123456789 prints as 1234567890123456800. The previous code printed the raw bytes.
Use a decoder with UseNumber to keep the original numeric text.
🛠️ Proposed fix
func redactDebugBody(data []byte) []byte {
var body any
- if err := json.Unmarshal(data, &body); err != nil {
+ decoder := json.NewDecoder(bytes.NewReader(data))
+ decoder.UseNumber()
+ if err := decoder.Decode(&body); err != nil {
return append([]byte(nil), data...)
}Note that printBody at line 30 also unmarshals into map[string]any before formatting, so the same rounding applies there. Fixing redactDebugBody alone does not remove the rounding in the colorized path.
🤖 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 `@pkg/http.go` around lines 43 - 54, Update redactDebugBody to decode JSON with
a decoder configured with UseNumber, preserving numeric text during redaction
and re-marshal. Also update printBody’s map[string]any decoding path to use the
same number-preserving decoder so colorized output retains large integer values.
The connectivity API renamed its resources to Connector /
ConnectorVersion / ConnectorInstance and now serves them under
/connectors, /connectors/{name}/versions and /connectorinstances.
Rename the CLI to match and re-model the client against the current
openapi.yaml:
- cmd/connectivity/plugins -> cmd/connectivity/connectors,
cmd/connectivity/instances -> cmd/connectivity/connectorinstances.
- ConnectorSpec is now lean metadata; the installable versions and their
configSchema live in ConnectorVersion objects, so install/configure
resolve a version (the pin, the applied version, or the newest) and
validate configuration against that version's schema.
- The API publishes no per-connector config defaults, so an install now
starts from an empty configuration.
- Drop the client-side patch shim: the API reshapes spec.config into the
CRD layout itself, so the client sends the documented API shape.
- Surface the resolved connector/version/digest and the spec channel in
`connectorinstances show`; `--plugin` becomes `--connector`.
f40be48 to
0fdc07a
Compare
Dav-14
changed the title
|
Rebased onto `main` and aligned with the renamed connectivity-api (RFC-0011 restore, on `develop`): distribution vocabulary is now Connector / ConnectorInstance / ConnectorVersion (paths `/connectors`, `/connectorinstances`), matching the server contract. Not a pure rename — the old models targeted a fatter, older Plugin CRD, so the client was realigned to the leaner shipped shape:
Breaking for users of the earlier branch: JSON keys Build/test/lint clean ( |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #174 (comment)
| if instance == nil { | ||
| return nil, fmt.Errorf("show connectivity connector instance %q: empty response", args[0]) | ||
| } | ||
| c.store.ConnectorInstance = *instance |
There was a problem hiding this comment.
🔴 [blocker] Redact inline configuration before storing output
When an instance contains inline config.env[*].value or config.files[*].value, assigning the API model directly exposes those secrets through --output json. The same raw-model storage occurs in list, install, and configure, so all instance command stores should use a redacted copy while preserving references and source metadata.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| specPatch["config"] = config |
There was a problem hiding this comment.
🟠 [major] Reject empty configuration updates
When the current instance has no config and the user supplies an empty --config file, BuildConfigureConfig returns an empty non-nil config and this sends "config": {}. A JSON merge patch applies no change, yet the command reports success; omit the config patch when configHasData(config) is false so the existing no-changes error is returned.
| Message string `json:"message"` | ||
| Details map[string]any `json:"details"` | ||
| }{} | ||
| if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { |
There was a problem hiding this comment.
🟠 [major] Fall back to HTTP status for empty API errors
For an error response whose body is null or an object without message, decoding succeeds but returns an APIError with an empty message, hiding useful status information such as 500 Internal Server Error. Use response.Status whenever the decoded message is empty.
| } | ||
| } | ||
|
|
||
| func redactDebugBody(data []byte) []byte { |
There was a problem hiding this comment.
🟠 [major] Preserve JSON numbers in debug output
With --debug, decoding JSON into any converts numbers to float64, so large integer identifiers or sequences are rounded before being printed (for example, 1234567890123456789 changes). Use decoders with UseNumber in both the redaction and color-formatting passes.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/connectivityclient/client.go (1)
288-303: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFall back to the HTTP status text when the error payload has no message.
json.Decodesucceeds for a body ofnulland leavespayloadzeroed.APIError.Error()then prints an empty code and message, which hides the HTTP status. Setpayload.Messagetoresponse.Statuswhen it is empty.🛠️ Proposed fix
if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { return &APIError{StatusCode: response.StatusCode, Message: response.Status} } + if payload.Message == "" { + payload.Message = response.Status + } return &APIError{🤖 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 `@internal/connectivityclient/client.go` around lines 288 - 303, Update decodeAPIError so that after successfully decoding the response payload, an empty payload.Message is replaced with response.Status before constructing APIError. Preserve the existing decode-error fallback and all populated payload fields.pkg/http.go (1)
43-54: 🎯 Functional Correctness | 🟡 Minor | 💤 Low valueDebug output loses numeric fidelity after the re-marshal.
redactDebugBodydecodes intoany, so each JSON number becomes afloat64. Large integers then print in an altered form. For example,1234567890123456789prints as1234567890123456800. Decode withUseNumberto keep the original numeric text. The same rounding applies to themap[string]anydecode inprintBodyat Line 30.🛠️ Proposed fix
func redactDebugBody(data []byte) []byte { var body any - if err := json.Unmarshal(data, &body); err != nil { + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + if err := decoder.Decode(&body); err != nil { return append([]byte(nil), data...) }🤖 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 `@pkg/http.go` around lines 43 - 54, Update redactDebugBody and the map[string]any decoding in printBody to use json.Decoder with UseNumber enabled instead of json.Unmarshal into any, preserving original JSON number text through redaction and re-marshalling while keeping existing fallback behavior.
🧹 Nitpick comments (4)
cmd/connectivity/connectorinstances/completion.go (1)
226-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
setFlagconstant instead of the literal"set".
suppliedSetKeyslooks up the flag by the literal"set". The command definitions in this package register the flag through thesetFlagconstant. If the constant changes, this lookup silently returns no supplied keys and completion suggests keys the user already provided.♻️ Proposed refactor
- if cmd != nil && cmd.Flags().Lookup("set") != nil { - if setValues, err := cmd.Flags().GetStringArray("set"); err == nil { + if cmd != nil && cmd.Flags().Lookup(setFlag) != nil { + if setValues, err := cmd.Flags().GetStringArray(setFlag); err == nil { values = append(values, setValues...) } }🤖 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 `@cmd/connectivity/connectorinstances/completion.go` around lines 226 - 240, Update suppliedSetKeys to use the existing setFlag constant when looking up and reading the flag, replacing the literal "set" while preserving the current argument-merging and key-detection behavior.cmd/connectivity/connectorinstances/list_test.go (1)
76-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the root registration test and cover all five subcommands.
This test verifies the command tree built by
root.go, but it lives inlist_test.go. It also asserts onlylistandshow, whileNewCommandregistersinstall,configure, anduninstallas well. Add the missing entries so an accidental removal of a mutation command fails this package's tests.♻️ Proposed change
wantAliases := map[string][]string{ "list": {"ls", "l"}, "show": {"get", "g", "sh", "s"}, + "install": {"i"}, + "configure": {"config", "update", "c"}, + "uninstall": {"remove", "rm", "delete", "u"}, }Align the expected aliases with the values declared in
install.go,configure.go, anduninstall.go. Consider placing this test in aroot_test.gofile inside this package.🤖 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 `@cmd/connectivity/connectorinstances/list_test.go` around lines 76 - 91, Move TestConnectorInstanceRootRegistersReadCommandsAndAliases from list_test.go into a root_test.go test file, and expand its expected command map to cover list, show, install, configure, and uninstall. Use the aliases declared by each corresponding command implementation, while preserving the existing root Use and root Aliases assertions.internal/connectivityclient/integration_test.go (1)
16-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assertinside bothhttptesthandlers.
requirecallst.FailNow, which exits only the server goroutine. This can hide the assertion failure behind an HTTP client error. Useassertfor handler checks, or record failures and assert them from the test goroutine.🤖 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 `@internal/connectivityclient/integration_test.go` around lines 16 - 44, Replace the require.Equal calls inside the httptest server handler with assert.Equal, covering the Accept header in the connector list case and Content-Type checks in the POST and PATCH cases. Keep the existing request validation and response behavior unchanged.cmd/connectivity/connectorinstances/config.go (1)
254-284: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRaise the scanner buffer for long dotenv lines.
bufio.Scanneruses a 64 KiB maximum token size by default. If an env file contains one long line, for example an inlined certificate or key material,scanner.Err()returnsbufio.ErrTooLongand the command fails with an opaque message. Set an explicit larger buffer.♻️ Proposed change
scanner := bufio.NewScanner(strings.NewReader(contents)) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) line := 0🤖 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 `@cmd/connectivity/connectorinstances/config.go` around lines 254 - 284, Update applyDotenv to configure the bufio.Scanner buffer before scanning, setting a larger explicit maximum token size sufficient for long dotenv values such as certificates or keys. Preserve the existing line parsing and scanner.Err handling.
🤖 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.
Duplicate comments:
In `@internal/connectivityclient/client.go`:
- Around line 288-303: Update decodeAPIError so that after successfully decoding
the response payload, an empty payload.Message is replaced with response.Status
before constructing APIError. Preserve the existing decode-error fallback and
all populated payload fields.
In `@pkg/http.go`:
- Around line 43-54: Update redactDebugBody and the map[string]any decoding in
printBody to use json.Decoder with UseNumber enabled instead of json.Unmarshal
into any, preserving original JSON number text through redaction and
re-marshalling while keeping existing fallback behavior.
---
Nitpick comments:
In `@cmd/connectivity/connectorinstances/completion.go`:
- Around line 226-240: Update suppliedSetKeys to use the existing setFlag
constant when looking up and reading the flag, replacing the literal "set" while
preserving the current argument-merging and key-detection behavior.
In `@cmd/connectivity/connectorinstances/config.go`:
- Around line 254-284: Update applyDotenv to configure the bufio.Scanner buffer
before scanning, setting a larger explicit maximum token size sufficient for
long dotenv values such as certificates or keys. Preserve the existing line
parsing and scanner.Err handling.
In `@cmd/connectivity/connectorinstances/list_test.go`:
- Around line 76-91: Move
TestConnectorInstanceRootRegistersReadCommandsAndAliases from list_test.go into
a root_test.go test file, and expand its expected command map to cover list,
show, install, configure, and uninstall. Use the aliases declared by each
corresponding command implementation, while preserving the existing root Use and
root Aliases assertions.
In `@internal/connectivityclient/integration_test.go`:
- Around line 16-44: Replace the require.Equal calls inside the httptest server
handler with assert.Equal, covering the Accept header in the connector list case
and Content-Type checks in the POST and PATCH cases. Keep the existing request
validation and response behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf3cafad-454b-4726-83a9-eb3eab453b55
📒 Files selected for processing (33)
cmd/connectivity/connectorinstances/completion.gocmd/connectivity/connectorinstances/completion_test.gocmd/connectivity/connectorinstances/config.gocmd/connectivity/connectorinstances/config_test.gocmd/connectivity/connectorinstances/configure.gocmd/connectivity/connectorinstances/configure_test.gocmd/connectivity/connectorinstances/install.gocmd/connectivity/connectorinstances/install_test.gocmd/connectivity/connectorinstances/list.gocmd/connectivity/connectorinstances/list_test.gocmd/connectivity/connectorinstances/root.gocmd/connectivity/connectorinstances/show.gocmd/connectivity/connectorinstances/show_test.gocmd/connectivity/connectorinstances/test_helpers_test.gocmd/connectivity/connectorinstances/uninstall.gocmd/connectivity/connectorinstances/uninstall_test.gocmd/connectivity/connectorinstances/version.gocmd/connectivity/connectors/completion.gocmd/connectivity/connectors/completion_test.gocmd/connectivity/connectors/list.gocmd/connectivity/connectors/list_test.gocmd/connectivity/connectors/root.gocmd/connectivity/connectors/show.gocmd/connectivity/connectors/show_test.gocmd/connectivity/connectors/test_helpers_test.gocmd/connectivity/root.gocmd/connectivity/root_test.gointernal/connectivityclient/client.gointernal/connectivityclient/client_test.gointernal/connectivityclient/integration_test.gointernal/connectivityclient/models.gopkg/http.gopkg/http_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/connectivity/root_test.go
- cmd/connectivity/root.go
- pkg/http_test.go
Descriptions are full sentences and pterm sizes a column to its widest cell, so one connector wrapped every row and the table stopped lining up -- the `Tags` column, which is what the list is useful for, ended up on its own line under each entry. `connectors show` prints the description, which is where a sentence belongs.
`connectorinstances list` failed with decode connectivity response: cursor.data[4]: connector instance spec.connector is required on a response the server had answered 200. A ConnectorInstance may pin spec.image on the CR instead of referencing a Connector -- the two are mutually exclusive -- so the API returns it with an empty connector, and one such row failed the decode of the entire page. validateConnectorInstance guards decoding, not creation: both call sites are response paths, so requiring a field here only rejects legitimate data. Keep metadata.name, which identifies the row and Kubernetes always sets, and drop connector and ledger -- enforcing a write-side invariant while reading is the same mistake in both cases. The two table cases asserting the old behaviour go with it.
2 participants
Summary
fctl connectivitymodule--set,--env-file,--config, and@fileinputsCompatibility note
--start-sequenceis intentionally not exposed: Connectivity API 0.1.0 models an integer sequence, while the pinned CRD uses an opaque byte cursor without a lossless mapping.Testing
nix develop -c just testsnix develop -c go test -race ./internal/connectivityclient ./cmd/connectivity/... ./pkgnix develop -c just completionsnix develop -c just pre-commitCombined Connectivity coverage: 87.5%.