[DUCT-12948] Add Option for default connector builder #581
Conversation
WalkthroughThe PR introduces capability discovery mechanisms for CLI commands by exposing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🧹 Nitpick comments (2)
pkg/cli/commands.go (2)
633-638: UserunCtxinstead ofctxfor consistency.Other connector operations in this function use
runCtx(see line 641), which includes the initialized logger context. Usingctxhere is inconsistent.if defaultV, ok := any(t).(field.ConfigurableDefault); ok { - c, err = connectorbuilder.NewConnector(ctx, defaultV.DefaultBuilder()) + c, err = connectorbuilder.NewConnector(runCtx, defaultV.DefaultBuilder()) if err != nil { return err } }
647-649: Improve error message when connector creation fails without an error.If both branches (lines 634 and 641) succeed but return nil connectors (unlikely but possible), wrapping a nil
errproduces a confusing message like "could not create connector %!w()".if c == nil { - return fmt.Errorf("could not create connector %w", err) + return fmt.Errorf("could not create connector") }Alternatively, you could use
errors.Newsince there's no error to wrap at this point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/cli/commands.go(3 hunks)pkg/config/generate.go(6 hunks)pkg/connectorbuilder/connectorbuilder.go(2 hunks)pkg/connectorbuilder/connectorbuilder_test.go(12 hunks)pkg/field/validation.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorbuilder/connectorbuilder_test.gopkg/config/generate.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorbuilder/connectorbuilder_test.gopkg/config/generate.go
📚 Learning: 2025-02-11T21:02:21.506Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 299
File: pkg/config/generate.go:0-0
Timestamp: 2025-02-11T21:02:21.506Z
Learning: The getter methods in config structs (GetString, GetInt, GetBool, GetStringSlice) must match Viper's interface signatures and cannot be changed to return errors.
Applied to files:
pkg/config/generate.go
🧬 Code graph analysis (3)
pkg/connectorbuilder/connectorbuilder.go (2)
pb/c1/connector/v2/connector.pb.go (2)
ConnectorCapabilities(739-746)ConnectorCapabilities(759-759)pb/c1/connector/v2/connector_protoopaque.pb.go (2)
ConnectorCapabilities(745-752)ConnectorCapabilities(765-765)
pkg/cli/commands.go (5)
pkg/types/types.go (1)
ConnectorServer(11-27)pkg/field/validation.go (1)
ConfigurableDefault(331-334)pkg/connectorbuilder/connectorbuilder.go (1)
NewConnector(83-193)pkg/cli/cli.go (2)
NewConnector(40-40)RunTimeOpts(18-21)pb/c1/connector/v2/connector.pb.go (4)
ConnectorCapabilities(739-746)ConnectorCapabilities(759-759)ConnectorServiceGetMetadataRequest(1056-1060)ConnectorServiceGetMetadataRequest(1073-1073)
pkg/config/generate.go (2)
pkg/connectorbuilder/connectorbuilder.go (1)
ConnectorBuilder(49-53)pkg/field/struct.go (1)
Configuration(3-14)
🔇 Additional comments (12)
pkg/connectorbuilder/connectorbuilder_test.go (1)
1209-1213: LGTM!The test correctly uses the newly exported
GetCapabilitiesmethod and the comment is appropriately updated to reflect the method name capitalization change.pkg/field/validation.go (1)
331-334: LGTM!Clean interface definition that properly extends
Configurablewith theDefaultBuilder() anymethod. This enables the CLI flow to check for and use default builders when available.pkg/connectorbuilder/connectorbuilder.go (2)
260-260: LGTM!The call site is correctly updated to use the newly exported method name.
325-326: LGTM!The method is now properly exported to support direct capability discovery from the CLI flow. The comment is appropriately updated.
pkg/cli/commands.go (3)
12-13: LGTM!New imports for
connectorbuilderandtypespackages are appropriately added to support the new connector resolution flow.
651-675: LGTM!The capability discovery flow correctly:
- Defines a local
getterinterface for theGetCapabilitiesmethod- Attempts direct capability retrieval if the connector implements the interface
- Falls back to metadata-based capabilities when
GetCapabilitiesisn't available- Properly validates that capabilities exist before proceeding
683-683: LGTM!Correctly marshals the
capabilitiesvariable obtained from either the directGetCapabilitiescall or the metadata fallback path.pkg/config/generate.go (5)
56-68: LGTM!The reflection logic correctly:
- Creates a zero-value of type T
- Handles pointer types by dereferencing
- Extracts the type name and package path for code generation
The
Type.String()returns the package-qualified name (e.g.,connector.MyBuilder) whileType.PkgPath()returns the full import path, which is the correct combination for generating valid import statements and type references.
128-131: LGTM!The
DefaultBuilder()method correctly implements thefield.ConfigurableDefaultinterface, returning a pointer to a zero-value of the builder type. This enables the CLI flow to instantiate the default builder when no configuration is provided.
85-85: No action needed — FileUpload support is not affected by this change.FileUpload fields use
StringVariantand are paired withFileUploadDecodeHook, which handles the string-to-[]byteconversion at configuration time. This line correctly mapsStringVariantto"string"for code generation. The[]byteconversion logic remains intact in the decode hook.
165-169: The panic behavior is correct and required by Viper's interface constraints.The
GetStringmethod must match Viper's interface signature (func GetString(key string) string), which cannot return errors. Given this constraint, panicking on type mismatch is the only way to handle errors. The strict type checking is intentional: the generated config struct fields should be properly typed during initialization, so a type mismatch indicates a configuration setup error that should fail fast. This implementation is consistent across all getter methods (GetInt,GetBool,GetStringSlice,GetStringMap).
25-25: Note: Breaking API change.The
Generatefunction signature changed to require a type parameter[T connectorbuilder.ConnectorBuilder]. This is a breaking change as noted in the PR description. Any external callers will need to update their usage to provide the type parameter.No existing callers of
Generatewere found in the codebase, suggesting this is likely a newly added public API function.
pkg/config/generate.go
Outdated
| import ( | ||
| "reflect" | ||
| "{{ .DefaultBuilderPkgPath }}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the generate.go file around the mentioned lines
cat -n pkg/config/generate.go | head -130 | tail -30Repository: ConductorOne/baton-sdk
Length of output: 871
🏁 Script executed:
# Search for DefaultBuilderPkgPath definition and usage
rg -n 'DefaultBuilderPkgPath' --type goRepository: ConductorOne/baton-sdk
Length of output: 255
🏁 Script executed:
# Search for the Generate function and how the template is used
rg -n 'type.*Generate|func.*Generate' pkg/config/ --type go -A 5Repository: ConductorOne/baton-sdk
Length of output: 447
🏁 Script executed:
# Read the complete Generate function
sed -n '25,90p' pkg/config/generate.goRepository: ConductorOne/baton-sdk
Length of output: 1842
🏁 Script executed:
# Search for builderType definition
rg -n 'builderType' pkg/config/generate.go -B 2 -A 2Repository: ConductorOne/baton-sdk
Length of output: 418
🏁 Script executed:
# Find ConnectorBuilder interface definition
rg -n 'type ConnectorBuilder' --type go -A 5Repository: ConductorOne/baton-sdk
Length of output: 853
🏁 Script executed:
# Check if there are any examples or tests showing local builders
rg -n 'DefaultBuilderPkgPath' --type go -B 5 -A 5Repository: ConductorOne/baton-sdk
Length of output: 1807
🏁 Script executed:
# Search for tests that might show when PkgPath could be empty
fd -t f -e 'go' | xargs grep -l "Generate\[" | head -5Repository: ConductorOne/baton-sdk
Length of output: 88
🏁 Script executed:
# Look for test files using Generate
find . -name '*_test.go' -type f | xargs grep -l 'Generate\[' 2>/dev/nullRepository: ConductorOne/baton-sdk
Length of output: 50
🏁 Script executed:
# Check if there are any examples or docs about Generate usage
rg -n 'Generate\[' --type go -B 3 -A 3Repository: ConductorOne/baton-sdk
Length of output: 451
🏁 Script executed:
# Look at pkg/config directory structure
ls -la pkg/config/Repository: ConductorOne/baton-sdk
Length of output: 301
🏁 Script executed:
# Search for comments or TODOs about PkgPath or imports in generate.go
rg -n 'TODO|FIXME|XXX|HACK' pkg/config/generate.goRepository: ConductorOne/baton-sdk
Length of output: 48
🏁 Script executed:
# Check if there are any comments explaining constraints
rg -n '//|/\*' pkg/config/generate.go | head -20Repository: ConductorOne/baton-sdk
Length of output: 495
🏁 Script executed:
# Search for how connectorbuilder types are used elsewhere
rg -n 'connectorbuilder.ConnectorBuilder' --type go | head -15Repository: ConductorOne/baton-sdk
Length of output: 366
🏁 Script executed:
# Let's verify what happens with reflect.Type.PkgPath() for different scenarios
python3 << 'EOF'
# Simulate Go's reflect behavior for PkgPath()
# According to Go docs: PkgPath returns the import path of the package defining t
# - For types in the current package: returns ""
# - For imported types: returns the import path
# - For built-in types: returns ""
scenarios = [
("Type defined in current package", ""),
("Type from external package", "github.com/example/pkg"),
("Built-in type", ""),
("Unexported type in same package", ""),
]
print("PkgPath() behavior in Go:")
for scenario, path in scenarios:
print(f" {scenario}: '{path}'")
if path == "":
print(f" -> Generated import would be: import ( \"reflect\" \"\" ) - INVALID")
else:
print(f" -> Generated import would be: import ( \"reflect\" \"{path}\" ) - VALID")
EOFRepository: ConductorOne/baton-sdk
Length of output: 152
Add validation to prevent empty package paths in generated imports.
The template at line 119 will generate an invalid import statement if DefaultBuilderPkgPath is empty. This occurs when builderType.PkgPath() returns an empty string—which happens when the builder type is defined in the same package. Add a validation check in the Generate function after the pointer type handling:
if builderType.PkgPath() == "" {
panic("builder type must be from an external package")
}🤖 Prompt for AI Agents
In pkg/config/generate.go around lines 117 to 120, the template may emit an
invalid import when DefaultBuilderPkgPath is empty because builderType.PkgPath()
can be empty for types in the same package; after you resolve pointer types in
Generate, validate builderType.PkgPath() and panic (or return an explicit error)
if it is empty so the generator never produces an empty import path—add a check
like `if builderType.PkgPath() == "" { panic("builder type must be from an
external package") }` immediately after pointer unwrapping to enforce this.
MarcusGoldschmidt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/config/generate.go (1)
112-112: Inconsistent receiver formatting in generated template.The receiver style
(c* {{ .StructName }})on line 112 lacks a space between the variable name and the asterisk, while all other methods in this template use(c *{{ .StructName }})with proper spacing (lines 127, 139, 151, 163, 175).-func (c* {{ .StructName }}) findFieldByTag(tagValue string) (any, bool) { +func (c *{{ .StructName }}) findFieldByTag(tagValue string) (any, bool) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/cli/commands.go(3 hunks)pkg/config/generate.go(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.go
📚 Learning: 2025-02-11T21:02:21.506Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 299
File: pkg/config/generate.go:0-0
Timestamp: 2025-02-11T21:02:21.506Z
Learning: The getter methods in config structs (GetString, GetInt, GetBool, GetStringSlice) must match Viper's interface signatures and cannot be changed to return errors.
Applied to files:
pkg/config/generate.go
🧬 Code graph analysis (1)
pkg/cli/commands.go (4)
pkg/types/types.go (1)
ConnectorServer(11-27)pkg/cli/cli.go (1)
RunTimeOpts(18-21)pb/c1/connector/v2/connector.pb.go (4)
ConnectorCapabilities(739-746)ConnectorCapabilities(759-759)ConnectorServiceGetMetadataRequest(1056-1060)ConnectorServiceGetMetadataRequest(1073-1073)vendor/github.com/google/go-cmp/cmp/internal/flags/flags.go (1)
Deterministic(9-9)
🪛 GitHub Check: go-lint
pkg/cli/commands.go
[failure] 632-632:
nilness: tautological condition: nil == nil (govet)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
🔇 Additional comments (4)
pkg/cli/commands.go (2)
643-654: Clean optional interface pattern.The local
getterinterface for optionalGetCapabilitiesimplementation is idiomatic Go. This allows connectors to optionally provide capabilities without modifying the coreConnectorServerinterface.
656-667: LGTM!The fallback to metadata-based capabilities with the
HasCapabilities()guard is a reasonable approach for backward compatibility with connectors that don't implement the optionalGetCapabilitiesmethod.pkg/config/generate.go (2)
70-82: LGTM!The variant-to-type mapping is straightforward and covers all expected cases. Standardizing
StringVariantto always map to"string"simplifies the logic.
23-99: Summary inconsistency: Missing generic signature and DefaultBuilder method.The AI summary describes that
Generateshould have a generic signatureGenerate[T connectorbuilder.ConnectorBuilder]and that the generated struct should include aDefaultBuilder() anymethod. However, the actual code shows:
Generateis non-generic:func Generate(name string, schema field.Configuration)- The template does not include a
DefaultBuilder()methodThis may indicate incomplete implementation or a stale summary.
Please verify if the generic signature and
DefaultBuilder()method were intentionally omitted or if this is a work-in-progress that needs completion.
MarcusGoldschmidt
force-pushed
the
goldschmidt/capabilities
branch
from
f5e00ad to
d7ef329
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cli/commands.go (1)
622-646: Error handling issues with variable shadowing and nil error wrapping.Several issues in this block:
- Line 629 redeclares
errwith:=, shadowing the outererrfrom line 624- Line 645 wraps
errwhich isnilif we successfully created the connector via the fallback path (since err is reassigned on line 638)- The
%wformat verb with a nil error produces malformed output🔎 Proposed fix
var c types.ConnectorServer - - c, err = defaultConnectorBuilder(ctx, opts...) - - if c == nil || err != nil { + c, defaultErr := defaultConnectorBuilder(ctx, opts...) + if c == nil || defaultErr != nil { readFromPath := true decodeOpts := field.WithAdditionalDecodeHooks(field.FileUploadDecodeHook(readFromPath)) t, err := MakeGenericConfiguration[T](v, decodeOpts) if err != nil { return fmt.Errorf("failed to make configuration: %w", err) } // validate required fields and relationship constraints if err := field.Validate(confschema, t, field.WithAuthMethod(v.GetString("auth-method"))); err != nil { return err } c, err = getconnector(runCtx, t, RunTimeOpts{}) if err != nil { return err } } if c == nil { - return fmt.Errorf("could not create connector %w", err) + return fmt.Errorf("could not create connector") }
🧹 Nitpick comments (2)
internal/tests/default_connector_capabilitites_test.go (1)
1-27: Typo in filename: "capabilitites" should be "capabilities".The filename
default_connector_capabilitites_test.gocontains a typo. Consider renaming todefault_connector_capabilities_test.gofor consistency.The test logic itself is correct and properly validates that the
capabilitiessub-command works with a default connector builder, bypassing required field validation.pkg/cli/commands.go (1)
720-731: Consider handling nil defaultConnector explicitly.
connectorbuilder.NewConnectormay handle nil input, but the current flow passes the raw result fromExtractDefaultConnector(which can benil) directly. Based on learnings,NewConnectorvalidates its input, but returning early whendefaultConnectoris nil would be more explicit.🔎 Proposed improvement
func defaultConnectorBuilder(ctx context.Context, opts ...connectorrunner.Option) (types.ConnectorServer, error) { defaultConnector, err := connectorrunner.ExtractDefaultConnector(ctx, opts...) if err != nil { return nil, err } + if defaultConnector == nil { + return nil, nil + } c, err := connectorbuilder.NewConnector(ctx, defaultConnector) if err != nil { return nil, err } return c, nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/tests/default_connector_capabilitites_test.gointernal/tests/dummymain.gointernal/tests/notrequireargsinsubcommands_test.gointernal/tests/passingvalues_test.gopkg/cli/commands.gopkg/config/config.gopkg/connectorrunner/runner.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gointernal/tests/default_connector_capabilitites_test.gointernal/tests/dummymain.gopkg/config/config.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gointernal/tests/default_connector_capabilitites_test.gointernal/tests/dummymain.gopkg/config/config.go
🧬 Code graph analysis (4)
pkg/connectorrunner/runner.go (2)
pkg/connectorbuilder/connectorbuilder.go (2)
ConnectorBuilder(49-53)ConnectorBuilderV2(55-59)internal/connector/connector.go (1)
Option(104-104)
internal/tests/default_connector_capabilitites_test.go (5)
pkg/field/field_options.go (1)
WithRequired(11-45)pkg/field/struct.go (1)
NewConfiguration(82-92)vendor/github.com/stretchr/testify/suite/suite.go (1)
Run(126-226)pkg/connectorrunner/runner.go (2)
Option(255-255)WithDefaultConnectorBuilder(683-688)internal/tests/dummyconnector.go (1)
Dummy(11-11)
internal/tests/dummymain.go (4)
pkg/connectorrunner/runner.go (1)
Option(255-255)internal/connector/connector.go (1)
Option(104-104)pkg/tasks/local/syncer.go (1)
Option(30-30)pkg/config/config.go (1)
DefineConfigurationV2(87-229)
pkg/config/config.go (2)
pkg/cli/cli.go (1)
AddCommand(73-84)pkg/cli/commands.go (1)
MakeCapabilitiesCommand(594-697)
🔇 Additional comments (6)
pkg/config/config.go (1)
210-216: LGTM! Intentional bypass of config validation for capabilities command.The switch from
cli.AddCommandto directmainCMD.AddCommandcorrectly enables thecapabilitiessub-command to run without validating required config flags. This is necessary for the default connector builder flow where configuration may be incomplete.internal/tests/passingvalues_test.go (1)
47-56: LGTM! Consistent signature updates for the new options parameter.All entrypoint calls correctly pass
nilfor the newoptionsparameter when no connector runner options are needed. The changes are mechanical and consistent across all test cases.internal/tests/notrequireargsinsubcommands_test.go (1)
22-35: LGTM! Tests correctly validate both success and failure paths.The test at line 28-29 correctly expects an error when
capabilitiesis called without a default connector builder and without required configuration. This validates the fallback behavior introduced by the PR.internal/tests/dummymain.go (1)
17-18: LGTM! Entrypoint correctly propagates options to DefineConfigurationV2.The signature update properly accepts
[]connectorrunner.Optionand spreads it toDefineConfigurationV2, enabling tests to exercise the new default connector builder functionality.pkg/connectorrunner/runner.go (1)
681-697: LGTM! Well-documented option functions for default connector builders.The two option functions correctly set the respective builder types on the runner config. The doc comments clearly explain the purpose (used by the "capabilities" sub-command).
pkg/cli/commands.go (1)
648-672: LGTM! Clean capability retrieval with interface check and metadata fallback.The pattern of checking for a
GetCapabilitiesinterface first and falling back to metadata is well-implemented. The localgetterinterface allows duck-typing without requiring the connector to implement a specific exported interface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/connectorrunner/runner.go (1)
723-742: Bug:return false, errshould bereturn nil, err.On line 729, the function returns
falseinstead ofnilfor theanyreturn type on the error path. While this compiles, it's semantically incorrect and could cause unexpected behavior for callers checking fornil.🔎 Proposed fix
func ExtractDefaultConnector(ctx context.Context, options ...Option) (any, error) { cfg := &runnerConfig{} for _, o := range options { err := o(ctx, cfg) if err != nil { - return false, err + return nil, err } }pkg/cli/commands.go (1)
630-654: Fix potential nilerrin error message.On line 653, if the fallback path at lines 646-649 succeeds but returns
nilconnector,errwill benilat this point, resulting in a malformed error message like"could not create connector %!w(<nil>)".🔎 Proposed fix
if c == nil { - return fmt.Errorf("could not create connector %w", err) + return fmt.Errorf("could not create connector: connector is nil") }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/tests/default_connector_capabilitites_test.gointernal/tests/dummymain.gointernal/tests/notrequireargsinsubcommands_test.gointernal/tests/passingvalues_test.gopkg/cli/commands.gopkg/config/config.gopkg/config/generate.gopkg/connectorbuilder/connectorbuilder.gopkg/connectorbuilder/connectorbuilder_test.gopkg/connectorrunner/runner.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/config/generate.go
- internal/tests/default_connector_capabilitites_test.go
- internal/tests/passingvalues_test.go
- pkg/connectorbuilder/connectorbuilder_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gointernal/tests/dummymain.gopkg/config/config.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gointernal/tests/dummymain.gopkg/config/config.go
🧬 Code graph analysis (6)
pkg/connectorbuilder/connectorbuilder.go (2)
pb/c1/connector/v2/connector.pb.go (2)
ConnectorCapabilities(739-746)ConnectorCapabilities(759-759)pb/c1/connector/v2/connector_protoopaque.pb.go (2)
ConnectorCapabilities(745-752)ConnectorCapabilities(765-765)
pkg/cli/commands.go (6)
pkg/connectorrunner/runner.go (2)
Option(257-257)ExtractDefaultConnector(723-742)pkg/types/types.go (1)
ConnectorServer(11-27)pkg/field/decode_hooks.go (2)
WithAdditionalDecodeHooks(36-40)FileUploadDecodeHook(48-66)pkg/cli/cli.go (2)
MakeGenericConfiguration(42-60)NewConnector(40-40)pb/c1/connector/v2/connector.pb.go (4)
ConnectorCapabilities(739-746)ConnectorCapabilities(759-759)ConnectorServiceGetMetadataRequest(1056-1060)ConnectorServiceGetMetadataRequest(1073-1073)pkg/connectorbuilder/connectorbuilder.go (1)
NewConnector(84-205)
pkg/connectorrunner/runner.go (1)
pkg/connectorbuilder/connectorbuilder.go (2)
ConnectorBuilder(50-54)ConnectorBuilderV2(56-60)
internal/tests/notrequireargsinsubcommands_test.go (2)
vendor/github.com/stretchr/testify/suite/suite.go (1)
Run(126-226)vendor/github.com/doug-martin/goqu/v9/expressions.go (1)
T(232-234)
internal/tests/dummymain.go (2)
pkg/connectorrunner/runner.go (1)
Option(257-257)pkg/config/config.go (1)
DefineConfigurationV2(87-229)
pkg/config/config.go (2)
pkg/cli/cli.go (1)
AddCommand(73-84)pkg/cli/commands.go (1)
MakeCapabilitiesCommand(602-705)
🪛 GitHub Actions: ci
internal/tests/notrequireargsinsubcommands_test.go
[error] 29-29: golang-test-annotations step failed. notrequireargsinsubcommands_test.go:29:
🪛 GitHub Check: go-test (1.25.2, ubuntu-latest)
internal/tests/notrequireargsinsubcommands_test.go
[failure] 29-29:
notrequireargsinsubcommands_test.go:29:
Error Trace: /home/runner/work/baton-sdk/baton-sdk/internal/tests/notrequireargsinsubcommands_test.go:29
Error: Error message not equal:
expected: "(Cobra) Execute failed: required flag(s) "name" not set"
actual : "(Cobra) Execute failed: errors found:\nfield name of type string is marked as required but it has a zero-value"
Test: TestCallSubCommand/should_run_«capabilities»_sub-command_without_success
🔇 Additional comments (6)
pkg/connectorbuilder/connectorbuilder.go (1)
337-338: LGTM! Method correctly exported for external use.The rename from
getCapabilitiestoGetCapabilitiesproperly exposes this method for the new capabilities command flow, allowing direct capability retrieval without going throughGetMetadata.pkg/config/config.go (1)
210-216: LGTM! Intentional bypass of config validation for capabilities command.The direct
mainCMD.AddCommandusage (instead ofcli.AddCommand) correctly skips flag validation, allowing the capabilities command to run even with incomplete configuration when a default connector builder is provided.internal/tests/dummymain.go (1)
17-18: LGTM! Test helper correctly updated for new options parameter.The signature change properly propagates runner options to
DefineConfigurationV2, enabling tests to exercise the default connector builder functionality.pkg/connectorrunner/runner.go (1)
705-721: LGTM! Well-structured option functions for default connector builders.The two option functions correctly set the respective builder fields. The V2 variant provides future-proofing for connectors implementing the newer interface.
pkg/cli/commands.go (2)
656-680: LGTM! Clean capability retrieval with appropriate fallback.The two-tier approach is well-designed:
- First attempts
GetCapabilities(ctx)directly if the connector implements thegetterinterface- Falls back to
GetMetadata→GetCapabilities()for connectors that don't expose the direct methodThis maintains backward compatibility while enabling the optimized path.
728-739: LGTM! Helper function correctly builds default connector.The function properly extracts the default connector from options and uses
connectorbuilder.NewConnectorwhich, based on learnings, already validates the input and handles nil or invalid connectors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/tests/notrequireargsinsubcommands_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (1)
internal/tests/notrequireargsinsubcommands_test.go (1)
23-23: LGTM! Signature updates are correct.The entrypoint calls have been correctly updated to include the new
args []stringparameter (passed asnil), aligning with the updated function signature.Also applies to: 28-28
MarcusGoldschmidt
changed the title
MarcusGoldschmidt
changed the title
pkg/cli/commands.go
Outdated
|
|
||
| c, err = defaultConnectorBuilder(ctx, opts...) | ||
|
|
||
| if c == nil || err != nil { |
There was a problem hiding this comment.
Do we want to be able to discern the difference, explicitly, between:
This connector with this config has these capabilities, and this connector can do X if properly configured?
There was a problem hiding this comment.
The capabilities, for most of connectors, is static so I don't think we need to discern between them.
Also for those dynamic connectors, like baton-sql, we could remove the default option, so this ensures the needed for config to generate the capabilities.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @internal/tests/default_connector_capabilitites_test.go:
- Line 1: The test filename contains a typo: rename the file from
default_connector_capabilitites_test.go to
default_connector_capabilities_test.go so it matches the
TestDefaultConnectorCapabilities test function name and Go test file naming
conventions; update any CI or references that explicitly mention the old
filename if present.
♻️ Duplicate comments (1)
pkg/connectorrunner/runner.go (1)
723-742: Bug: Incorrect return value on error path (duplicate).On line 729, the function returns
falseinstead ofnilfor theanyreturn type. This issue has already been identified in a previous review.
🧹 Nitpick comments (1)
internal/tests/default_connector_capabilitites_test.go (1)
21-26: Consider verifying the command output.The test currently only asserts that no error occurs. Consider enhancing it to verify that:
- The capabilities were actually retrieved
- The Dummy connector was properly instantiated
- The output contains expected capability information
This would provide stronger validation of the default connector builder flow.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/tests/default_connector_capabilitites_test.gopkg/connectorrunner/runner.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
internal/tests/default_connector_capabilitites_test.gopkg/connectorrunner/runner.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
internal/tests/default_connector_capabilitites_test.gopkg/connectorrunner/runner.go
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
internal/tests/default_connector_capabilitites_test.go
🧬 Code graph analysis (1)
pkg/connectorrunner/runner.go (1)
pkg/connectorbuilder/connectorbuilder.go (2)
ConnectorBuilder(50-54)ConnectorBuilderV2(56-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (3)
pkg/connectorrunner/runner.go (3)
14-14: LGTM: Import addition is appropriate.The
connectorbuilderpackage import is necessary for the new builder type fields and option functions.
360-361: LGTM: Config fields are properly structured.The two builder fields are appropriately unexported and follow the existing config pattern. Supporting both
ConnectorBuilderandConnectorBuilderV2provides good forward compatibility.
705-721: LGTM: Option functions follow established patterns.Both option constructors are well-documented and follow the consistent option pattern used throughout the file. The naming clearly indicates these are for default capabilities connector builders, addressing previous naming concerns.
MarcusGoldschmidt
force-pushed
the
goldschmidt/capabilities
branch
from
7f208fe to
844d6a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @pkg/cli/commands.go:
- Around line 632-656: The code currently treats any error from
defaultConnectorBuilder as "no connector" by using `if c == nil || err != nil`;
change this to explicitly handle three cases: if `err != nil` return the error
immediately (preserving the original error), else if `c == nil` then perform the
fallback flow that calls `MakeGenericConfiguration[T]` and `getconnector`, and
otherwise continue; also fix the final error message to include the missing
colon and handle a nil err by returning a clear message, e.g. replace
`fmt.Errorf("could not create connector %w", err)` with `fmt.Errorf("could not
create connector: %w", err)` (or a non-wrapping message if `err` is nil).
In @pkg/connectorrunner/runner.go:
- Around line 725-744: In ExtractDefaultConnector, the option error path
currently returns false instead of a nil interface; change the early return
inside the options loop to return nil, err so the function consistently returns
(any, error) on failure — update the error handling in the for _, o := range
options loop to return nil, err when o(ctx, cfg) produces an error and leave
other return paths unchanged.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/tests/default_connector_capabilitites_test.gointernal/tests/dummymain.gointernal/tests/notrequireargsinsubcommands_test.gointernal/tests/passingvalues_test.gopkg/cli/commands.gopkg/config/config.gopkg/connectorbuilder/connectorbuilder.gopkg/connectorbuilder/connectorbuilder_test.gopkg/connectorrunner/runner.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/tests/passingvalues_test.go
- pkg/connectorbuilder/connectorbuilder.go
- internal/tests/dummymain.go
- internal/tests/default_connector_capabilitites_test.go
- pkg/connectorbuilder/connectorbuilder_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gopkg/config/config.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/cli/commands.gopkg/connectorrunner/runner.gopkg/config/config.go
📚 Learning: 2026-01-02T17:21:01.723Z
Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.
Applied to files:
internal/tests/notrequireargsinsubcommands_test.go
🧬 Code graph analysis (2)
pkg/connectorrunner/runner.go (1)
pkg/connectorbuilder/connectorbuilder.go (2)
ConnectorBuilder(50-54)ConnectorBuilderV2(56-60)
pkg/config/config.go (2)
pkg/cli/cli.go (1)
AddCommand(73-84)pkg/cli/commands.go (1)
MakeCapabilitiesCommand(604-707)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
🔇 Additional comments (8)
pkg/connectorrunner/runner.go (3)
14-14: LGTM!New import for
connectorbuilderpackage is correctly added to support the new default capabilities builder functionality.
361-362: LGTM!New fields for default capabilities connector builders are correctly typed and follow the existing pattern in the struct.
707-723: LGTM!Both option functions follow the established pattern in this file and are appropriately documented. Based on learnings, nil validation is not needed as
connectorbuilder.NewConnectorhandles validation internally.pkg/config/config.go (1)
210-216: LGTM!The change to bypass
cli.AddCommandfor the capabilities subcommand is well-justified by the comment. This allows querying connector capabilities without requiring complete configuration, which aligns with the PR objective of supporting static capabilities. The options are correctly passed through toMakeCapabilitiesCommand.internal/tests/notrequireargsinsubcommands_test.go (1)
22-30: LGTM!Tests correctly updated to match the new
entrypointsignature that includes anargsparameter. Passingnilfor args when no arguments are needed is appropriate.pkg/cli/commands.go (3)
12-13: LGTM!New imports for
connectorbuilderandtypespackages are correctly added to support the default connector builder functionality.
658-682: LGTM!The getter interface pattern for optional
GetCapabilitiessupport is well-implemented. The fallback to metadata-based capabilities with proper validation (HasCapabilities()check) provides good backward compatibility.
729-741: LGTM!The helper function correctly composes
ExtractDefaultConnectorandNewConnector. Based on learnings,connectorbuilder.NewConnectorhandles nil validation internally, so no additional checks are needed. Note that this function depends on the fix forExtractDefaultConnectorreturningfalseinstead ofnilon error (flagged in runner.go review).
| var c types.ConnectorServer | ||
|
|
||
| c, err = defaultConnectorBuilder(ctx, opts...) | ||
|
|
||
| if c == nil || err != nil { | ||
| readFromPath := true | ||
| decodeOpts := field.WithAdditionalDecodeHooks(field.FileUploadDecodeHook(readFromPath)) | ||
| t, err := MakeGenericConfiguration[T](v, decodeOpts) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to make configuration: %w", err) | ||
| } | ||
| // validate required fields and relationship constraints | ||
| if err := field.Validate(confschema, t, field.WithAuthMethod(v.GetString("auth-method"))); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| c, err = getconnector(runCtx, t, RunTimeOpts{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| // validate required fields and relationship constraints | ||
| if err := field.Validate(confschema, t, field.WithAuthMethod(v.GetString("auth-method"))); err != nil { | ||
| return err | ||
|
|
||
| if c == nil { | ||
| return fmt.Errorf("could not create connector %w", err) | ||
| } |
There was a problem hiding this comment.
Silently ignoring errors from defaultConnectorBuilder may mask issues.
The condition if c == nil || err != nil on line 636 treats all errors the same as "no connector available", which could mask configuration errors. Consider only falling back when c == nil && err == nil (no default configured), and returning early if there's an actual error.
Additionally, line 655 has a format string issue - missing colon before %w.
🐛 Proposed fix
var c types.ConnectorServer
c, err = defaultConnectorBuilder(ctx, opts...)
-
- if c == nil || err != nil {
+ if err != nil {
+ return err
+ }
+
+ if c == nil {
readFromPath := true
decodeOpts := field.WithAdditionalDecodeHooks(field.FileUploadDecodeHook(readFromPath))
t, err := MakeGenericConfiguration[T](v, decodeOpts)
if err != nil {
return fmt.Errorf("failed to make configuration: %w", err)
}
// validate required fields and relationship constraints
if err := field.Validate(confschema, t, field.WithAuthMethod(v.GetString("auth-method"))); err != nil {
return err
}
c, err = getconnector(runCtx, t, RunTimeOpts{})
if err != nil {
return err
}
}
if c == nil {
- return fmt.Errorf("could not create connector %w", err)
+ return fmt.Errorf("could not create connector")
}🤖 Prompt for AI Agents
In @pkg/cli/commands.go around lines 632 - 656, The code currently treats any
error from defaultConnectorBuilder as "no connector" by using `if c == nil ||
err != nil`; change this to explicitly handle three cases: if `err != nil`
return the error immediately (preserving the original error), else if `c == nil`
then perform the fallback flow that calls `MakeGenericConfiguration[T]` and
`getconnector`, and otherwise continue; also fix the final error message to
include the missing colon and handle a nil err by returning a clear message,
e.g. replace `fmt.Errorf("could not create connector %w", err)` with
`fmt.Errorf("could not create connector: %w", err)` (or a non-wrapping message
if `err` is nil).
Add option for static capabilities
Usage
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.