[BB-1883] Containerize connector #35
Conversation
FeliLucero1
requested review from
a team,
JavierCarnelli-ConductorOne,
agustin-conductor,
aldevv,
btipling,
johnallers,
luisina-santos,
mateoHernandez123,
mateovespConductor and
sergiocorral-conductorone
WalkthroughReplaces Viper-based config with a generated typed Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant NewC as NewConnector
participant Val as ValidateConfig
participant Auth as prepareClientAuth
participant Host as getHostname
participant Builder as ConnectorBuilderV2
rect rgb(240,248,255)
Caller->>NewC: NewConnector(ctx, cfg, opts)
end
NewC->>Val: ValidateConfig(ctx, cfg)
Val-->>NewC: ok / error
alt valid
NewC->>Auth: prepareClientAuth(cfg)
Auth-->>NewC: auth method + client (basic | oauth | token | none)
NewC->>Host: getHostname(cfg)
Host-->>NewC: hostname, account-hostname
NewC->>Builder: build ConnectorBuilderV2(auth, hostname, opts)
Builder-->>Caller: ConnectorBuilderV2 (+ opts)
else invalid
NewC-->>Caller: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)pkg/config/schema.go (2)
🔇 Additional comments (5)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go.mod (2)
3-3: Go version 1.25.2 does not exist.The current Go stable releases are in the 1.22/1.23 range. Version 1.25.2 is not a released Go version. This will cause build failures.
🔎 Proposed fix
-go 1.25.2 +go 1.23.0
116-116: Update to a stable release of gRPC.v1.73.0 is a stable release available for use, and v1.77.0 is the current stable version referenced in official documentation. The current
v1.73.0-devis a pre-release development version that may contain unstable changes. Use a stable release instead to ensure production stability.
🧹 Nitpick comments (3)
pkg/config/conf.gen.go (1)
32-42: Accessor methods panic on type mismatch instead of returning errors.The generated
Get*methods panic when encountering unexpected types. This could cause runtime crashes if the configuration is malformed or if these methods are called with incorrect field names. Consider modifying the generator to return(T, error)tuples instead.Additionally,
GetInt,GetBool, andGetStringMapare generated but theDatabricksstruct has no fields of those types—these are effectively dead code.Since this is generated code, the fix should be applied to the generator at
pkg/config/gen. For example, a safer pattern:func (c *Databricks) GetString(fieldName string) (string, error) { v, ok := c.findFieldByTag(fieldName) if !ok { return "", fmt.Errorf("field %q not found", fieldName) } if t, ok := v.(string); ok { return t, nil } return "", fmt.Errorf("field %q is not a string", fieldName) }Also applies to: 44-56, 58-68, 70-80, 82-92
pkg/connector/connector.go (2)
259-259: Unused variableaccountHostnameinprepareClientAuth.
accountHostnameis computed on line 259 but only used in the OAuth case. Consider moving this computation inside the OAuth case to avoid unnecessary work for other auth methods.🔎 Proposed fix
func prepareClientAuth(_ context.Context, cfg *config.Databricks, l *zap.Logger) databricks.Auth { accountID := cfg.AccountId databricksClientId := cfg.DatabricksClientId databricksClientSecret := cfg.DatabricksClientSecret username := cfg.Username password := cfg.Password workspaces := cfg.Workspaces tokens := cfg.WorkspaceTokens - accountHostname := databricks.GetAccountHostname(getHostname(cfg)) switch { case username != "" && password != "": // ... basic auth case case databricksClientId != "" && databricksClientSecret != "": + accountHostname := databricks.GetAccountHostname(getHostname(cfg)) l.Info( "using oauth", zap.String("account-id", accountID),
301-307: Hostname default is duplicated.The default hostname
"cloud.databricks.com"is defined here and also inschema.go(line 19 viaWithDefaultValue). Consider using a shared constant to avoid drift.🔎 Proposed fix
In
pkg/config/schema.go, export the constant:const DefaultHostname = "cloud.databricks.com"Then in
connector.go:func getHostname(cfg *config.Databricks) string { - const defaultHost = "cloud.databricks.com" if cfg.Hostname == "" { - return defaultHost + return config.DefaultHostname } return cfg.Hostname }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (48)
.github/workflows/capabilities_and_config.yamlis excluded by none and included by none.github/workflows/ci.yamlis excluded by none and included by none.github/workflows/main.yamlis excluded by none and included by none.github/workflows/release.yamlis excluded by none and included by noneMakefileis excluded by none and included by nonecmd/baton-databricks/config/schema_test.gois excluded by none and included by nonecmd/baton-databricks/main.gois excluded by none and included by noneconf.gen.gois excluded by none and included by nonepkg/config/gen/gen.gois excluded by!**/gen/**and included bypkg/**vendor/github.com/conductorone/baton-sdk/pkg/test/configSchema.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/test/integration.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/test/pagination.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/test/ratelimit.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/test/validation.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypass.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypasssafe.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/common.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/config.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/doc.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/dump.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/format.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/spew.gois excluded by!vendor/**and included by nonevendor/github.com/pmezard/go-difflib/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/pmezard/go-difflib/difflib/difflib.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/doc.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/errors.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/doc.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require_forward.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/requirements.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (4)
go.modpkg/config/conf.gen.gopkg/config/schema.gopkg/connector/connector.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/config/conf.gen.go (1)
pkg/connector/connector.go (1)
Databricks(19-22)
🔇 Additional comments (9)
go.mod (1)
87-88: Dependency scope changes look appropriate.Moving
viperto indirect and addingobjxaligns with the transition from Viper-based configuration to the newDatabricksstruct. The changes reflect that Viper is now only a transitive dependency.pkg/config/schema.go (4)
28-28: Good addition of secret flags for sensitive fields.Marking
DatabricksClientSecretField,PasswordField, andTokensFieldas secrets ensures proper handling of sensitive credentials throughout the configuration lifecycle.Also applies to: 37-37, 46-46
82-120: Field groups are well-structured.The authentication method groupings provide clear organization with descriptive
HelpTextfor each flow (OAuth, PAT, username/password). Setting OAuth as the default group is a sensible choice.
131-144: ValidateConfig signature and logic look correct.The migration from
*viper.Viperto*Databricksaligns with the new typed configuration approach. The validation logic correctly ensures workspace/token count parity.
123-128: Generator directory and entry point verified.The
./gendirectory exists inpkg/config/with a validgen.goentry point (containingpackage mainandfunc main()). The generator successfully producesconf.gen.goas expected.pkg/config/conf.gen.go (2)
17-30: Reflection-based field lookup is acceptable for configuration access.While reflection adds overhead, configuration access is infrequent enough that this shouldn't cause performance issues. The implementation correctly handles the pointer dereferencing and tag matching.
6-15: Struct fields align with schema definition.The
Databricksstruct fields and theirmapstructuretags correctly correspond to the field definitions inschema.go.pkg/connector/connector.go (2)
43-68: V1 to V2 adapter pattern is well-implemented.The adapter correctly delegates to the underlying V1 syncer and translates pagination tokens to the V2
SyncOpResultsstructure. This provides a clean migration path without rewriting all resource syncers.
217-244:NewConnectororchestration looks correct.The function properly validates configuration, derives hostnames, prepares authentication, and constructs the connector. The error handling with logging is appropriate.
One minor note: returning
nilfor the[]connectorbuilder.Opt(line 243) is fine if no additional options are needed, but consider adding a brief comment explaining this for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/connector/connector.go (2)
246-249: Consider consolidating validation logic.The condition
len(workspaces) == len(tokens)is also checked inconfig.ValidateConfig(pkg/config/schema.go:130-144). While this duplication is harmless sinceValidateConfigis called first inNewConnector, consolidating the logic would reduce maintenance burden.
296-304: Authentication warning addresses past review concern, but consider returning an error.The warning log added here addresses the past review comment about silently falling back to
NoAuth. The detailed message will help users diagnose configuration issues.However, for a fail-fast approach, consider returning an error instead of proceeding with
NoAuth, since API calls will inevitably fail without valid credentials. This would provide immediate feedback during connector initialization rather than obscure failures during sync operations.Based on learnings, a past review suggested returning an error or logging a warning when no valid auth method is detected. The warning has been added, but an error would be more robust.
🔎 Optional: Return error instead of NoAuth
If you decide to return an error, you'll need to update the function signature:
-func prepareClientAuth(_ context.Context, cfg *config.Databricks, l *zap.Logger) databricks.Auth { +func prepareClientAuth(_ context.Context, cfg *config.Databricks, l *zap.Logger) (databricks.Auth, error) {And update the default case:
default: - l.Warn( - "no valid authentication method detected, falling back to NoAuth. " + - "This will likely cause API calls to fail. " + - "Please configure one of: OAuth (client-id + client-secret), " + - "username/password, or personal access token (workspace-tokens + workspaces)", - ) - return &databricks.NoAuth{} + return nil, fmt.Errorf( + "no valid authentication method detected. " + + "Please configure one of: OAuth (client-id + client-secret), " + + "username/password, or personal access token (workspace-tokens + workspaces)", + )Then update the caller in
NewConnectorto handle the error.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/connector.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/connector.go (4)
pkg/databricks/client.go (2)
Client(36-46)GetAccountHostname(48-55)pkg/config/conf.gen.go (1)
Databricks(6-15)pkg/config/schema.go (1)
ValidateConfig(131-145)pkg/databricks/auth.go (5)
Auth(14-17)NewBasicAuth(72-77)NewOAuth2(96-105)NewTokenAuth(37-48)NoAuth(19-19)
🔇 Additional comments (4)
pkg/connector/connector.go (4)
24-41: LGTM! Clean adapter pattern for V1→V2 migration.The adapter approach allows incremental migration to ResourceSyncerV2 without modifying the underlying syncer implementations.
43-68: LGTM! Adapter correctly translates V1 to V2 interface.The adapter properly bundles
pageTokenandannosintoSyncOpResultsfor the V2 interface.
307-313: LGTM! Clean hostname resolution with sensible default.The default hostname
cloud.databricks.comis appropriate for Databricks cloud deployments.
217-244: Address unused return value and redundant error logging in NewConnector.Two issues to resolve:
The second return value (
[]connectorbuilder.Opt) is alwaysniland never populated. Either populate it based on theopts *cli.ConnectorOptsparameter (if options should be returned), or remove it from the signature to match standard connector patterns in the Baton ecosystem.Line 239 logs the error before returning it, potentially causing duplicate logging if the caller also logs the error. Remove the log statement and let the caller handle error logging.
pkg/connector/connector.go
Outdated
| } | ||
|
|
||
| hostname := getHostname(cfg) | ||
| accountHostname := databricks.GetAccountHostname(hostname) |
There was a problem hiding this comment.
so the C1 side of this has two configs for host name, hostname and accountHostname and only call this when when accountHostname was the empty string. Now we are always doing this and ignoring the accountHostname configuration. Is there a reason we changed this?
https://github.com/ductone/c1/blob/main/pkg/connector/databricks/databricks.go
There was a problem hiding this comment.
From baton-databricks, we only handle hostname and calculate accountHostname from there when the schema is used directly (via NewConnector()). C1 does not use NewConnector(); it calls New() directly, passing hostname and accountHostname as parameters. Therefore, C1 controls accountHostname, and baton-databricks does not overwrite it. Am I right?
| AccountIdField, | ||
| DatabricksClientIdField, | ||
| DatabricksClientSecretField, | ||
| HostnameField, |
There was a problem hiding this comment.
this is missing the AccountHostnameField. Please see the other comment and confirm if this is correct.
https://github.com/ductone/c1/blob/main/pkg/builtin_connectors/databricks.go#L48
There was a problem hiding this comment.
I'm not seeing that field here https://github.com/ConductorOne/baton-databricks/blob/main/cmd/baton-databricks/config/schema.go , do I need to add it?
conf.gen.go
Outdated
| @@ -0,0 +1,92 @@ | |||
| // Code generated by baton-sdk. DO NOT EDIT!!! | |||
| package config | |||
There was a problem hiding this comment.
I don't think we need this file here, we have this in config package
| DatabricksClientSecretField = field.StringField( | ||
| "databricks-client-secret", | ||
| field.WithDescription("The Databricks service principal's client secret used to connect to the Databricks Account and Workspace API"), | ||
| field.WithIsSecret(true), |
There was a problem hiding this comment.
we need the display name on these fields, we use these to populate the field label in the UI
| field.WithConstraints(fieldRelationships...), | ||
| field.WithFieldGroups(fieldGroups), |
There was a problem hiding this comment.
need app icon, docs link, connector name. we also use these to populate UI
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/connector/connector.go (1)
296-304: NoAuth fallback now logs a detailed warning - addresses previous review.The warning message clearly explains that no valid auth method was detected and lists the available options. This addresses the earlier feedback about silently masking configuration errors.
🧹 Nitpick comments (2)
pkg/config/conf.gen.go (1)
33-43: Panics on type mismatches could crash the application in production.The accessor methods (
GetStringSlice,GetInt,GetBool,GetStringMap) panic when encountering type mismatches. While this is generated code, panics in production are risky as they can crash the entire application rather than allowing graceful error handling.Additionally,
GetInt,GetBool, andGetStringMapappear unused since no fields of those types exist in the struct.Consider updating the generator to either:
- Return errors instead of panicking, or
- Only generate accessors for field types that actually exist in the struct
Also applies to: 59-69, 71-81, 83-93
pkg/connector/connector.go (1)
251-259: Duplicate hostname computation inprepareClientAuth.Line 259 recomputes
accountHostnameviagetAccountHostname(cfg, getHostname(cfg)), but this is already computed inNewConnectorat line 227-228. Consider passingaccountHostnameas a parameter to avoid redundant computation.🔎 Proposed refactor
-func prepareClientAuth(_ context.Context, cfg *config.Databricks, l *zap.Logger) databricks.Auth { +func prepareClientAuth(_ context.Context, cfg *config.Databricks, accountHostname string, l *zap.Logger) databricks.Auth { accountID := cfg.AccountId databricksClientId := cfg.DatabricksClientId databricksClientSecret := cfg.DatabricksClientSecret username := cfg.Username password := cfg.Password workspaces := cfg.Workspaces tokens := cfg.WorkspaceTokens - accountHostname := getAccountHostname(cfg, getHostname(cfg))Then update the call site in
NewConnector:- auth := prepareClientAuth(ctx, cfg, l) + auth := prepareClientAuth(ctx, cfg, accountHostname, l)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/config/conf.gen.gopkg/config/schema.gopkg/connector/connector.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/config/conf.gen.go (1)
pkg/connector/connector.go (1)
Databricks(19-22)
pkg/connector/connector.go (3)
pkg/databricks/client.go (2)
Client(36-46)GetAccountHostname(48-55)pkg/config/schema.go (1)
ValidateConfig(151-165)pkg/databricks/auth.go (5)
Auth(14-17)NewBasicAuth(72-77)NewOAuth2(96-105)NewTokenAuth(37-48)NoAuth(19-19)
pkg/config/schema.go (3)
pkg/databricks/client.go (1)
Name(640-643)pkg/config/conf.gen.go (1)
Databricks(6-16)pkg/connector/connector.go (1)
Databricks(19-22)
🔇 Additional comments (12)
pkg/config/conf.gen.go (2)
1-4: This is generated code - issues should be addressed in the generator.The file header correctly indicates this is auto-generated. Any structural issues noted below should be addressed in the
baton-sdkgenerator rather than in this file directly.
6-16: Struct definition aligns with schema fields.The
Databricksstruct correctly mirrors the configuration fields defined inschema.gowith appropriatemapstructuretags for config binding.pkg/config/schema.go (5)
11-15:AccountHostnameFieldadded - addresses previous review comment.The field is now properly defined with description and display name, resolving the earlier feedback about the missing
AccountHostnameField.
36-37: Sensitive fields properly marked as secrets with display names.
DatabricksClientSecretField,PasswordField, andTokensFieldare correctly configured withWithIsSecret(true)and have display names added, addressing the earlier review feedback.Also applies to: 47-48, 58-59
96-137: Field groups provide clear authentication method organization.The three auth groups (
oauth_auth,personal_access_token_auth,username_password_auth) are well-structured with appropriate fields, help text, and default settings. Each group correctly includes the common fields (AccountIdField,HostnameField,AccountHostnameField) alongside method-specific fields.
140-148: Configuration variable with connector metadata - addresses previous review.The
Configvariable now includesWithConnectorDisplayName,WithHelpUrl, andWithIconUrlas requested in earlier feedback. Thego:generatedirective enables code generation for the config struct.
151-164:ValidateConfigsignature updated to use typed*Databricksstruct.The function now accepts
*Databricksinstead of*viper.Viper, providing type safety and direct field access. The validation logic for workspace/token count matching remains correct.pkg/connector/connector.go (5)
24-41: V1 to V2 adapter pattern is clean and correct.The
ResourceSyncersmethod properly creates V1 syncers and wraps them with the adapter. This approach allows gradual migration to the V2 interface while reusing existing syncer implementations.
43-68: Adapter correctly translates V1 syncer outputs to V2SyncOpResults.The
resourceSyncerV1toV2Adapterproperly maps the V1 tuple returns(items, pageToken, annos, err)to the V2 structure([]*Type, *SyncOpResults, error). The implementation handles all three operations:List,Entitlements, andGrants.
217-244:NewConnectorprovides a clean typed entry point.The function validates config, resolves hostnames, prepares authentication, and constructs the connector. Error handling is appropriate with logging.
One minor observation: returning
nilfor the options slice (line 243) is fine, but consider[]connectorbuilder.Opt{}for consistency if callers might iterate without nil checks.
307-313: Default hostname matches schema definition.The
defaultHostconstant"cloud.databricks.com"correctly matches theWithDefaultValueset onHostnameFieldinschema.go(line 25).
315-321:getAccountHostnamecorrectly prioritizes config value over computed value.The function returns the configured
AccountHostnameif set, otherwise delegates todatabricks.GetAccountHostname()for cloud-specific derivation (Azure, GCP, or default). This aligns with the discussion in past reviews about C1 controllingaccountHostnamedirectly.
pkg/config/schema.go
Outdated
| fieldGroups = []field.SchemaFieldGroup{ | ||
| { | ||
| Name: "oauth_auth", | ||
| DisplayName: "OAuth Authentication", |
There was a problem hiding this comment.
please use the same name as in c1.
pkg/config/schema.go
Outdated
| }, | ||
| { | ||
| Name: "personal_access_token_auth", | ||
| DisplayName: "Personal Access Token Authentication", |
pkg/config/schema.go
Outdated
| }, | ||
| { | ||
| Name: "username_password_auth", | ||
| DisplayName: "Username/Password Authentication", |
There was a problem hiding this comment.
I think it's done, could you please check it?
pkg/config/schema.go
Outdated
| ) | ||
| fieldGroups = []field.SchemaFieldGroup{ | ||
| { | ||
| Name: "OAuth", |
There was a problem hiding this comment.
name should be in this format, similar to field names
example:
https://github.com/ConductorOne/baton-github-test/blob/main/pkg/config/config.go#L89
There was a problem hiding this comment.
I think it's done, could you please check it?
There was a problem hiding this comment.
looks good, thank you
in c1, we will have to map the field group names, similar to fields
https://github.com/ductone/c1/blob/badc8d4244af453766a7d0b90486fc577a7e34a9/pkg/builtin_connectors/builtin_deployment.go#L363
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.