[CX-832] Containerize and update baton-sdk version #97
Conversation
WalkthroughRefactors the connector to use a generated config.Snowflake struct and configuration-driven New(cfg *config.Snowflake). Converts pagination/token-based APIs to rs.SyncOpAttrs/ *rs.SyncOpResults across builders, renames unused context params to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
FeliLucero1
changed the title
![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
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 70-91: The constructor New currently dereferences cfg without a
nil check and doesn't validate required Snowflake fields; add an initial nil
check for cfg and return a descriptive error if nil, then validate that
cfg.AccountUrl, cfg.AccountIdentifier, and cfg.UserIdentifier are non-empty (and
any other required fields for JWT/client init) before proceeding to key parsing;
update the error messages to clearly mention which field is missing and keep
validations near the top of New so subsequent code (private key parsing and
Connector creation) can safely assume cfg is populated.
| func New(ctx context.Context, cfg *config.Snowflake) (*Connector, error) { | ||
| if cfg.PrivateKeyPath == "" && cfg.PrivateKey == "" { | ||
| return nil, fmt.Errorf("private-key or private-key-path is required") | ||
| } | ||
| if privateKeyPath != "" && privateKey != "" { | ||
| if cfg.PrivateKeyPath != "" && cfg.PrivateKey != "" { | ||
| return nil, fmt.Errorf("only one of private-key or private-key-path can be provided") | ||
| } | ||
| var privateKeyValue any | ||
| if privateKeyPath != "" { | ||
| if cfg.PrivateKeyPath != "" { | ||
| var err error | ||
| privateKeyValue, err = snowflake.ReadPrivateKey(privateKeyPath) | ||
| privateKeyValue, err = snowflake.ReadPrivateKey(cfg.PrivateKeyPath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| if privateKey != "" { | ||
| if cfg.PrivateKey != "" { | ||
| var err error | ||
| privateKeyValue, err = snowflake.ParsePrivateKey([]byte(privateKey)) | ||
| privateKeyValue, err = snowflake.ParsePrivateKey([]byte(cfg.PrivateKey)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Add nil check for cfg parameter and consider validating required fields.
The function dereferences cfg immediately without checking if it's nil, which would cause a panic. Additionally, AccountUrl, AccountIdentifier, and UserIdentifier appear to be required for proper JWT generation and client initialization but lack validation.
🛡️ Proposed fix
func New(ctx context.Context, cfg *config.Snowflake) (*Connector, error) {
+ if cfg == nil {
+ return nil, fmt.Errorf("config is required")
+ }
+ if cfg.AccountUrl == "" {
+ return nil, fmt.Errorf("account-url is required")
+ }
+ if cfg.AccountIdentifier == "" {
+ return nil, fmt.Errorf("account-identifier is required")
+ }
+ if cfg.UserIdentifier == "" {
+ return nil, fmt.Errorf("user-identifier is required")
+ }
if cfg.PrivateKeyPath == "" && cfg.PrivateKey == "" {
return nil, fmt.Errorf("private-key or private-key-path is required")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func New(ctx context.Context, cfg *config.Snowflake) (*Connector, error) { | |
| if cfg.PrivateKeyPath == "" && cfg.PrivateKey == "" { | |
| return nil, fmt.Errorf("private-key or private-key-path is required") | |
| } | |
| if privateKeyPath != "" && privateKey != "" { | |
| if cfg.PrivateKeyPath != "" && cfg.PrivateKey != "" { | |
| return nil, fmt.Errorf("only one of private-key or private-key-path can be provided") | |
| } | |
| var privateKeyValue any | |
| if privateKeyPath != "" { | |
| if cfg.PrivateKeyPath != "" { | |
| var err error | |
| privateKeyValue, err = snowflake.ReadPrivateKey(privateKeyPath) | |
| privateKeyValue, err = snowflake.ReadPrivateKey(cfg.PrivateKeyPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if privateKey != "" { | |
| if cfg.PrivateKey != "" { | |
| var err error | |
| privateKeyValue, err = snowflake.ParsePrivateKey([]byte(privateKey)) | |
| privateKeyValue, err = snowflake.ParsePrivateKey([]byte(cfg.PrivateKey)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| func New(ctx context.Context, cfg *config.Snowflake) (*Connector, error) { | |
| if cfg == nil { | |
| return nil, fmt.Errorf("config is required") | |
| } | |
| if cfg.AccountUrl == "" { | |
| return nil, fmt.Errorf("account-url is required") | |
| } | |
| if cfg.AccountIdentifier == "" { | |
| return nil, fmt.Errorf("account-identifier is required") | |
| } | |
| if cfg.UserIdentifier == "" { | |
| return nil, fmt.Errorf("user-identifier is required") | |
| } | |
| if cfg.PrivateKeyPath == "" && cfg.PrivateKey == "" { | |
| return nil, fmt.Errorf("private-key or private-key-path is required") | |
| } | |
| if cfg.PrivateKeyPath != "" && cfg.PrivateKey != "" { | |
| return nil, fmt.Errorf("only one of private-key or private-key-path can be provided") | |
| } | |
| var privateKeyValue any | |
| if cfg.PrivateKeyPath != "" { | |
| var err error | |
| privateKeyValue, err = snowflake.ReadPrivateKey(cfg.PrivateKeyPath) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| if cfg.PrivateKey != "" { | |
| var err error | |
| privateKeyValue, err = snowflake.ParsePrivateKey([]byte(cfg.PrivateKey)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 70 - 91, The constructor New
currently dereferences cfg without a nil check and doesn't validate required
Snowflake fields; add an initial nil check for cfg and return a descriptive
error if nil, then validate that cfg.AccountUrl, cfg.AccountIdentifier, and
cfg.UserIdentifier are non-empty (and any other required fields for JWT/client
init) before proceeding to key parsing; update the error messages to clearly
mention which field is missing and keep validations near the top of New so
subsequent code (private key parsing and Connector creation) can safely assume
cfg is populated.
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
Let's revisit resource_types.go and add Annotations: getSkipEntitlementsAnnotation(), for resources that are not implementing entitlements and grants
| ) | ||
| AccountIdentifierField = field.StringField( | ||
| "account-identifier", | ||
| field.WithDisplayName("Account Identifier"), |
There was a problem hiding this comment.
based on c1 this field should be renamed to "Account ID / Locator" > https://github.com/ductone/c1/blob/main/pkg/builtin_connectors/snowflake.go
There was a problem hiding this comment.
we should refactor all the descriptions since there isn't much descriptive info
| field.WithRequired(true), | ||
| field.WithDescription("User Identifier."), | ||
| ) | ||
| PrivateKeyPathField = field.StringField( |
There was a problem hiding this comment.
this field should include Export target CLI only > Example
| field.WithDisplayName("Private Key Path"), | ||
| field.WithDescription("Private Key Path."), | ||
| ) | ||
| PrivateKeyField = field.StringField( |
There was a problem hiding this comment.
This field should be replaces by an input file field with allowed extensions (".p8", ".pem", ".key")
| configurationFields, | ||
| field.WithConstraints(fieldRelationships...), | ||
| field.WithConnectorDisplayName("Snowflake"), | ||
| field.WithHelpUrl("/docs/baton/snowflake"), |
There was a problem hiding this comment.
help url should match the v2 connector url ("/docs/baton/snowflake-v2") > https://github.com/ductone/c1/blob/main/pkg/builtin_connectors/snowflake.go#L32
| bag, cursor, err := parseCursorFromToken(opts.PageToken.Token, &v2.ResourceId{ResourceType: o.resourceType.Id}) | ||
| if err != nil { | ||
| return nil, "", nil, wrapError(err, "failed to get next page offset") | ||
| return nil, &rs.SyncOpResults{}, wrapError(err, "failed to get next page offset") |
There was a problem hiding this comment.
we should return nil here instead of empty struct
There was a problem hiding this comment.
please revisit all scenarios where you are returning empty op results
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.