Migrate to DefineConfigurationV2 / RunConnector (prerequisite for CE-1183) - #33
Migrate to DefineConfigurationV2 / RunConnector (prerequisite for CE-1183)#33laurenleach wants to merge 3 commits into
Conversation
Prerequisite plumbing only — no guard logic, no behaviour change.
This connector was still on the V1 entrypoint, which means *cli.ConnectorOpts
never reaches connector.New. That is what blocks reading the sync resource-type
filter (CE-1183), so the migration is split out here on its own.
- cmd: config.DefineConfiguration + getConnector + cmd.Execute() ->
config.RunConnector, with WithProvisioningEnabled (this connector implements
Grant/Revoke) and WithDefaultCapabilitiesConnectorBuilderV2 so a zero-value
Connector{} serves the capability set.
- pkg/config: `Config` -> `Configuration`, adding the WithConstraints wiring
that FieldRelationships already declared but never passed. Regenerated
conf.gen.go; the GetString change is the current generator's output for the
pinned SDK, which also accepts []byte.
- connector.New: now returns (connectorbuilder.ConnectorBuilderV2,
[]connectorbuilder.Opt, error) and takes *cli.ConnectorOpts. The opts
parameter is accepted but deliberately unread — wiring it up is the follow-up.
field.Validate moved in from the deleted getConnector so validation is not
lost.
- ResourceSyncers returns []connectorbuilder.ResourceSyncerV2, and both builders
move to the V2 sync signatures: *pagination.Token -> resourceSdk.SyncOpAttrs,
and (…, string, annotations.Annotations, error) -> (…, *SyncOpResults, error).
Grant/Revoke are unchanged; V2 only alters the three sync methods.
- Tests updated for the new signatures.
baton_capabilities.json and config_schema.json regenerate byte-identical, which
is the check that this is pure plumbing. golangci-lint reports the same 8
pre-existing staticcheck issues as main (the deprecated trait/actions APIs) and
no new ones.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| opts, err := getPageOptions(&attrs.PageToken, client.ItemsPerPage) | ||
| if err != nil { | ||
| return nil, "", nil, err | ||
| return nil, nil, err |
There was a problem hiding this comment.
🟡 Suggestion: This is the only path in List that returns a nil *SyncOpResults; every other return builds one (&SyncOpResults{Annotations: ann}). The SDK nil-guards this (resource_syncer.go sets retOptions = &SyncOpResults{}), so it is not a runtime panic, but the new tests dereference the results value unconditionally (e.g. syncResults.NextPageToken in users_test.go:38), so a test covering an invalid page token would panic here. Returning &resourceSdk.SyncOpResults{} keeps the contract uniform.
| func New(ctx context.Context, config *cfg.Metabase, _ *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) { | ||
| l := ctxzap.Extract(ctx) | ||
|
|
||
| if err := field.Validate(cfg.Configuration, config); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: RunConnector already validates the config before it calls New — pkg/cli/commands.go runs field.Validate(confschema, t, field.WithAuthMethod(v.GetString("auth-method"))) on every path (run, server, capabilities). This call repeats that check but omits field.WithAuthMethod, so if auth methods are ever added to Configuration this copy would reject configs the SDK accepted. Harmless today (no auth methods, FieldRelationships is empty); consider dropping it or passing the auth method through from opts.SelectedAuthMethod.
Connector PR Review: Migrate to DefineConfigurationV2 / RunConnector (prerequisite for CE-1183)Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe only new commit ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Review feedback: baton-sdk already validates the configuration as part of RunConnector, so calling field.Validate inside New duplicates it. The call was carried over from the deleted V1 getConnector, where it was the connector's own responsibility; under V2 it is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback. The V1 DefineConfiguration call passed no connectorrunner options at all, so adding WithProvisioningEnabled() here would have flipped provisioning on unconditionally (it sets cfg.provisioningEnabled = true) rather than leaving it under whatever gates it before. That is a behaviour change, and this PR is meant to be pure entrypoint plumbing. Implementing Grant/Revoke/CreateAccount is not the same thing as the runner being told to enable provisioning, which is the mistake behind the original change. WithDefaultCapabilitiesConnectorBuilderV2 stays: that is what lets `capabilities` run without credentials. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Prerequisite for CE-1183. Plumbing only — this PR deliberately contains no guard logic.
baton-metabasewas still on the V1 entrypoint, so*cli.ConnectorOptsnever reachedconnector.New. That is precisely what blocks reading the sync resource-type filter, so the migration is split out on its own to keep the guard diff reviewable.What changed
cmd/baton-metabaseDefineConfiguration+getConnector+cmd.Execute()→config.RunConnectorpkg/configConfig→Configuration; regeneratedconf.gen.goconnector.New*cli.ConnectorOpts, returns(ConnectorBuilderV2, []connectorbuilder.Opt, error)ResourceSyncers[]connectorbuilder.ResourceSyncer→[]connectorbuilder.ResourceSyncerV2userBuilder,groupBuilder*pagination.Token→resourceSdk.SyncOpAttrs;(…, string, annotations.Annotations, error)→(…, *SyncOpResults, error)Grant/Revokeare untouched — V2 only changes the three sync methods.Notes for review
optsis accepted but deliberately unread. Wiring it into the builders is the CE-1183 follow-up; adding it here would be the guard change this PR is meant to exclude.field.Validatemoved intoNewfrom the deletedgetConnector, so config validation is not silently lost.RunConnectorgetsWithProvisioningEnabled()(this connector implementsGrant/Revoke) andWithDefaultCapabilitiesConnectorBuilderV2(&connector.Connector{}), so a zero-valueConnector{}serves the capability set.WithConstraints(FieldRelationships...)is now actually passed —FieldRelationshipswas declared but never wired in under V1.conf.gen.goGetStringchange is the current generator's output for the pinned SDK (it now also accepts[]byte), not a hand edit.Verification
baton_capabilities.jsonandconfig_schema.jsonregenerate byte-identical — the strongest available signal that this is pure plumbing with no behaviour change.go build,go vet,go test ./...all pass.golangci-lintreports the same 8 pre-existingstaticcheckissues asmain(deprecated trait and actions APIs, called out in CE-1183) and adds none.Unblocks CE-1183, and in turn CE-1209 (
baton-metabase-v049vendors this connector).🤖 Generated with Claude Code