feat(connector): Databricks create — 4 auth modes with unit + e2e coverage#21
Merged
Merged
Conversation
Group.Flags closures need a Lookup-guarded int wrapper — stdlib flag.IntVar panics on duplicate declarations, and the existing DeclareString/DeclareBool helpers already cover string/bool. This completes the primitive set so dialect Groups can inherit-declare integer flags (e.g. Databricks --port) without clobbering leaves.
…, OAuth SSO, OAuth individual
Mirrors the Snowflake create structure. One dialect Group with the shared
workspace flags (name, host, http-path, port, catalog, schema) inherited
by every auth-mode leaf via cli.ApplyAncestorFlags; four leaves, one per
auth mode, each populating a nested `databricksAuth` one-of variant:
- `access-token` → `databricksAuth.pat.token` (authStrategy=service_role)
- `client-credentials` → `databricksAuth.clientCredentials.{clientId, clientSecret}` (authStrategy=service_role)
- `oauth-sso` → `databricksAuth.oauthU2m.{clientId, clientSecret}` (authStrategy=oauth_sso); prints the configured endpoint in the success note so self-hosted operators get the right callback URL
- `oauth-individual` → same `oauthU2m` variant with authStrategy=per_member_oauth; prints a per-member-lazy note
Wire label mismatch: OAuth modes populate `oauthU2m`, NOT `oauthSso` — the
server rejects the latter with `databricks.databricks_auth requires pat,
client_credentials, or oauth_u2m`. Captured in the types_databricks.go doc
block so future contributors don't rediscover it.
One _test.go file per leaf, mirroring the Snowflake layout: happy path (wire shape assertions + omitempty negative assertions), stdin secret read, empty-stdin usage error, read-error propagation, missing secret, missing flags, empty-string validation on every required ancestor flag, JSON bypass, render-write error, bad-flag rejection, Unary error, and remarshal error. Access-token adds a port-range validation test + a custom-port happy path; OAuth SSO adds a custom-endpoint echo test that locks in the self-hosted-endpoint behavior. Also expands TestCreateGroupHelpMentionsDialects to require `databricks` so future dialect adds trip the same guard. Keeps the 100% coverage gate on `./internal/...` green.
One test per auth mode, each env-gated:
- access-token: ANA_E2E_DBX_TOKEN + workspace env
- client-credentials: ANA_E2E_DBX_CLIENT_{ID,SECRET} + workspace env
- oauth-sso: ANA_E2E_DBX_OAUTH_CLIENT_{ID,SECRET} + workspace env (asserts the success note references ANA_E2E_ENDPOINT)
- oauth-individual: same OAuth env (asserts the per-member-lazy note)
Reuses the Snowflake suite's id-extraction + LIFO cleanup primitives
(extractConnectorID + RegisterConnectorCleanupByName) and gates every
test on a shared ANA_E2E_DBX_{HOST,HTTP_PATH,CATALOG,SCHEMA} env tuple.
README documents every env var; cli-readiness bumps the Databricks row
from Probed to Implemented across all four auth modes.
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Databricks dialect to Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "ana CLI"
participant Secret as "resolveSecret (stdin)"
participant Deps as "Deps (Unary client)"
participant Service as "Connector Service"
participant Output as "cli.RenderOutput"
CLI->>Secret: request token/client-secret when --*-stdin used
Secret-->>CLI: secret or error
CLI->>Deps: Unary(CreateConnector request with databricks spec)
Deps->>Service: gRPC/HTTP CreateConnector
Service-->>Deps: CreateConnector response (connectorId,...)
Deps-->>CLI: response
CLI->>Output: render JSON or human output
Output-->>CLI: write to stdout or return error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/connector_databricks_test.go`:
- Around line 122-160: The test TestConnectorCreateDatabricksOAuthSSO
redundantly calls os.Getenv("ANA_E2E_ENDPOINT") and asserts it is set even
though harness.Begin() already validates the env; remove lines that re-read and
assert ANA_E2E_ENDPOINT and instead use the harness to obtain the endpoint
(either call a new getter H.Endpoint() you add to the harness type or reference
an existing accessor) and compare stdout against that value; if you choose the
getter, add func (h *H) Endpoint() string that returns the harness-stored
endpoint and update the comparison to use h.Endpoint() rather than os.Getenv.
In `@internal/cli/cli_test.go`:
- Around line 1044-1085: Update the three tests
(TestDeclareIntGuardsAgainstRedeclare, TestDeclareIntFreshDeclaration,
TestDeclareIntDefault) to invoke the CLI parser helper ParseFlags instead of
calling fs.Parse directly so the ErrUsage chain and CLI behavior are preserved;
replace calls like fs.Parse([]string{"--port","7"}) or fs.Parse(nil) with the
corresponding ParseFlags(fs, []string{...}) or ParseFlags(fs, nil) invocation
and handle the returned error exactly as the tests currently do.
In `@internal/connector/create_databricks_oauth_individual.go`:
- Around line 54-64: Remove the redundant empty-string check for the client ID
in the oauth-individual command: the explicit if c.clientID == "" { ... } block
duplicates validation already performed by cli.RequireFlags in the connector
create databricks oauth-individual flow, so delete that conditional and its
UsageErrf call and rely on cli.RequireFlags (and existing
requireDatabricksCommon) to validate --client-id; locate the check by the symbol
c.clientID and the surrounding function handling the oauth-individual create
path.
In `@internal/connector/create_databricks_oauth_sso.go`:
- Around line 59-69: Remove the redundant empty-string validation for
--client-id: the call to cli.RequireFlags in the connector create databricks
oauth-sso path already enforces presence of "client-id", so delete the block
that checks if c.clientID == "" and returns cli.UsageErrf; ensure
requireDatabricksCommon and other flag checks (requireDatabricksCommon,
cli.RequireFlags) remain unchanged — if you intended to enforce non-empty values
beyond presence, apply the same explicit check to other leaves (e.g.,
client-credentials, oauth-individual) consistently instead of only for
c.clientID.
🪄 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: da464d12-b7a9-4a27-a1df-deb61a4d9a1a
⛔ Files ignored due to path filters (5)
docs/cli-readiness.mdis excluded by!**/docs/**,!**/*.mde2e/CLAUDE.mdis excluded by!**/*.mde2e/README.mdis excluded by!**/*.mdinternal/cli/CLAUDE.mdis excluded by!**/*.mdinternal/connector/CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (16)
e2e/connector_databricks_test.gointernal/cli/cli_test.gointernal/cli/root.gointernal/connector/create.gointernal/connector/create_databricks.gointernal/connector/create_databricks_access_token.gointernal/connector/create_databricks_access_token_test.gointernal/connector/create_databricks_client_credentials.gointernal/connector/create_databricks_client_credentials_test.gointernal/connector/create_databricks_oauth_individual.gointernal/connector/create_databricks_oauth_individual_test.gointernal/connector/create_databricks_oauth_sso.gointernal/connector/create_databricks_oauth_sso_test.gointernal/connector/create_postgres_password_test.gointernal/connector/types.gointernal/connector/types_databricks.go
cli.RequireFlags only asserts explicit presence, so '--client-id ""' slips through — every Databricks leaf that takes --client-id needs an extra empty-string guard. Extract the inline check into a shared requireDatabricksClientID helper so the three leaves (client-credentials, oauth-sso, oauth-individual) apply identical validation and the intent is documented in one place.
The OAuth-SSO smoke tests in both the Snowflake and Databricks suites were re-fetching ANA_E2E_ENDPOINT via os.Getenv and asserting non-empty, but harness.Begin already validates the var and skips the test if it's unset. Add a thin H.Endpoint() accessor so tests that need the endpoint (to match it against CLI stdout) pull it from the harness rather than re-reading the environment.
stdlib flag.Parse re-wraps Set errors with %v, so the ErrUsage chain
that callers rely on only survives when parsing goes through
cli.ParseFlags. The six Declare{String,Bool,Int}* tests were calling
fs.Parse directly, bypassing the production path. Route them through
ParseFlags to match the rule already enforced in flags_test.go.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/connector_databricks_test.go`:
- Around line 159-192: Update TestConnectorCreateDatabricksOAuthIndividual to
perform the same post-create "connector get" verification as the other tests:
after calling extractConnectorID(t, stdout) and h.RegisterConnectorCleanup(id)
(and only if not h.DryRun()), run the CLI "connector get <id>" via h.Run (or
h.RunStdin if needed), check for errors, and assert the returned stdout contains
"connectorType: DATABRICKS" and the "lazily at first query" per-member-lazy
note; reuse the same helpers (extractConnectorID, h.Run/h.RunStdin) and mirror
the existing assertion text used earlier in the test for consistency.
- Around line 91-120: Add a post-create verification call to run "connector get"
for the created connector ID in TestConnectorCreateDatabricksClientCredentials:
after calling extractConnectorID(t, stdout) and h.RegisterConnectorCleanup(id),
invoke the harness to execute the equivalent of "connector get <id>" (using the
harness Run/RunStdin helper like h.Run or h.RunStdin) and assert the returned
stdout contains "connectorType: DATABRICKS" (same check used earlier in the
access-token test), failing the test if the verification fails.
🪄 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: 4428356f-fc9a-4b61-a9f1-85e58ddb63d0
📒 Files selected for processing (8)
e2e/connector_databricks_test.goe2e/connector_snowflake_test.goe2e/harness/harness.gointernal/cli/cli_test.gointernal/connector/create_databricks.gointernal/connector/create_databricks_client_credentials.gointernal/connector/create_databricks_oauth_individual.gointernal/connector/create_databricks_oauth_sso.go
Databricks client-credentials and oauth-individual were missing the post-create `connector get` verification that access-token already had. Same drift had crept into Snowflake keypair/oauth-sso/oauth-individual. Add the get round-trip to every create-leaf smoke so the server read-back is uniformly asserted.
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 `@e2e/connector_databricks_test.go`:
- Around line 61-201: Tests TestConnectorCreateDatabricksAccessToken,
TestConnectorCreateDatabricksClientCredentials,
TestConnectorCreateDatabricksOAuthSSO and
TestConnectorCreateDatabricksOAuthIndividual duplicate the
create/run/dry-run/id/cleanup/get pattern; extract a helper (e.g.,
createAndVerifyDatabricksConnector) that accepts the harness, command args (or
leaf name), stdin content, expected stdout fragments and returns the created id
(or error), reusing databricksCommonArgs and extractConnectorID; replace each
test body to build args, call the helper, register cleanup by name before helper
if needed, and assert leaf-specific stdout fragments (like "connectorType:
DATABRICKS" or "complete OAuth at "+h.Endpoint()) outside or via expected
fragments passed into the helper.
🪄 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: c3bbd1d3-7ce0-4b85-90e0-b41bdc9e647d
📒 Files selected for processing (2)
e2e/connector_databricks_test.goe2e/connector_snowflake_test.go
… + Databricks The create/dry-run/id/cleanup/connectorType/get round-trip was copy-pasted across all 8 Snowflake + Databricks create-leaf smokes, and the parity gap that motivated 0f50b55 (missing `connector get` on 5 of 8 leaves) could only recur because each test hand-rolled the sequence. Hoist the pattern into connectorCreateLeaf{Name,Args,Stdin,ConnectorType, Extra}.Run — the only way to skip the `get` round-trip now is to bypass the helper on purpose. Also moves extractConnectorID (and its shared regex) into the new dialect-neutral helpers file so it no longer lives awkwardly next to Snowflake-only code. Leaf-specific stdout assertions (OAuth endpoint note, per-member-lazy note) ride along on the `Extra` hook so each test stays a single declarative block.
This was referenced Apr 22, 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
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
access-token,client-credentials,oauth-sso,oauth-individual) matching the wire shapes captured inapi-catalog/POST_connector.create.databricks.*.json../internal/...green.cli.DeclareInt(Lookup-guardedfs.IntVar) so the Databricks Group can inherit-declare the shared--portflag.Pattern applied
One dialect Group with inheritable workspace flags (
--name --host --http-path --port --catalog --schema) plus four leaves, each populating a nesteddatabricksAuthone-of variant:authStrategydatabricksAuthvariantaccess-tokenservice_rolepat.tokenclient-credentialsservice_roleclientCredentials.{clientId, clientSecret}oauth-ssooauth_ssooauthU2m.{clientId, clientSecret}oauth-individualper_member_oauthoauthU2m.{clientId, clientSecret}Wire gotcha captured in types_databricks.go: OAuth modes populate
oauthU2m, NOToauthSso— the server rejects the latter. SSO vs Individual is expressed via envelope-levelauthStrategy, not the nested one-of.Summary by CodeRabbit
New Features
Tests
Other