[BB-1686] add push attribute action support and example #90
Conversation
agustin-conductor
requested review from
btipling and
laurenleach
as code owners
WalkthroughExports query-variable preparation as PrepareQueryVars, normalizes CEL results (nulls and DB-specific booleans), exposes an Changes
Sequence DiagramsequenceDiagram
participant Handler as Action Handler
participant CEL as PrepareQueryVars (CEL)
participant DB as RunProvisioningQueries
Handler->>Handler: Build inputs = {"input": argMap}
Handler->>CEL: PrepareQueryVars(ctx, inputs, vars)
CEL->>CEL: Evaluate CEL expressions\napply normalizeValue to results
alt evaluation success
CEL-->>Handler: queryVars (map)
Handler->>Handler: Merge queryVars into argMap\n(queryVars take precedence)
Handler->>DB: RunProvisioningQueries(ctx, augmented argMap)
DB-->>Handler: results
else evaluation error
CEL-->>Handler: error
Handler-->>Handler: return formatted error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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)
🔇 Additional comments (4)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
examples/mysql-test.ymlis excluded by none and included by noneexamples/oracle-test.ymlis excluded by none and included by noneexamples/postgres-test.ymlis excluded by none and included by nonetest/mysql-init.sqlis excluded by none and included by nonetest/oracle-init.sqlis excluded by none and included by nonetest/postgres-init.sqlis excluded by none and included by none
📒 Files selected for processing (7)
pkg/bcel/bcel.go(1 hunks)pkg/bsql/entitlements.go(1 hunks)pkg/bsql/grants.go(1 hunks)pkg/bsql/provisioning.go(1 hunks)pkg/bsql/query.go(4 hunks)pkg/bsql/resources.go(1 hunks)pkg/connector/action.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/connector/action.gopkg/bsql/resources.gopkg/bcel/bcel.gopkg/bsql/provisioning.gopkg/bsql/query.gopkg/bsql/grants.gopkg/bsql/entitlements.go
🧬 Code graph analysis (1)
pkg/bsql/query.go (2)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/database/database.go (1)
Oracle(29-29)
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: go-lint
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (8)
pkg/bcel/bcel.go (1)
30-30: LGTM! Clean addition of input variable for CEL.The new "input" variable declaration follows the existing pattern and the comment clearly explains its purpose for action variables and account provisioning.
pkg/bsql/resources.go (1)
24-24: LGTM! Updated to use exported method.The change from
prepareQueryVarstoPrepareQueryVarsaligns with the broader migration to the exported API across the bsql package.pkg/bsql/provisioning.go (1)
163-163: LGTM! Updated to use exported method.The change from
prepareQueryVarstoPrepareQueryVarsis consistent with the export pattern applied across the codebase.pkg/bsql/grants.go (1)
80-80: LGTM! Updated to use exported method.The change from
prepareQueryVarstoPrepareQueryVarsfollows the consistent pattern of migrating to the exported API.pkg/bsql/entitlements.go (1)
84-84: LGTM! Updated to use exported method.The change from
prepareQueryVarstoPrepareQueryVarsis consistent with the export pattern applied throughout the PR.pkg/bsql/query.go (2)
357-382: LGTM! Clean export and integration of normalization.The exported
PrepareQueryVarsmethod correctly integrates the newnormalizeValueutility in both code paths (direct input reference and CEL evaluation), ensuring consistent handling of CEL null types and special values.
8-8: LGTM! Import additions support the new normalization logic.The
reflectand CELtypesimports are properly placed and necessary for thenormalizeValueimplementation.As per coding guidelines
Also applies to: 13-13
pkg/connector/action.go (1)
191-205: CEL variable precedence is intentional and properly documented.The precedence rule is by design:
PrepareQueryVarsevaluates CEL expressions from the action config and returns derived variables that are merged intoargMap. This allows CEL-computed values to override user-provided arguments for query parameterization purposes. The comment on line 202 correctly documents this behavior.
pkg/bsql/query.go
Outdated
| } | ||
|
|
||
| // normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility | ||
| // Also converts booleans to strings ('1'/'0') for Oracle compatibility when used in DECODE statements. |
| } | ||
|
|
||
| return val | ||
| } |
There was a problem hiding this comment.
Could you check if coderabbit's suggestion is valid? It seems to be, in my opinion.
pkg/bsql/query.go
Outdated
|
|
||
| // normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility | ||
| // Also converts booleans to strings ('1'/'0') for Oracle compatibility when used in DECODE statements. | ||
| func (s *SQLSyncer) normalizeValue(val interface{}) interface{} { |
There was a problem hiding this comment.
nit: should be (val any) any
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
There are some conflicts and questions from Felipe
agustin-conductor
force-pushed
the
feature/add_push_attr_action
branch
from
4767eae to
cf5c0b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
examples/mysql-test.ymlis excluded by none and included by noneexamples/oracle-test.ymlis excluded by none and included by noneexamples/postgres-test.ymlis excluded by none and included by nonetest/mysql-init.sqlis excluded by none and included by nonetest/oracle-init.sqlis excluded by none and included by nonetest/postgres-init.sqlis excluded by none and included by none
📒 Files selected for processing (7)
pkg/bcel/bcel.go(1 hunks)pkg/bsql/entitlements.go(1 hunks)pkg/bsql/grants.go(1 hunks)pkg/bsql/provisioning.go(1 hunks)pkg/bsql/query.go(4 hunks)pkg/bsql/resources.go(1 hunks)pkg/connector/action.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/bsql/entitlements.go
- pkg/bsql/grants.go
- pkg/bsql/provisioning.go
- pkg/connector/action.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bcel/bcel.gopkg/bsql/resources.gopkg/bsql/query.go
🧬 Code graph analysis (1)
pkg/bsql/query.go (2)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/database/database.go (1)
Oracle(31-31)
⏰ 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 (ubuntu-latest)
🔇 Additional comments (3)
pkg/bcel/bcel.go (1)
30-30: LGTM! New CEL variable supports action vars.The addition of the
inputvariable enables CEL expressions to access action variables and account provisioning data, which aligns with the PR's objective to support attribute push actions.pkg/bsql/resources.go (1)
24-24: LGTM! Correctly uses the exported method.The update to use the newly exported
PrepareQueryVarsmethod is consistent with similar changes across the codebase.pkg/bsql/query.go (1)
357-382: CEL null value handling in normalization is correctly implemented.The
normalizeValuefunction (lines 386-421) comprehensively handles CEL null types through three mechanisms:
- Direct nil check (line 387) — handles Go nil values
- Type assertion for
types.Null(lines 395-397) — catches CEL's null type directly- Reflection-based fallback (line 416-418) — catches other CEL null variants via type name inspection for "NullValue" or "types.Null"
Both code paths in
PrepareQueryVarsconsistently apply normalization:
- Direct input references (line 367)
- CEL evaluation results (line 378)
This ensures CEL null values are converted to Go nil before reaching the SQL driver, maintaining database compatibility. While no dedicated unit tests exist for this function, the implementation is sound and the normalization is applied uniformly across all callers.
| // normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility | ||
| // Also converts booleans to strings ("1"/"0") for Oracle compatibility when used in DECODE statements. | ||
| func (s *SQLSyncer) normalizeValue(val interface{}) interface{} { | ||
| if val == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // Check for CEL null types | ||
| switch v := val.(type) { | ||
| case string: | ||
| return v | ||
| case types.Null: | ||
| // CEL Null type | ||
| return nil | ||
| case bool: | ||
| // Convert boolean to string for Oracle compatibility (Oracle DECODE expects CHAR) | ||
| // Only convert for Oracle to avoid breaking other databases | ||
| if s.dbEngine == database.Oracle { | ||
| if v { | ||
| return "1" | ||
| } | ||
| return "0" | ||
| } | ||
| // For other databases, return as-is (let the driver handle it) | ||
| return v | ||
| } | ||
|
|
||
| // Use reflection to check for CEL null value types | ||
| valType := reflect.TypeOf(val) | ||
| if valType != nil { | ||
| typeName := valType.String() | ||
| // Check for CEL null value types | ||
| if strings.Contains(typeName, "NullValue") || strings.Contains(typeName, "types.Null") { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| return val | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant reflection-based null check.
The reflection-based fallback (lines 411-419) is unnecessary and fragile:
- Redundant: CEL-Go represents null values as
types.Null(already handled in the switch at line 395), so the reflection check serves no purpose. - Fragile: Using
strings.Containson type names could produce false positives with custom types containing "NullValue" or "types.Null" in their names. - Style: Modern Go prefers
anyoverinterface{}for the parameter type.
Based on the web search results confirming that types.Null is the canonical CEL null representation, the reflection block should be removed.
Apply this diff:
-// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility
-// Also converts booleans to strings ("1"/"0") for Oracle compatibility when used in DECODE statements.
-func (s *SQLSyncer) normalizeValue(val interface{}) interface{} {
+// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility.
+// Also converts booleans to strings ("1"/"0") for Oracle compatibility when used in DECODE statements.
+func (s *SQLSyncer) normalizeValue(val any) any {
if val == nil {
return nil
}
// Check for CEL null types
switch v := val.(type) {
case string:
return v
case types.Null:
// CEL Null type
return nil
case bool:
// Convert boolean to string for Oracle compatibility (Oracle DECODE expects CHAR)
// Only convert for Oracle to avoid breaking other databases
if s.dbEngine == database.Oracle {
if v {
return "1"
}
return "0"
}
// For other databases, return as-is (let the driver handle it)
return v
}
- // Use reflection to check for CEL null value types
- valType := reflect.TypeOf(val)
- if valType != nil {
- typeName := valType.String()
- // Check for CEL null value types
- if strings.Contains(typeName, "NullValue") || strings.Contains(typeName, "types.Null") {
- return nil
- }
- }
-
return val
}If the reflection block is removed, also remove the unused reflect import at line 8.
🤖 Prompt for AI Agents
In pkg/bsql/query.go around lines 384 to 422, remove the redundant and fragile
reflection-based null check (the block that inspects reflect.TypeOf(val) and
checks typeName for "NullValue" or "types.Null"); change the function signature
parameter from interface{} to any; delete the reflect import if no longer used;
ensure the existing switch case for types.Null remains and return val for all
other cases unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/bsql/query.go (2)
419-427: Consider removing redundant reflection-based null check.This reflection block was flagged in a previous review as potentially redundant since CEL-Go represents null values as
types.Null, which is already handled in the switch case at line 395. Thestrings.Containscheck on type names is also fragile and could produce false positives.If the
types.Nullcase truly covers all CEL null representations, this block can be safely removed along with thereflectimport.return v } - // Use reflection to check for CEL null value types - valType := reflect.TypeOf(val) - if valType != nil { - typeName := valType.String() - // Check for CEL null value types - if strings.Contains(typeName, "NullValue") || strings.Contains(typeName, "types.Null") { - return nil - } - } - return val }
357-357: Add doc comment for exported methodPrepareQueryVars.Per Go conventions and coding guidelines, exported methods require a doc comment that is a complete sentence ending with a period.
+// PrepareQueryVars evaluates CEL expressions from vars against inputs and returns a map of normalized query variables. func (s *SQLSyncer) PrepareQueryVars(ctx context.Context, inputs map[string]any, vars map[string]string) (map[string]any, error) {Based on learnings, comments for exported items must be complete sentences ending with periods.
🧹 Nitpick comments (1)
pkg/bsql/query.go (1)
384-385: Doc comment should end with a period.Per coding guidelines, comments for exported items must be complete sentences ending with periods. The second line of this comment is missing a period.
-// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility -// Also converts booleans to strings ("1"/"0") for Oracle compatibility when used in DECODE statements. +// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility. +// Also converts booleans to strings ("1"/"0") for Oracle and integers (1/0) for MSSQL compatibility.Based on learnings, comments for exported items must be complete sentences ending with periods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/sqlserver-test.ymlis excluded by none and included by nonetest/sqlserver-init.sqlis excluded by none and included by none
📒 Files selected for processing (3)
pkg/bsql/config.go(2 hunks)pkg/bsql/query.go(4 hunks)pkg/connector/action.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/config.gopkg/bsql/query.gopkg/connector/action.go
🧠 Learnings (1)
📚 Learning: 2025-09-27T00:53:43.641Z
Learnt from: CR
Repo: ConductorOne/baton-sql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T00:53:43.641Z
Learning: Applies to **/*.go : Comments for exported items must be complete sentences ending with periods
Applied to files:
pkg/bsql/query.go
🧬 Code graph analysis (1)
pkg/bsql/query.go (2)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/database/database.go (2)
Oracle(31-31)MSSQL(30-30)
⏰ 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 (ubuntu-latest)
🔇 Additional comments (6)
pkg/connector/action.go (2)
131-133: LGTM!Good validation ensuring at least one of
QueryorQueriesis configured. This aligns with the updatedActionConfig.Validate()logic inconfig.go.
191-212: LGTM! CEL preprocessing and multi-query support.The implementation correctly:
- Wraps
argMapin aninputcontainer for CEL expressions- Evaluates CEL expressions via
PrepareQueryVars- Merges evaluated vars into
argMapwith proper precedence- Builds the queries slice from either
QueriesorQueryError handling follows guidelines with
fmt.Errorfand%wfor wrapping.pkg/bsql/config.go (2)
436-437: LGTM!Good addition of the
Queriesfield for multi-query support. Theomitemptytags on both fields correctly reflect their optional nature when the other is provided.
452-455: LGTM!The validation logic correctly enforces that at least one of
QueryorQueriesmust be provided, maintaining backward compatibility while supporting the new multi-query feature.pkg/bsql/query.go (2)
367-378: LGTM!Good application of
normalizeValueto both direct input references and CEL-evaluated outputs, ensuring consistent handling of null types and database-specific boolean conversions.
398-416: LGTM! Database-specific boolean handling.Good addition of MSSQL boolean conversion (lines 408-414) alongside the existing Oracle handling. The implementation correctly converts booleans to integers for MSSQL and strings for Oracle while letting other database drivers handle booleans natively.
agustin-conductor
force-pushed
the
feature/add_push_attr_action
branch
from
cd8185d to
e0099a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/bsql/query.go (2)
357-382: Add Go doc comment for exportedPrepareQueryVars.Since
PrepareQueryVarsis exported, it should have a Go doc comment starting with the method name and ending with a period, per the project guidelines. This also matches the earlier review feedback on this symbol.Example:
+// PrepareQueryVars evaluates vars against inputs, normalizes values for SQL, and returns the resulting map. func (s *SQLSyncer) PrepareQueryVars(ctx context.Context, inputs map[string]any, vars map[string]string) (map[string]any, error) {
8-14: SimplifynormalizeValue: drop reflection-based null detection and align comment with behavior.The new CEL/null and bool handling is useful, but the reflection fallback is fragile and likely unnecessary:
- It relies on substring checks of
reflect.TypeOf(val).String()for"NullValue"or"types.Null", which is brittle and may misclassify unrelated types.- CEL already exposes
types.Nullas the canonical null representation, which you’re explicitly handling.- Once this block is removed, the
reflectimport becomes unused.Also, the comment above
normalizeValuementions Oracle string conversion but not the MSSQL 1/0 path, and the first sentence lacks a trailing period.Consider:
-import ( - "context" - "database/sql" - "errors" - "fmt" - "reflect" - "regexp" - "strconv" - "strings" - ... -) +import ( + "context" + "database/sql" + "errors" + "fmt" + "regexp" + "strconv" + "strings" + ... +) @@ -// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility -// Also converts booleans to strings ("1"/"0") for Oracle compatibility when used in DECODE statements. +// normalizeValue converts CEL null types and other special values to Go nil for SQL compatibility. +// It also converts booleans to strings ("1"/"0") for Oracle and integers (1/0) for MSSQL when needed. func (s *SQLSyncer) normalizeValue(val any) any { @@ - // Use reflection to check for CEL null value types - valType := reflect.TypeOf(val) - if valType != nil { - typeName := valType.String() - // Check for CEL null value types - if strings.Contains(typeName, "NullValue") || strings.Contains(typeName, "types.Null") { - return nil - } - } - return val }This keeps the explicit
types.Nullhandling and bool normalization while avoiding the type-name heuristics.Also applies to: 384-430
🧹 Nitpick comments (1)
pkg/bsql/config.go (1)
452-464: Consider validating mutual exclusivity ofQueryandQueries.The validation allows both
QueryandQueriesto be set simultaneously. While the code elsewhere handles precedence (Queries takes priority per the AI summary), users might be confused about which field is used when both are provided.Consider either:
- Adding validation to ensure mutual exclusivity:
func (a *ActionConfig) Validate() error { if a.Query == "" && len(a.Queries) == 0 { return status.Errorf(codes.InvalidArgument, "query or queries is required") } + if a.Query != "" && len(a.Queries) > 0 { + return status.Errorf(codes.InvalidArgument, "query and queries are mutually exclusive; provide only one") + } for _, arg := range a.Arguments {OR
- Documenting the precedence behavior in the struct field comments (already suggested in the previous comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
examples/mysql-test.ymlis excluded by none and included by noneexamples/oracle-test.ymlis excluded by none and included by noneexamples/postgres-test.ymlis excluded by none and included by noneexamples/sqlserver-test.ymlis excluded by none and included by nonetest/mysql-init.sqlis excluded by none and included by nonetest/oracle-init.sqlis excluded by none and included by nonetest/postgres-init.sqlis excluded by none and included by nonetest/sqlserver-init.sqlis excluded by none and included by none
📒 Files selected for processing (8)
pkg/bcel/bcel.go(1 hunks)pkg/bsql/config.go(2 hunks)pkg/bsql/entitlements.go(1 hunks)pkg/bsql/grants.go(1 hunks)pkg/bsql/provisioning.go(1 hunks)pkg/bsql/query.go(4 hunks)pkg/bsql/resources.go(1 hunks)pkg/connector/action.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/bsql/grants.go
- pkg/bsql/provisioning.go
- pkg/bsql/resources.go
- pkg/bcel/bcel.go
- pkg/connector/action.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Error handling: usefmt.Errorfwith contextual messages; check specific errors witherrors.Is
Organize imports: standard library first, then third-party, then project imports; alphabetize within each group
Naming: CamelCase for exported identifiers; camelCase for unexported; preserve acronyms like ID, URL, HTTP, API
Limit line length to a maximum of 200 characters
Comments for exported items must be complete sentences ending with periods
Do not uselog.Fatalorlog.Panic(ruleguard-enforced)
Files:
pkg/bsql/entitlements.gopkg/bsql/config.gopkg/bsql/query.go
🧠 Learnings (1)
📚 Learning: 2025-09-27T00:53:43.641Z
Learnt from: CR
Repo: ConductorOne/baton-sql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-27T00:53:43.641Z
Learning: Applies to **/*.go : Comments for exported items must be complete sentences ending with periods
Applied to files:
pkg/bsql/query.go
🧬 Code graph analysis (1)
pkg/bsql/query.go (2)
pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/database/database.go (2)
Oracle(31-31)MSSQL(30-30)
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: go-test (ubuntu-latest)
- GitHub Check: go-lint
🔇 Additional comments (1)
pkg/bsql/entitlements.go (1)
80-88: Switch to exportedPrepareQueryVarslooks correct.The call now cleanly uses the exported helper with the same inputs and error handling, which aligns this code with the shared query-var preparation logic.
pkg/bsql/config.go
Outdated
| NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty" validate:"omitempty"` | ||
| Query string `yaml:"query" json:"query" validate:"required"` | ||
| Query string `yaml:"query,omitempty" json:"query,omitempty" validate:"omitempty"` | ||
| Queries []string `yaml:"queries,omitempty" json:"queries,omitempty" validate:"omitempty,dive"` |
There was a problem hiding this comment.
Add documentation comment for the Queries field.
The Queries field is missing a documentation comment. Per coding guidelines, comments for exported items must be complete sentences ending with periods.
Add a comment above line 437:
NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty" validate:"omitempty"`
Query string `yaml:"query,omitempty" json:"query,omitempty" validate:"omitempty"`
+ // Queries is a list of SQL statements to execute for the action. If non-empty, Queries takes precedence over Query.
Queries []string `yaml:"queries,omitempty" json:"queries,omitempty" validate:"omitempty,dive"`
// TODO: add validation?📝 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.
| Queries []string `yaml:"queries,omitempty" json:"queries,omitempty" validate:"omitempty,dive"` | |
| NoTransaction bool `yaml:"no_transaction,omitempty" json:"no_transaction,omitempty" validate:"omitempty"` | |
| Query string `yaml:"query,omitempty" json:"query,omitempty" validate:"omitempty"` | |
| // Queries is a list of SQL statements to execute for the action. If non-empty, Queries takes precedence over Query. | |
| Queries []string `yaml:"queries,omitempty" json:"queries,omitempty" validate:"omitempty,dive"` | |
| // TODO: add validation? |
🤖 Prompt for AI Agents
In pkg/bsql/config.go at line 437, the exported field `Queries` lacks a
documentation comment; add a comment line immediately above that starts with
"Queries" and is a complete sentence ending with a period that briefly describes
the field's purpose (e.g., that it holds an optional list of SQL or
parameterized query strings to execute), ensuring the comment follows the
project's guideline for exported identifiers.
| user: "${DB_USER}" | ||
| password: "${DB_PASSWORD}" | ||
|
|
||
| actions: |
There was a problem hiding this comment.
We should probably test the update user attr action in the postgres CI test.
It would be nice if we ran the mysql example in CI, similar to how we run the postgres example. But that can be a separate pull request.
There was a problem hiding this comment.
added ci test
examples/oracle-test.yml
Outdated
| role_name as "roleName" | ||
| FROM | ||
| DBA_ROLES | ||
| ROLES |
There was a problem hiding this comment.
Doesn't this change the behavior of the oracle example? Now it's operating on a roles table instead of the actual Oracle DB roles, right?
It's also removing support for Oracle privileges.
There was a problem hiding this comment.
mmm... might have misunderstood something... I thought this was bind to the example using this and if I remember correctly it wasnt working at all. I'll take another look
There was a problem hiding this comment.
reverted changes not related to the new action
| if a.Query == "" { | ||
| return status.Errorf(codes.InvalidArgument, "query is required") | ||
| if a.Query == "" && len(a.Queries) == 0 { | ||
| return status.Errorf(codes.InvalidArgument, "query or queries is required") |
There was a problem hiding this comment.
We should error if both Query and Queries are set. Also I think we can have some validate tags in the struct. something like validate:"required_without=...
There was a problem hiding this comment.
added missing validation to function and validate tags. Validate() was not running on cli invoke, replace validation statement for call to Validate func
Description
Add support and example for attribute push action
Useful links:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.