fix(connector): test verb fetches config via GetConnector before probing#19
Merged
Conversation
Server's TestConnector RPC validates a full `{config: {...}}` body — it's
a pre-create probe, not a test-existing op. The previous CLI sent a bare
`{connectorId}` which the server rejected with `config is required`
(typed-error 400) and, before that, with a string-vs-int field-type
mismatch on `connectorId`.
Behavior change:
- `ana connector test <id>` now does GetConnector → builds a config from
the returned `<dialect>Metadata` block → posts to TestConnector. The
CLI surface (id-only positional) is unchanged.
- `<dialect>Metadata` blocks omit secrets; we fill `password: "redacted"`
so the dialect-specific config validates. The driver returns its
auth/dial error via the standard `200 {error: ...}` shape, which the
CLI surfaces as `FAIL: <msg>` — the same contract as before.
Coverage gate stays at 100% on `./internal/...`. The previous CATALOG
DEVIATION docstring is replaced with a CATALOG REALITY block describing
why the verb is forced through Get + Test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe connector test flow now calls GetConnector, builds a full connector config from the returned connector (converting each Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant GetConn as GetConnector RPC
participant Extract as ConfigExtractor
participant TestConn as TestConnector RPC
CLI->>GetConn: GetConnector(connectorId)
GetConn-->>CLI: ConnectorResponse (with <dialect>Metadata)
CLI->>Extract: configFromGetConnector(response)
Extract-->>CLI: Derived Config Map (dialect keys, ensure password:"redacted")
CLI->>TestConn: TestConnector(config)
TestConn-->>CLI: Test Result / Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/test.go`:
- Around line 100-104: The code mutates the caller's map by writing "redacted"
into block inside the loop; to avoid surprising in-place mutation, clone the
block (a copy of the map derived from getRaw/raw) before modifying it in
configFromGetConnector so you write into the copy rather than the original;
locate the manipulation of variable block in configFromGetConnector (called
after getRaw) and create a shallow map copy, then perform the secret insertion
on that copy and use the copy for downstream logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74db3685-03e6-4a76-b9b8-9e0692d7eac1
📒 Files selected for processing (2)
internal/connector/test.gointernal/connector/test_test.go
Avoid in-place mutation of the caller's map when inserting the 'redacted' password placeholder. The function is currently called once with a fresh map, but defending against future reuse keeps the helper a pure function. Addresses CodeRabbit nit on PR #19.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/connector/test.go`:
- Around line 105-109: The redaction loop currently only injects a placeholder
for the "password" key (the slice literal in the for loop that iterates over
[]string{"password"} and writes to the out map), which misses other
dialect-specific secret fields; either expand that slice to include additional
known secret keys (e.g., "privateKey", "clientSecret", etc.) so test redaction
covers other connectors, or add a clear inline comment next to the for loop
explaining this limitation and that new dialects must extend the slice when
added; update the loop that writes out[secret] = "redacted" (and any tests
relying on it) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f530511-e24a-4b7a-be6d-78b46e508e76
📒 Files selected for processing (1)
internal/connector/test.go
Only "password" is placeholdered today. Postgres uses it; Snowflake's non-password auth modes (keypair, oauth-sso, oauth-individual) use privateKey / oauthClientSecret. Document that so a reader extending the dialect matrix knows to update the slice. Addresses PR #19 review nit.
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
1 participant
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.
Summary
ana connector test <id>previously sent{connectorId: <int>}which the server rejected (invalid value for string field connectorIdand, after stringifying,config is required). The TestConnector RPC validates a full{config: {...}}body — it's a pre-create probe, not a test-by-id op.<dialect>Metadatablock, and posts that to TestConnector. The CLI surface stays id-only.password: "redacted"so config validation passes; the driver returns its real auth/dial error via the standard{error: ...}shape and the CLI surfaces it asFAIL: <msg>(unchanged contract).Validation
./internal/...coverage gate stays at 100.0%.TestConnectorTestin the e2e suite (PR feat(e2e): comprehensive live-smoke coverage across all captured verbs #18) flips from FAIL to PASS once this lands.Test plan
make lintmake cover(100.0%)go test -v ./e2e/ -run TestConnectorTest$against the demo org → PASSSummary by CodeRabbit
Bug Fixes
connector testnow retrieves full connector metadata before testing, improving validation and ensuring complete configuration is used.Tests