Baton Actions #302
Baton Actions #302
Conversation
WalkthroughThese changes introduce robust validation logic for Protocol Buffers message types and add a new RPC service for action management. In multiple Go files, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AS as ActionService
participant V as Validation Module
C->>+AS: Send Invoke request (ActionServiceInvokeRequest)
AS->>+V: Validate request (Validate/ValidateAll)
V-->>-AS: Return validation result (error or success)
AS->>AS: Process action if request is valid
AS-->>-C: Return Invoke response (ActionServiceInvokeResponse)
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
proto/c1/connector/v2/action.proto (1)
27-76: Messages design
The definitions forBatonActionSchemaand the request/response messages look coherent. Each message has fields needed for the newActionService. The usage ofgoogle.protobuf.Anyforannotationsis flexible but be mindful of potential overhead and type safety at runtime. Consider well-defined message types if feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)proto/c1/connector/v2/action.proto(1 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
proto/c1/connector/v2/action.proto
20-20: Enum value name "STATUS_UNKNOWN" should be prefixed with "BATON_ACTION_STATUS_".
(ENUM_VALUE_PREFIX)
20-20: Enum zero value name "STATUS_UNKNOWN" should be suffixed with "_UNSPECIFIED".
(ENUM_ZERO_VALUE_SUFFIX)
21-21: Enum value name "STATUS_PENDING" should be prefixed with "BATON_ACTION_STATUS_".
(ENUM_VALUE_PREFIX)
22-22: Enum value name "STATUS_RUNNING" should be prefixed with "BATON_ACTION_STATUS_".
(ENUM_VALUE_PREFIX)
23-23: Enum value name "STATUS_COMPLETE" should be prefixed with "BATON_ACTION_STATUS_".
(ENUM_VALUE_PREFIX)
24-24: Enum value name "STATUS_FAILED" should be prefixed with "BATON_ACTION_STATUS_".
(ENUM_VALUE_PREFIX)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/action.proto
[failure] 9-9:
Files in package "c1.connector.v2" have multiple values "github.com/conductorone/baton-sdk/pb/c1/config/v1,github.com/conductorone/baton-sdk/pb/c1/connector/v2" for option "go_package" and all values must be equal.
[failure] 20-20:
Enum value name "STATUS_UNKNOWN" should be prefixed with "BATON_ACTION_STATUS_".
[failure] 20-20:
Enum zero value name "STATUS_UNKNOWN" should be suffixed with "_UNSPECIFIED".
[failure] 21-21:
Enum value name "STATUS_PENDING" should be prefixed with "BATON_ACTION_STATUS_".
[failure] 22-22:
Enum value name "STATUS_RUNNING" should be prefixed with "BATON_ACTION_STATUS_".
[failure] 23-23:
Enum value name "STATUS_COMPLETE" should be prefixed with "BATON_ACTION_STATUS_".
[failure] 24-24:
Enum value name "STATUS_FAILED" should be prefixed with "BATON_ACTION_STATUS_".
🔇 Additional comments (18)
pb/c1/action/v1/action.pb.validate.go (6)
1-36: Avoid over-customizing auto-generated files.
This file appears to be generated byprotoc-gen-validate, so any manual modifications could be overwritten. Generally, application-specific logic is implemented in separate files, and generation is re-triggered upon changes to the proto definitions. Unless there's a pressing need for adjustments within the generated code, consider leaving these segments as-is.
38-96: Implementation of BatonAction validations looks correct.
TheValidate,ValidateAll, and internalvalidate(all bool)methods properly detect embedded message errors and aggregate them whenallis true. This structure is in line with standard protoc-gen-validate patterns. No concurrency, security, or performance issues are apparent here.
98-113: Multi-error wrapper for BatonAction.
TheBatonActionMultiErroris straightforward and aggregates multiple validation errors well. This is a standard approach for capturing batched validation feedback.
114-166: Structured individual validation error.
BatonActionValidationErroreffectively carries details about the problematic field, reason, and underlying cause. Such clarity is valuable when debugging validation failures.
168-297: Validation logic for BatonActionSchema.
These methods (and the embedded message checks forArguments,Constraints,ReturnTypes) mirror the pattern used forBatonAction. Everything looks consistent, and errors are collected properly.
301-372: Multi-error and individual error types for BatonActionSchema.
Implementation is consistent with the rest of the file and appears functionally complete. No further recommendations for this auto-generated logic.pb/c1/connector/v2/action.pb.validate.go (11)
1-37: Auto-generated validation boilerplate.
Similar to the previous file, these imports, variables, and initial comments appear to be standard protoc-gen-validate output. No action needed.
38-166: Mirroring BatonActionSchema validations for connector.
This block essentially replicates theBatonActionSchemavalidation logic in thev2package context. It enforces the same rules forArguments,Constraints, andReturnTypes. Everything looks coherent.
244-337: ActionServiceInvokeRequest validations.
The function checks embedded message validation forArgsand each item inAnnotations. This approach is correct and thorough. No concurrency or data race concerns here.
338-410: InvokeRequest multi-error and validation error structures.
The multi-error approach forActionServiceInvokeRequestis consistent. Fields such asfield,reason, andcauseprovide clarity on failure points.
411-543: ActionServiceInvokeResponse validations.
Again, looks good. It checksAnnotationsarray for embedded message compliance. No immediate improvements necessary.
552-683: ActionServiceStatusRequest validations.
Checks onAnnotationsmirror earlier patterns. No issues to resolve.
692-823: ActionServiceStatusResponse validations.
Similar pattern. The approach is consistent across request/response pairs.
833-963: ActionServiceGetSchemaRequest validations.
ValidatesAnnotationsarray. The code is uniform with prior definitions; no improvements needed.
972-1134: ActionServiceGetSchemaResponse validations.
The loops forArgumentsandReturnTypesreplicate the known approach of calling embeddedValidate. Implementation is robust.
1143-1279: ActionServiceListSchemasRequest validations.
Similar structure. Recursively validates each item inAnnotations.
1280-1418: ActionServiceListSchemasResponse validations.
Check of embeddedBatonActionSchemaitems is well-implemented. Overall, the entire file is consistent and meets typical protoc-gen-validate conventions.proto/c1/connector/v2/action.proto (1)
12-17: Service definitions:
The four RPCs (Invoke,Status,GetSchema,ListSchemas) match your new action management flows. The structure is straightforward, no immediate issues.
ggreer
force-pushed
the
ggreer/baton-actions
branch
2 times, most recently
from
42118f5 to
868fbc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
pb/c1/connector/v2/action.pb.validate.go (7)
1-37: Auto-generated code notice.This file is marked as auto-generated by protoc-gen-validate. Typically, such files should be excluded from manual edits and code reviews unless there’s a compelling reason to commit changes. Please confirm that committing this file is intentional to avoid potential merge conflicts and maintenance overhead.
38-243: No explicit validation rule for FunctionName.The generated code indicates “no validation rules for FunctionName” in the
BatonActionSchema. If you intend to enforce constraints (e.g., require a non-empty function name), consider specifying them in the proto definition so that protoc-gen-validate can generate the proper checks.
244-551: No validation rules for the 'Name' field in InvokeRequest.
ActionServiceInvokeRequestdoes not impose any constraints on theNamefield. If you need strict rules (e.g., there must be at least one character), add them to the.protofile.
552-832: Check 'Name' and 'Id' fields in StatusRequest.
ActionServiceStatusRequestalso omits explicit validation onNameorId. If these fields must be non-empty or match certain patterns, consider adding constraints within the.protofile for more robust validation.
972-1142: Arrays of 'Arguments' and 'ReturnTypes' lack global constraints.In
ActionServiceGetSchemaResponse, each element is validated if it implementsValidate, but the array size itself isn’t constrained. If you need a minimum or maximum length (e.g., at least one argument), define it in the proto to have protoc-gen-validate enforce it.
1143-1279: ListSchemasRequest fields lack explicit checks.
ActionServiceListSchemasRequestvalidates embedded messages but has no direct field-level constraints. Confirm whether certain fields need required or non-empty checks. If so, add them in the proto.
1280-1418: Repeated multi-error patterns.Multiple messages (e.g.,
ActionServiceListSchemasResponse) include nearly identical multi-error and validation error logic. Although standard for auto-generated code, it leads to duplication. If you’d prefer a leaner approach, consider adjusting your protobuf or protoc-gen-validate plugin configuration to reduce repeated boilerplate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)proto/c1/connector/v2/action.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pb/c1/action/v1/action.pb.validate.go
- proto/c1/connector/v2/action.proto
🔇 Additional comments (1)
pb/c1/connector/v2/action.pb.validate.go (1)
833-971: Missing validation constraints for 'Name' in GetSchemaRequest.Like previous messages,
ActionServiceGetSchemaRequestdoesn’t enforce rules for theNamefield. Verify whether this field should have any restrictions to maintain data integrity.
ggreer
force-pushed
the
ggreer/baton-actions
branch
2 times, most recently
from
baaed3e to
e940691
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/connectorbuilder/connectorbuilder.go (2)
1008-1032: Consider paginating or limiting the size of action schemas.
If the connector returns a large list, pagination support inListActionSchemasmay be beneficial to prevent excessive data transfers.
1060-1085: Invoking actions is handled correctly.
Error handling and metric recording appear consistent. Optionally, consider including more detailed error messages for easier debugging.pkg/tasks/c1api/actions.go (1)
1-184: Consider unifying repetitive handler logic.The four handlers (
actionListSchemasTaskHandler,actionGetSchemaTaskHandler, etc.) share similar responsibility patterns: opening spans, extracting the logger, and calling the connector client. Consolidating them into a more generic handler function or shared infrastructure could reduce code duplication and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
internal/connector/connector.go(2 hunks)pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(6 hunks)pkg/sync/syncer_test.go(1 hunks)pkg/tasks/c1api/actions.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/action.proto(1 hunks)proto/c1/connectorapi/baton/v1/baton.proto(3 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
proto/c1/connector/v2/action.proto
13-13: RPC request type "ActionServiceInvokeRequest" should be named "InvokeActionRequest" or "ActionServiceInvokeActionRequest".
(RPC_REQUEST_STANDARD_NAME)
13-13: RPC response type "ActionServiceInvokeResponse" should be named "InvokeActionResponse" or "ActionServiceInvokeActionResponse".
(RPC_RESPONSE_STANDARD_NAME)
14-14: RPC request type "ActionServiceStatusRequest" should be named "GetActionStatusRequest" or "ActionServiceGetActionStatusRequest".
(RPC_REQUEST_STANDARD_NAME)
14-14: RPC response type "ActionServiceStatusResponse" should be named "GetActionStatusResponse" or "ActionServiceGetActionStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
15-15: RPC request type "ActionServiceGetSchemaRequest" should be named "GetActionSchemaRequest" or "ActionServiceGetActionSchemaRequest".
(RPC_REQUEST_STANDARD_NAME)
15-15: RPC response type "ActionServiceGetSchemaResponse" should be named "GetActionSchemaResponse" or "ActionServiceGetActionSchemaResponse".
(RPC_RESPONSE_STANDARD_NAME)
16-16: RPC request type "ActionServiceListSchemasRequest" should be named "ListActionSchemasRequest" or "ActionServiceListActionSchemasRequest".
(RPC_REQUEST_STANDARD_NAME)
16-16: RPC response type "ActionServiceListSchemasResponse" should be named "ListActionSchemasResponse" or "ActionServiceListActionSchemasResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/action.proto
[failure] 13-13:
RPC request type "ActionServiceInvokeRequest" should be named "InvokeActionRequest" or "ActionServiceInvokeActionRequest".
[failure] 13-13:
RPC response type "ActionServiceInvokeResponse" should be named "InvokeActionResponse" or "ActionServiceInvokeActionResponse".
[failure] 14-14:
RPC request type "ActionServiceStatusRequest" should be named "GetActionStatusRequest" or "ActionServiceGetActionStatusRequest".
[failure] 14-14:
RPC response type "ActionServiceStatusResponse" should be named "GetActionStatusResponse" or "ActionServiceGetActionStatusResponse".
[failure] 15-15:
RPC request type "ActionServiceGetSchemaRequest" should be named "GetActionSchemaRequest" or "ActionServiceGetActionSchemaRequest".
[failure] 15-15:
RPC response type "ActionServiceGetSchemaResponse" should be named "GetActionSchemaResponse" or "ActionServiceGetActionSchemaResponse".
[failure] 16-16:
RPC request type "ActionServiceListSchemasRequest" should be named "ListActionSchemasRequest" or "ActionServiceListActionSchemasRequest".
[failure] 16-16:
RPC response type "ActionServiceListSchemasResponse" should be named "ListActionSchemasResponse" or "ActionServiceListActionSchemasResponse".
🔇 Additional comments (26)
pkg/sync/syncer_test.go (1)
458-458: Consider adding test coverage for the new action service functionality.The
ActionServiceClientinterface has been added to themockConnectorstruct, but there are no test cases covering this new functionality.Consider adding test cases to verify the integration and behavior of the action service in the sync process.
pkg/types/types.go (1)
24-24: LGTM!The addition of
ActionServiceServerandActionServiceClientinterfaces is consistent and maintains symmetry between server and client interfaces.Also applies to: 41-41
pkg/tasks/c1api/manager.go (1)
274-281: LGTM!The new action-related task types and their handlers are well-integrated into the task processing flow, following the established pattern.
pkg/types/tasks/tasks.go (1)
53-60: LGTM!The new action-related task types and their string representations are well-defined and follow the established pattern.
Also applies to: 91-94
internal/connector/connector.go (2)
49-49: Addition ofActionServiceClientfield appears consistent.
This new field aligns well with the existing pattern of embedding service clients inconnectorClient.
363-363: Initialization ofActionServiceClientis correctly integrated.
Instantiating the newActionServiceClientwithcw.connfollows the same approach as the other service clients and should work as expected.proto/c1/connectorapi/baton/v1/baton.proto (3)
12-12: Import ofgoogle/protobuf/struct.protois appropriate.
This import is needed to support structured arguments in the new action messages.
112-131: New action task definitions look consistent.
These messages follow the existing pattern of tasks, each usingrepeated google.protobuf.Anyfor annotations. Verify that any required validation or processing logic for these new tasks is present in the downstream handlers.
151-154: Integration into theoneof task_typeis correct.
Referencing the new action tasks within the existingoneofblock ensures they work seamlessly with the existing task dispatch.pkg/connectorbuilder/connectorbuilder.go (6)
17-17: Import ofstructpbsupports structured data.
This import is required to handle complex argument structures in action invocations.
84-89:ActionManagerinterface addition is well-structured.
Defining clear methods for listing, fetching, invoking actions, and checking their status keeps the API cohesive.
103-103: Addition of theactionManagerfield is consistent.
Storing theActionManagerat the builder level follows the pattern used for other managers (e.g.,ticketManager).
385-390: Logic for detecting and setting anActionManagermirrors other managers.
Preventing duplicates is a good safeguard to avoid conflicts.
1034-1058: Fetching a single action schema is properly implemented.
The method handles errors consistently, recording success or failure as needed.
1087-1111: ModelingGetActionStatusmatches the rest of the design.
This method provides clear success/failure metric tracking and is parallel in workflow to the other methods.pb/c1/action/v1/action.pb.validate.go (1)
1-373: Auto-generated validation code looks good.This appears to be standard protoc-gen-validate output, and the generated validation logic is consistent with typical usage. No issues stand out from a correctness or security perspective.
pb/c1/connector/v2/action.pb.validate.go (1)
1-1449: Auto-generated validation code looks good.Similar to the previous file, this is standard protoc-gen-validate output. The validation approach is consistent and aligns with best practices for generated code.
proto/c1/connector/v2/action.proto (1)
9-9:✅ Verification successful
Confirm single go_package usage.
Previous reviews flagged multiple go_package values in this package. It appears you've set only one go_package option now. Please verify that no additional go_package lines exist in this file to avoid lint conflicts.
🏁 Script executed:
#!/bin/bash # Check for multiple go_package occurrences in this file rg --context 2 'option go_package' proto/c1/connector/v2/action.protoLength of output: 186
Confirmed single go_package in proto/c1/connector/v2/action.proto
After running the verification script, only one occurrence of the
option go_packageline was found in the file:
option go_package = "github.com/conductorone/baton-sdk/pb/c1/connector/v2";There are no additional
go_packagestatements, and the file complies with linting requirements.pb/c1/connectorapi/baton/v1/baton.pb.validate.go (8)
681-721: Consistent addition forTask_ActionListSchemasin the switch statement.
These lines closely mirror the established validation pattern for other oneof cases, correctly handling nil checks and embedded message validation.
722-761: Proper oneof handling forTask_ActionGetSchema.
The logic for detecting typed-nil and optionally invokingValidateAllorValidateis consistent with the rest of the file.
763-803: No issues withTask_ActionInvokecase.
This snippet properly checks forniland proceeds with embedded message validation, in line with other branches.
804-843: Validation flow is coherent forTask_ActionStatus.
Similar checks for typed-nil and embedded validation ensure consistency with the rest of the switch.
5354-5490:Task_ActionListSchemasTaskvalidation looks correct.
The code properly iterates over repeated fields, performs embedded checks, and returns multi-errors if needed.
5491-5627:Task_ActionGetSchemaTaskvalidation mirrors existing patterns.
All relevant validations, including annotations and embedded messages, are handled consistently.
5628-5794:Task_ActionInvokeTaskvalidation is inline with the canonical approach.
The block correctly handles optional struct validation forArgsand repeated annotations.
5795-5934:Task_ActionStatusTaskvalidation follows the same robust structure.
Checks for nil fields, annotations, and embedded messages remain aligned with the established auto-generated style.
proto/c1/connector/v2/action.proto
Outdated
| service ActionService { | ||
| rpc InvokeAction(ActionServiceInvokeRequest) returns (ActionServiceInvokeResponse); | ||
| rpc GetActionStatus(ActionServiceStatusRequest) returns (ActionServiceStatusResponse); | ||
| rpc GetActionSchema(ActionServiceGetSchemaRequest) returns (ActionServiceGetSchemaResponse); | ||
| rpc ListActionSchemas(ActionServiceListSchemasRequest) returns (ActionServiceListSchemasResponse); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename RPC request/response message types to align with recommended naming conventions.
Static analysis suggests renaming these request and response messages to comply with standard RPC naming guidelines (e.g., InvokeActionRequest, InvokeActionResponse, etc.). Adopting these names ensures better consistency and may prevent lint failures.
Consider applying a diff like this:
service ActionService {
- rpc InvokeAction(ActionServiceInvokeRequest) returns (ActionServiceInvokeResponse);
- rpc GetActionStatus(ActionServiceStatusRequest) returns (ActionServiceStatusResponse);
- rpc GetActionSchema(ActionServiceGetSchemaRequest) returns (ActionServiceGetSchemaResponse);
- rpc ListActionSchemas(ActionServiceListSchemasRequest) returns (ActionServiceListSchemasResponse);
+ rpc InvokeAction(InvokeActionRequest) returns (InvokeActionResponse);
+ rpc GetActionStatus(GetActionStatusRequest) returns (GetActionStatusResponse);
+ rpc GetActionSchema(GetActionSchemaRequest) returns (GetActionSchemaResponse);
+ rpc ListActionSchemas(ListActionSchemasRequest) returns (ListActionSchemasResponse);
}📝 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.
| service ActionService { | |
| rpc InvokeAction(ActionServiceInvokeRequest) returns (ActionServiceInvokeResponse); | |
| rpc GetActionStatus(ActionServiceStatusRequest) returns (ActionServiceStatusResponse); | |
| rpc GetActionSchema(ActionServiceGetSchemaRequest) returns (ActionServiceGetSchemaResponse); | |
| rpc ListActionSchemas(ActionServiceListSchemasRequest) returns (ActionServiceListSchemasResponse); | |
| } | |
| service ActionService { | |
| rpc InvokeAction(InvokeActionRequest) returns (InvokeActionResponse); | |
| rpc GetActionStatus(GetActionStatusRequest) returns (GetActionStatusResponse); | |
| rpc GetActionSchema(GetActionSchemaRequest) returns (GetActionSchemaResponse); | |
| rpc ListActionSchemas(ListActionSchemasRequest) returns (ListActionSchemasResponse); | |
| } |
🧰 Tools
🪛 Buf (1.47.2)
13-13: RPC request type "ActionServiceInvokeRequest" should be named "InvokeActionRequest" or "ActionServiceInvokeActionRequest".
(RPC_REQUEST_STANDARD_NAME)
13-13: RPC response type "ActionServiceInvokeResponse" should be named "InvokeActionResponse" or "ActionServiceInvokeActionResponse".
(RPC_RESPONSE_STANDARD_NAME)
14-14: RPC request type "ActionServiceStatusRequest" should be named "GetActionStatusRequest" or "ActionServiceGetActionStatusRequest".
(RPC_REQUEST_STANDARD_NAME)
14-14: RPC response type "ActionServiceStatusResponse" should be named "GetActionStatusResponse" or "ActionServiceGetActionStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
15-15: RPC request type "ActionServiceGetSchemaRequest" should be named "GetActionSchemaRequest" or "ActionServiceGetActionSchemaRequest".
(RPC_REQUEST_STANDARD_NAME)
15-15: RPC response type "ActionServiceGetSchemaResponse" should be named "GetActionSchemaResponse" or "ActionServiceGetActionSchemaResponse".
(RPC_RESPONSE_STANDARD_NAME)
16-16: RPC request type "ActionServiceListSchemasRequest" should be named "ListActionSchemasRequest" or "ActionServiceListActionSchemasRequest".
(RPC_REQUEST_STANDARD_NAME)
16-16: RPC response type "ActionServiceListSchemasResponse" should be named "ListActionSchemasResponse" or "ActionServiceListActionSchemasResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 13-13:
RPC request type "ActionServiceInvokeRequest" should be named "InvokeActionRequest" or "ActionServiceInvokeActionRequest".
[failure] 13-13:
RPC response type "ActionServiceInvokeResponse" should be named "InvokeActionResponse" or "ActionServiceInvokeActionResponse".
[failure] 14-14:
RPC request type "ActionServiceStatusRequest" should be named "GetActionStatusRequest" or "ActionServiceGetActionStatusRequest".
[failure] 14-14:
RPC response type "ActionServiceStatusResponse" should be named "GetActionStatusResponse" or "ActionServiceGetActionStatusResponse".
[failure] 15-15:
RPC request type "ActionServiceGetSchemaRequest" should be named "GetActionSchemaRequest" or "ActionServiceGetActionSchemaRequest".
[failure] 15-15:
RPC response type "ActionServiceGetSchemaResponse" should be named "GetActionSchemaResponse" or "ActionServiceGetActionSchemaResponse".
[failure] 16-16:
RPC request type "ActionServiceListSchemasRequest" should be named "ListActionSchemasRequest" or "ActionServiceListActionSchemasRequest".
[failure] 16-16:
RPC response type "ActionServiceListSchemasResponse" should be named "ListActionSchemasResponse" or "ActionServiceListActionSchemasResponse".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/tasks/c1api/actions.go (4)
27-37: Adequate error handling for list action schemas.
The logic for callingListActionSchemasand finishing the task on error is appropriate. Ensure timeouts or cancellations via thectxare respected further up the call stack for reliability.
71-73: Provide more actionable context in user-facing errors.
When the action name is missing, the error is “action name required.” It can be helpful to display which action was attempted for ease of debugging (e.g., referencing the current task or code path).- return c.helpers.FinishTask(ctx, nil, nil, errors.New("action name required")) + return c.helpers.FinishTask(ctx, nil, nil, fmt.Errorf("action name required for task %q", c.task.GetId()))
116-118: Enforce argument presence with clearer context.
Similar to action names, a descriptive message for missing arguments can help debugging. Consider referencing the missing argument fields.
152-162: Ensure consistent error log naming.
The debug line labels this as"ActionInvoke response"instead of something like"ActionStatus response". For clarity and uniformity, use consistent naming in log statements.- l.Debug("ActionInvoke response", zap.Any("resp", resp)) + l.Debug("ActionStatus response", zap.Any("resp", resp))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
pb/c1/connector/v2/action.pb.validate.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(7 hunks)pkg/tasks/c1api/actions.go(1 hunks)proto/c1/connector/v2/action.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- proto/c1/connector/v2/action.proto
- pkg/connectorbuilder/connectorbuilder.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (4)
proto/c1/connector/v2/connector.proto (1)
62-62: Confirm usage of the newly added capability.
It's good practice to verify that all references toCAPABILITY_ACTIONSproperly handle this new capability, especially within any capability checks or feature-gating logic.pb/c1/connector/v2/action.pb.validate.go (3)
1-3: Auto-generated validation code detected.
These lines indicate that the file is generated byprotoc-gen-validate. Typically, no manual changes are necessary. Adjustments should be made in your protobuf definitions instead, then regenerate this file.
38-43: Validation approach is clear and consistent.
TheValidate()method and its subsequent calls are straightforward. The flow for error accumulation aligns well with the auto-generated style. No concerns here; just ensure any custom validations are handled in your proto definitions.
164-169: Multi-error handling is comprehensive.
The usage ofBatonActionSchemaMultiErrorto collect all validation errors is a solid approach. This pattern allows clients to address multiple issues in one pass. No further adjustments needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connectorbuilder/connectorbuilder.go (1)
391-397: Clean up commented codeThis commented-out section suggests uncertainty about the architectural decision of whether ActionManager should be implemented at the connector or resource level. Since you've implemented it at the connector level (lines 340-345), this commented code should be removed to avoid confusion.
- // This should be on the connector level? - // if actionManager, ok := rb.(ActionManager); ok { - // if ret.actionManager != nil { - // return nil, fmt.Errorf("error: duplicate resource type found for action manager %s", rType.Id) - // } - // ret.actionManager = actionManager - //}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
internal/connector/connector.go(3 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)pkg/tasks/tasks.go(2 hunks)proto/c1/connector/v2/action.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/connector/connector.go
- proto/c1/connector/v2/action.proto
🔇 Additional comments (9)
pkg/tasks/tasks.go (2)
61-68: New action types added appropriately.The implementation correctly adds checks for the new action-related task types (ActionListSchemas, ActionGetSchema, ActionInvoke, ActionStatus) in the
Isfunction, following the established pattern.
112-119: Task type mappings look good.The
GetTypefunction is properly extended with corresponding case statements for the new action-related tasks, maintaining consistency with the existing implementation pattern.pkg/connectorbuilder/connectorbuilder.go (6)
84-89: Well-designed ActionManager interfaceThe interface is cleanly designed with four essential methods covering the complete lifecycle of actions: listing schemas, getting schema details, invoking actions, and checking action status. The signature properly handles both synchronous returns and annotations for extensibility.
694-697: LGTM: Proper capability registrationThe implementation correctly adds the
CAPABILITY_ACTIONScapability to both connector-level capabilities and resource-type capabilities when appropriate.
1021-1045: Consistent implementation of ListActionSchemasThe method follows the established pattern in the codebase for manager methods, with appropriate tracing, error handling, and metrics recording.
1047-1071: Consistent implementation of GetActionSchemaThe implementation follows the same pattern as other methods with proper context handling, error reporting, and metrics.
1073-1099: Consistent implementation of InvokeActionThe method properly delegates to the action manager and handles results and errors according to the established pattern.
1101-1125: Consistent implementation of GetActionStatusThis method completes the action management lifecycle implementation with consistent error handling and metrics recording.
pb/c1/connector/v2/action.pb.validate.go (1)
1-1470: LGTM: Well-structured auto-generated validation codeThis validation code follows the standard pattern for Protocol Buffers validation. Since this is auto-generated code, no modifications are needed. The file properly implements validation for all the action-related message types and their fields, with comprehensive error handling patterns.
Each message type has both
Validate()andValidateAll()methods that perform validation at different levels of detail, along with appropriate error types and interfaces for consistent error reporting.
ggreer
force-pushed
the
ggreer/baton-actions
branch
from
d621285 to
6c2a97e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/connectorbuilder/connectorbuilder.go (1)
391-397: Consider removing commented-out code.The commented-out code seems to be a design decision that was reconsidered (moving from resource-level to connector-level action management), but it's now just adding noise to the codebase.
Consider removing this commented block since the decision has already been implemented at the connector level (lines 340-345).
pkg/tasks/c1api/actions.go (4)
173-173: Correct the status handler's debug log message.This line logs
"ActionInvoke response"in theactionStatusTaskHandler. Likely a copy-paste oversight. Consider updating it to reflect the correct handler name, e.g.,"ActionStatus response".Apply this diff to fix the message:
-l.Debug("ActionInvoke response", zap.Any("resp", resp)) +l.Debug("ActionStatus response", zap.Any("resp", resp))
17-20: Consider reducing repetitive code across multiple interfaces.Each task’s helpers interface declares the same
ConnectorClient()andFinishTask(...)functions. You could reduce duplication by defining a single shared interface for these methods if there is no strong reason to separate them per task type.Also applies to: 52-55, 94-97, 141-144
27-43: Refactor common handler logic to improve maintainability.All four handlers share a similar flow:
- Start a span,
- Extract the logger,
- Access fields from
c.task,- Make a client call,
- Log the response,
- Finish the task.
Consider extracting common logic into a helper to avoid duplication and make the code more modular.
Also applies to: 62-85, 104-132, 151-176
1-184: Add or update unit tests for new handlers.These handlers contain critical workflow logic. Ensure comprehensive test coverage (including success conditions and error scenarios triggered by missing fields like
action nameoraction id) to validate correctness and reduce regressions.Would you like help creating a new test suite or adding cases to existing tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
internal/connector/connector.go(3 hunks)pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)pkg/sync/syncer_test.go(1 hunks)pkg/tasks/c1api/actions.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/action.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connectorapi/baton/v1/baton.proto(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- proto/c1/connector/v2/connector.proto
- pkg/types/types.go
- pkg/tasks/tasks.go
- pkg/tasks/c1api/manager.go
- pkg/sync/syncer_test.go
- internal/connector/connector.go
- proto/c1/connectorapi/baton/v1/baton.proto
- proto/c1/connector/v2/action.proto
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (15)
pkg/types/tasks/tasks.go (2)
53-60: Well-structured task type additions.The new action-related task types are properly integrated into the
String()method with descriptive return values that follow the established naming convention.
91-94: Consistent enum constant additions.The task type constants are added sequentially following the existing pattern, maintaining the correct iota-based enumeration approach.
pkg/connectorbuilder/connectorbuilder.go (9)
84-89: Well-defined ActionManager interface.The interface provides a clear set of methods for action management with appropriate parameter and return types. The method signatures align well with the action lifecycle (listing schemas, getting schema details, invoking actions, and checking status).
103-103: Field addition follows struct field organization.The
actionManagerfield is appropriately positioned among other manager fields in thebuilderImplstruct.
313-313: Proper initialization in NewConnector.The
actionManagerfield is correctly initialized tonil, consistent with how other optional managers are initialized.
340-345: Robust type detection and assignment.The code properly checks if the connector implements the ActionManager interface and ensures only one action manager can be set, consistent with other manager initialization patterns.
694-697: Complete capability detection for actions.The capability detection logic correctly sets the action capability at both the connector level and resource type level when a resource implements ActionManager.
1021-1045: Thoroughly implemented ListActionSchemas method.The method follows the established pattern for task handling:
- Creates a span for tracing
- Checks if the action manager is implemented
- Records metrics for both success and failure cases
- Provides clear error messages
- Returns a properly formatted response
1047-1071: GetActionSchema implementation follows consistent patterns.The method correctly matches other similar methods in error handling, metrics recording, and response formatting.
1073-1099: InvokeAction handles action responses appropriately.The method correctly captures all return values from the action manager including ID, status, response data, and annotations.
1101-1125: GetActionStatus implementation completes the action lifecycle.The method properly implements status checking for previously invoked actions, maintaining the consistent pattern of error handling and metrics recording.
pb/c1/action/v1/action.pb.validate.go (1)
1-373: Auto-generated validation code looks correct.This is auto-generated validation code for Protocol Buffer messages that properly implements validation for
BatonActionandBatonActionSchematypes. The code follows standard protoc-gen-validate patterns with appropriate validation logic for nested fields.pb/c1/connector/v2/action.pb.validate.go (1)
1-1470: Comprehensive request/response validation code.This auto-generated file properly implements validation for all action-related request and response messages. Each message type has both
Validate()andValidateAll()methods with appropriate error handling for both individual and aggregated validation errors.pb/c1/connectorapi/baton/v1/baton.pb.validate.go (2)
681-844: New validation logic for Action tasks follows consistent patterns.The additions for
Task_ActionListSchemas,Task_ActionGetSchema,Task_ActionInvoke, andTask_ActionStatusvalidation follow the same patterns used throughout the file, with proper nil checks and validation of embedded messages.
5354-5933: Task validation implementations are correctly structured.The implementations for
Task_ActionListSchemasTask,Task_ActionGetSchemaTask,Task_ActionInvokeTask, andTask_ActionStatusTaskvalidation follow the established pattern in the codebase. Each has proper:
- Null pointer checks
- Validation of annotations
- Field-specific validation where needed
- Consistent error handling
The validation functions properly distinguish between validating just the first error (
Validate()) and validating all fields (ValidateAll()).
| } | ||
|
|
||
| func (c *actionListSchemasTaskHandler) HandleTask(ctx context.Context) error { | ||
| ctx, span := tracer.Start(ctx, "actionListSchemasTaskHandler.HandleTask") |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify the tracer import or definition.
All four task handlers invoke tracer.Start(...), but there is no visible import for tracer. Ensure that the correct package is imported or that tracer is defined in the codebase to avoid compile-time errors.
Please confirm if there's an import like go.opentelemetry.io/otel or a local tracer package. If it’s missing, the following script can help locate references to tracer outside this file:
Also applies to: 63-63, 105-105, 152-152
🏁 Script executed:
#!/bin/bash
# Search for definitions of `tracer` in the Go codebase:
rg "tracer\."Length of output: 17163
Action Required: Confirm Tracer Import/Definition Consistency
The task handler in pkg/tasks/c1api/actions.go (line 28) calls tracer.Start(...) without a directly visible import or local declaration. Although the repository-wide search shows multiple uses of tracer.Start(...), please verify that a proper tracer instance is provided – either via an explicit import (e.g., from go.opentelemetry.io/otel) or a local/global definition.
- Check that this file (and similar ones: lines 63, 105, 152) imports the same tracer package or has a local definition consistent with the other modules.
- Ensure that any alias (if used) aligns with the project's tracing implementation.
ggreer
force-pushed
the
ggreer/baton-actions
branch
from
a78637c to
cfd20fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/connectorbuilder/connectorbuilder.go (2)
84-89: Well-defined ActionManager interface
The interface cleanly declares methods for listing, retrieving, invoking, and checking status of actions. Adding short doc comments for each method would be helpful for clarity.
1065-1091: InvokeAction method
The design integrates well with the manager and logs appropriate failures/success. Consider adding validation for the action name or arguments to avoid unexpected action calls.pkg/tasks/c1api/actions.go (2)
22-43: Consider reducing repetitive code in the task handlers.Each
HandleTaskmethod follows a similar pattern:
- Start a tracer span.
- Extract logger.
- Retrieve the connector client.
- Validate input fields.
- Call the relevant connector method.
- Use
FinishTaskto finalize results.Extracting this pattern into a shared helper or a base type would simplify future maintenance and reduce code duplication.
Also applies to: 57-85, 99-132, 146-176
173-173: Fix the incorrect log message.Within
actionStatusTaskHandler.HandleTask, the debug message incorrectly refers to "ActionInvoke response" despite being in the "Action Status" context. This can be confusing for debugging and tracing.Consider applying this fix:
- l.Debug("ActionInvoke response", zap.Any("resp", resp)) + l.Debug("ActionStatus response", zap.Any("resp", resp))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
internal/connector/connector.go(3 hunks)pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)pkg/sync/syncer_test.go(1 hunks)pkg/tasks/c1api/actions.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/action.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connectorapi/baton/v1/baton.proto(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/types/types.go
- pkg/sync/syncer_test.go
- proto/c1/connector/v2/connector.proto
- pkg/tasks/tasks.go
- pkg/types/tasks/tasks.go
- pkg/tasks/c1api/manager.go
- internal/connector/connector.go
- proto/c1/connectorapi/baton/v1/baton.proto
- proto/c1/connector/v2/action.proto
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (18)
pkg/connectorbuilder/connectorbuilder.go (7)
17-17: New import added for structpb
This import is necessary to handle protobuf Struct types and appears consistent with the usage in the newly introduced action methods.
103-103: Added actionManager field to builderImpl
Storing the ActionManager in the builderImpl structure aligns with the pattern used by other managers (e.g., ticketManager).
340-346: Conditional assignment of actionManager
Ensuring only one actionManager is set prevents conflicts. This is consistent with how ticketManager is handled.
686-686: Minor change on closing brace
No functional impact; likely a formatting or minor change.
1012-1037: ListActionSchemas method
The flow follows the established pattern of checking for an implemented manager, calling the manager method, recording success/failure metrics, and returning results. Error handling appears solid.
1039-1063: GetActionSchema method
Consistent approach to retrieving an action schema. The error handling and metrics recording match existing patterns.
1093-1117: GetActionStatus method
Implementation follows the same consistent error and metrics pattern. No immediate issues noted.pb/c1/action/v1/action.pb.validate.go (1)
1-373: Auto-generated validation code
These lines are generated by protoc-gen-validate. Manual changes are typically discouraged; no significant review concerns.pb/c1/connector/v2/action.pb.validate.go (1)
1-1470: Auto-generated validation code
These lines are also generated by protoc-gen-validate. Adjustments should be made at the proto file level if any changes are needed.pkg/tasks/c1api/actions.go (1)
28-28: Confirm thetracerimport or definition.All four task handlers call
tracer.Start(...)(lines 28, 63, 105, and 152), but notracerimport or definition is visible in this file. This may cause a compile-time error if thetracerpackage or object does not exist or is not imported correctly in the codebase. Please confirm that the required package is imported and that it provides the appropriateStart(...)function.Also applies to: 63-63, 105-105, 152-152
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (8)
681-721: LGTM!
The new case forActionListSchemasaligns with the existing pattern for validating oneof submessages, performing the appropriateValidateAll/Validatecalls and nil checks.
722-762: LGTM!
This case forActionGetSchemaconsistently follows the same patterns of validation as the other tasks, ensuring optional error aggregation.
763-803: LGTM!
The new case forActionInvokeproperly checks for typed-nil and validates the embedded message, mirroring existing approach for other tasks.
804-844: LGTM!
TheActionStatuscase is consistent with the prior logic, including typed-nil checks and an invocation ofValidate()orValidateAll().
5354-5489: No issues found.
The generated validation code forTask_ActionListSchemasTaskis aligned with protoc-gen-validate’s standard approach, checking for nil references and performing field-level validations.
5490-5627: Looks Great.
The validation methods forTask_ActionGetSchemaTaskare consistent with the plugin’s conventions, handling annotations and field checks as expected.
5628-5794: Validation logic is consistent.
Task_ActionInvokeTaskuses the same well-established code pattern, ensuring correct embedded message validation and error aggregation.
5795-5934: Well-structured validation routines.
Task_ActionStatusTaskfollows the same blueprint, performing range checks, repeated-field validations, and submessage validations appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
pkg/connectorbuilder/connectorbuilder.go (5)
84-89: Consider adding doc comments for clarity and concurrency implications.
TheCustomActionManagerinterface is straightforward, but you may want to clarify whether implementations are expected to be thread-safe. Adding a doc comment can guide implementers and maintainers on concurrency requirements and potential side effects.
351-363: Robust registration flow.
The defensive checks against setting multiple managers and the nil-manager check are commendable. Consider adding a debug log if registration succeeds, similar to how resource managers are handled.
719-722: Enabling the ACTIONS capability is consistent with existing patterns.
When anactionManageris present, addingCAPABILITY_ACTIONSto the connector capabilities seamlessly extends the feature set. A debug log or doc comment might help clarify that this capability is conditionally enabled.
1030-1055: Add pagination or filtering if large sets are expected.
ListActionSchemasdumps all schemas without pagination. If the number of action schemas grows large, consider implementing paging for scalability.
1083-1109: Assess concurrency for multiple InvokeAction calls.
Consider whether concurrent invokes could conflict or overwhelm theactionManager. If concurrency is expected, ensure your manager or the invoked actions are thread-safe.pkg/actions/actions.go (7)
16-16: Straightforward SyncActionHandler.
This is a clean signature for synchronous action handling. If tasks can be long-running, consider clarifying expected timeouts or performance constraints.
22-27: Async handler design is incomplete.
TheAsyncActionHandlerinterface outlines the basics for async workflows, but it’s partially commented out above. If you plan to finalize async support, ensure consistent naming and usage across Start, Status, and Finish.Would you like help drafting a more complete async implementation pattern?
29-34: Type alias and struct for outstanding actions are straightforward.
This design sets up a foundation to track each active action. If concurrency is needed, remember to protect simultaneous access toOutstandingAction.
36-41: Clear internal structure for managing actions.
Storing actions in maps is fine for small-scale usage. For large volumes or distributed usage, you might consider a more robust store (e.g., DB or external cache).
56-79: Registration logic is robust.
The checks for empty name, nil schema, or nil handler prevent misconfiguration. Consider adding doc comments to clarify that it is not safe to re-register the same name.
81-88: ListActionSchemas returns all schemas.
Implementation is straightforward. If you expect many schemas, you may want to consider paging or filtering.
118-142: InvokeAction approach is workable for sync tasks.
The logic ensures the action is tracked, invoked, and promptly removed from outstanding actions when complete. Be mindful that truly asynchronous tasks may require the action to remain outstanding longer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (12)
pkg/connectorbuilder/connectorbuilder.go (6)
17-17: No concerns with the new import.
This import ofstructpbappears necessary for passing structured arguments to your custom action interface.
91-93: Naming of interface is clear.
No immediate issues with theRegisterActionManagerinterface; it’s succinct and purposeful.
107-107: Field initialization is consistent with the new interfaces.
This addition cleanly introduces theactionManagerfield, aligning with the newly addedCustomActionManagerinterface.
344-349: Logic effectively prevents multiple action managers.
Restricting the implementation to a singleCustomActionManagerensures that there’s no conflicting or overlapping logic from different managers.
1057-1081: Sufficient error handling.
TheGetActionSchemamethod properly surfaces an error if unavailable. Your logic to delegate the actual retrieval tob.actionManageris aligned with the rest of the builder.
1111-1135: Gets action status effectively.
Fetching the action status fromb.actionManagerfollows a consistent pattern. Your tracking of tasks metrics also looks good.pkg/actions/actions.go (6)
1-2: Package declaration looks good.
No issues found with package name or statements.
3-14: Imports are appropriate.
All imported packages align with the file’s functionalities. No redundant or missing imports detected.
43-49: Constructor is clear.
All maps are properly initialized, preventing nil map errors later on.
51-54: Simple ID generation.
Using an incrementinguint64counters is quick and understandable. Consider concurrency restrictions if multiple goroutines can call this simultaneously.
90-96: Fetching schema by name is direct and effective.
No concerns here. The error message helps identify unknown schemas.
98-116: Status retrieval is set for future async expansion.
Right now, the method returnsRUNNINGorUNKNOWN. Once async functionality is implemented, hooking in theAsyncActionHandler.Status()logic here will be straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/actions/actions.go (1)
1-149: 🛠️ Refactor suggestionConsider concurrency control on maps.
The
ActionManagerstruct uses maps without locking. If invoked concurrently, it may lead to data races. Adding a mutex lock or using sync-safe structures will improve thread safety.
🧹 Nitpick comments (2)
pkg/connectorbuilder/connectorbuilder.go (2)
84-89: Add doc comments for interface methods.Providing method-level doc comments in the
CustomActionManagerinterface will improve clarity for implementers and maintainers.
91-93: Add doc comments for interface methods.Similarly, adding doc comments for the
RegisterActionManagerinterface clarifies usage and design intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
pb/c1/connector/v2/action.pb.validate.go(1 hunks)pkg/actions/actions.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)proto/c1/connector/v2/action.proto(1 hunks)
🔇 Additional comments (10)
pkg/connectorbuilder/connectorbuilder.go (8)
17-17: No issues with the new import.
107-107: New field for action manager.No issues; this neatly integrates with the newly introduced interfaces.
344-349: Check for existing custom action managers.Logic is correct to prevent multiple action managers from being set.
351-363: Register action manager dynamically.The approach of registering via
RegisterActionManageris consistent and prevents multiple registrations.
1031-1055: ListActionSchemas implementation.Consistently checks if the action manager is available and records metrics correctly.
1057-1081: GetActionSchema implementation.Straightforward usage of the
actionManagerwith clear error handling.
1083-1109: InvokeAction implementation.Properly delegates the invocation to the
actionManager, recording success/failure.
1111-1138: GetActionStatus method.Consistent usage pattern, no apparent issues.
pb/c1/connector/v2/action.pb.validate.go (1)
1-1499: Generated validation code.This file is auto-generated by protoc-gen-validate. Modifications should happen in the
.protodefinitions as needed. Otherwise, no issues observed.proto/c1/connector/v2/action.proto (1)
1-82: New ActionService definitions.The service and messages align well with typical gRPC patterns. The
go_packageoption looks correct and consistent.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pkg/actions/actions_test.go (2)
16-33: Schema naming consistency and clarity.
ThetestActionSchemafor "lock_account" is well-defined and clear. Consider documenting the "dn" (Distinguished Name) usage if it’s domain-specific. This can prevent confusion for future maintainers.
53-90: Expand test coverage for timeouts and concurrency.
Currently, the test checks happy path and missing argument scenarios. Consider adding a case where the action handler intentionally runs longer than the 1-second threshold to confirm that the status remains "PENDING" and is updated later. This will ensure robust coverage of the async invocation mechanism.pkg/actions/actions.go (2)
84-100: Unregistration logic is complete but can affect outstanding actions.
A future improvement might be to cancel or gracefully handle any outstanding actions referencing this schema/handler before removing them from the map (per the TODO on line 97).
102-109: Schema listing is straightforward.
The iteration approach is correct, returning the entire slice in no particular order. If ordering becomes important, consider sorting the results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.go(1 hunks)pkg/actions/actions_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: go-test (1.22.x, windows-latest)
- GitHub Check: go-lint
🔇 Additional comments (6)
pkg/actions/actions_test.go (1)
1-16: Good initial structure and imports.
All imported packages look necessary and aligned with the testing goals.pkg/actions/actions.go (5)
19-28: Neat definition of ActionHandler and OutstandingAction.
This code is clear, and each field is well-defined. Make sure to keep concurrency in mind if these fields are updated from multiple goroutines.
37-48: Constructor follows best practices.
TheNewActionManager()function is straightforward. Once concurrency mechanisms are introduced, consider ensuring that concurrency primitives are also initialized here if needed.
50-62: Schema registration checks are appropriate.
The validation for empty names, nil schemas, and duplicates helps prevent bad registrations. No major issues here.
64-82: Handler registration and duplicate checks look good.
The checks before assigning the handler seem comprehensive. No immediate concerns besides concurrency.
111-117: Specific schema retrieval meets requirements.
Error handling is aligned with gRPC’s NotFound logic. Nice usage of typed status codes.
| func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { | ||
| _, ok := args.Fields["dn"].GetKind().(*structpb.Value_StringValue) | ||
| if !ok { | ||
| return nil, nil, fmt.Errorf("missing dn") | ||
| } |
There was a problem hiding this comment.
Potential nil-pointer panic due to missing map key.
Calling args.Fields["dn"].GetKind() can panic if "dn" is absent from the map (i.e., args.Fields["dn"] may be nil). You can prevent this by checking for field existence first:
func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) {
- _, ok := args.Fields["dn"].GetKind().(*structpb.Value_StringValue)
- if !ok {
+ dnVal, ok := args.Fields["dn"]
+ if !ok || dnVal == nil {
return nil, nil, fmt.Errorf("missing dn")
}
...
}📝 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 testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { | |
| _, ok := args.Fields["dn"].GetKind().(*structpb.Value_StringValue) | |
| if !ok { | |
| return nil, nil, fmt.Errorf("missing dn") | |
| } | |
| func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { | |
| dnVal, ok := args.Fields["dn"] | |
| if !ok || dnVal == nil { | |
| return nil, nil, fmt.Errorf("missing dn") | |
| } | |
| ... | |
| } |
pkg/actions/actions.go
Outdated
| // TODO: use syncmaps here | ||
| type ActionManager struct { | ||
| actionId uint64 | ||
| schemas map[string]*v2.BatonActionSchema // map of action name to schema | ||
| handlers map[string]ActionHandler | ||
| actions map[string]OutstandingAction // map of actions IDs | ||
| } |
There was a problem hiding this comment.
Consider concurrency controls for ActionManager fields.
The use of plain maps and the actionId counter without locking can cause data races if accessed from multiple goroutines. Since there is a TODO for syncmaps, expect concurrency safety to come soon.
Would you like help integrating sync.Map or standard mutex locking to ensure thread safety?
pkg/actions/actions.go
Outdated
| func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { | ||
| oa, ok := a.actions[actionId] | ||
| if !ok { | ||
| return v2.BatonActionStatus_BATON_ACTION_STATUS_UNKNOWN, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("action id %s not found", actionId)) | ||
| } | ||
|
|
||
| return oa.Status, oa.Rv, nil, nil | ||
| } |
There was a problem hiding this comment.
Status retrieval not updated for concurrency.
Storing the final result in a.actions[actionId] requires concurrency-safe access. If multiple goroutines read & write concurrently, data races can occur.
pkg/actions/actions.go
Outdated
| func (a *ActionManager) InvokeAction(ctx context.Context, name string, args *structpb.Struct) (string, v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { | ||
| handler, ok := a.handlers[name] | ||
| if !ok { | ||
| return "", v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("handler for action %s not found", name)) | ||
| } | ||
|
|
||
| actionId := a.GetNewActionId() | ||
| oa := OutstandingAction{ | ||
| Name: name, | ||
| Status: v2.BatonActionStatus_BATON_ACTION_STATUS_PENDING, | ||
| } | ||
| a.actions[actionId] = oa | ||
|
|
||
| done := make(chan struct{}) | ||
|
|
||
| // If handler exits within a second, return result. | ||
| // If handler takes longer than a second, return status pending. | ||
| go func() { | ||
| oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING | ||
| oa.Rv, oa.Annos, oa.Err = handler(ctx, args) | ||
| if oa.Err == nil { | ||
| oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE | ||
| } else { | ||
| oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED | ||
| } | ||
| done <- struct{}{} | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| return actionId, oa.Status, oa.Rv, oa.Annos, oa.Err | ||
| case <-time.After(1 * time.Second): | ||
| return actionId, oa.Status, oa.Rv, oa.Annos, oa.Err | ||
| case <-ctx.Done(): | ||
| return actionId, v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED, nil, nil, ctx.Err() | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race and incomplete status updates.
Inside InvokeAction, the goroutine modifies oa (status, error, etc.), but a.actions[actionId] is never overwritten with the final oa. Consequently, calls to GetActionStatus may see stale data. For full correctness, reassign or switch to pointer-based storage:
- a.actions[actionId] = oa
+ a.actions[actionId] = &oa
...
// Then in the goroutine:
- oa.Status = ...
+ a.actions[actionId].Status = ...
...
- done <- struct{}{}
+ // Reassign after completion or store updates in that pointer-based referenceWithout concurrency safety, other goroutines may still read uninitialized data.
Committable suggestion skipped: line range outside the PR's diff.
- also moves action manager to connector level, rather than resource level
…nd the action id.
ggreer
force-pushed
the
ggreer/baton-actions
branch
from
fa624f3 to
f68448e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/actions/actions.go (3)
29-35:⚠️ Potential issueImplement concurrency controls for ActionManager.
This code reintroduces the same concurrency issue noted in past reviews. AccessingactionId,schemas,handlers, andactionswithout synchronization can result in data races when invoked by multiple goroutines.Below is an example snippet for adding a
sync.Mutexto safeguard reads and writes:29 // TODO: use syncmaps here type ActionManager struct { + mu sync.Mutex actionId uint64 schemas map[string]*v2.BatonActionSchema // map of action name to schema handlers map[string]ActionHandler actions map[string]*OutstandingAction // map of actions IDs }
128-164:⚠️ Potential issueSynchronize access to actions and OutstandingAction fields in InvokeAction.
Although this code stores a pointer to theOutstandingAction, it updates that struct from within a goroutine without synchronization. Data races can arise ifGetActionStatusor subsequent invocations read it simultaneously. Add locking or use a thread-safe structure to ensure consistency.+ a.mu.Lock() a.actions[actionId] = &oa + a.mu.Unlock() go func() { + a.mu.Lock() oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING oa.Rv, oa.Annos, oa.Err = handler(ctx, args) if oa.Err == nil { oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE } else { oa.Status = v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED } + a.mu.Unlock() done <- struct{}{} }()
45-48:⚠️ Potential issueGuard actionId increments with a mutex.
Incrementinga.actionIdwithout a lock is unsafe in concurrent scenarios. Multiple goroutines may race when callingGetNewActionId().func (a *ActionManager) GetNewActionId() string { + a.mu.Lock() a.actionId++ + a.mu.Unlock() return strconv.FormatUint(a.actionId, 10) }
🧹 Nitpick comments (4)
pkg/actions/actions_test.go (1)
53-90: Well-organized test suite; consider optimizing the unnecessary sleep.
The 2ms delay in the handler (time.Sleep(2 * time.Millisecond)) might slow down test runs when scaling up. You could replace it with a mock or remove it if it does not directly test any timing-oriented logic.internal/connector/connector.go (1)
382-382: Registering ActionService unconditionally.
You may want to introduce a toggle similar to ticketing or provisioning if there are scenarios where actions should be disabled.pkg/connectorbuilder/connectorbuilder.go (2)
84-89: Consider reducing the number of return parameters inCustomActionManagermethods.
Each method returns multiple values, which can become unwieldy. Consolidating them into a custom struct could improve maintainability and readability.
91-93: Minor naming duplication.
The methodRegisterActionManagerinside theRegisterActionManagerinterface can cause confusion. Renaming either the interface or method might improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pb/c1/action/v1/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
internal/connector/connector.go(3 hunks)pb/c1/action/v1/action.pb.validate.go(1 hunks)pb/c1/connector/v2/action.pb.validate.go(1 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/actions/actions.go(1 hunks)pkg/actions/actions_test.go(1 hunks)pkg/connectorbuilder/connectorbuilder.go(8 hunks)pkg/sync/syncer_test.go(1 hunks)pkg/tasks/c1api/actions.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/action.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connectorapi/baton/v1/baton.proto(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- proto/c1/connector/v2/connector.proto
- pkg/types/types.go
- pkg/sync/syncer_test.go
- pkg/tasks/c1api/manager.go
- pkg/types/tasks/tasks.go
- pkg/tasks/tasks.go
- proto/c1/connectorapi/baton/v1/baton.proto
- proto/c1/connector/v2/action.proto
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (28)
pkg/actions/actions_test.go (1)
36-37: Potential nil-pointer panic ifargs.Fields["dn"]is missing.
As noted in a previous review, callingargs.Fields["dn"].GetKind()can panic if the"dn"key does not exist. Consider checking for field existence before callingGetKind().internal/connector/connector.go (2)
49-49: No issues with introducing the ActionServiceClient.
This is straightforward and aligns with the existing client pattern.
419-419: New ActionServiceClient instantiation looks correct.
This seamlessly fits into the existing pattern of instantiating other service clients.pb/c1/action/v1/action.pb.validate.go (1)
1-373: Auto-generated code.
This file is generated byprotoc-gen-validateand should generally not be modified manually. No issues found.pkg/connectorbuilder/connectorbuilder.go (10)
17-17: Import looks correct.
The new import forstructpbis necessary for implementing theCustomActionManagerinterface.
107-107: Field declaration is straightforward.
Storing a reference to theCustomActionManageris necessary for the subsequent methods.
317-317: Initialization is fine.
TheactionManagerfield is correctly initialized tonil.
344-349: Logic for setting a custom action manager.
The guard against multiple action managers is appropriate. This ensures only one manager is set.
351-363: Logic for registering an action manager.
The checks for a nilactionManagerand handling of potential errors are handled well.
719-722: Capability addition is consistent.
CAPABILITY_ACTIONS is properly appended only when an action manager is provided.
1031-1055:ListActionSchemasimplementation looks good.
The code follows the pattern established by other methods, checking for a nil manager and recording task metrics.
1057-1082:GetActionSchemaaligned with existing approach.
Ensures the action manager is not nil and properly handles errors, consistent with the rest of the connector builder.
1083-1110:InvokeActionlogic is well-structured.
Captures error scenarios properly (task failure) and returns the relevant data in case of success, mirroring other method patterns.
1111-1138:GetActionStatusmethod follows the same reliable structure.
Ensures the manager is present, distinct error and success paths are handled consistently.pb/c1/connector/v2/action.pb.validate.go (1)
1-1499: Generated validation code is acceptable.
This file appears to be generated byprotoc-gen-validate. Generally, we avoid modifying generated files. No issues stand out.pkg/tasks/c1api/actions.go (5)
28-28: Verify thetracerreference .
Please confirm thattraceris properly imported or defined to avoid compilation issues.
63-63: Verify thetracerreference .
Please confirm thattraceris properly imported or defined to avoid compilation issues.
105-105: Verify thetracerreference .
Please confirm thattraceris properly imported or defined to avoid compilation issues.
152-152: Verify thetracerreference .
Please confirm thattraceris properly imported or defined to avoid compilation issues.
1-184: Overall new file structure is consistent.
Each task handler cleanly delegates to a connector client, checks for required parameters, and logs debug information. Error handling is standardized via the helper’sFinishTask. The concurrency model appears straightforward, with typical request/response calls.pb/c1/connectorapi/baton/v1/baton.pb.validate.go (8)
681-721: No concerns: code follows the typical validation pattern for newTask_ActionListSchemas.
722-762: No concerns: code follows the typical validation pattern for newTask_ActionGetSchema.
763-803: No concerns: code follows the typical validation pattern for newTask_ActionInvoke.
804-844: No concerns: code follows the typical validation pattern for newTask_ActionStatus.
5354-5489: Code forTask_ActionListSchemasTaskappears consistent with protoc-gen-validate’s boilerplate. No issues found.
5490-5627: Code forTask_ActionGetSchemaTaskfollows the same established pattern. Looks good.
5628-5794: Code forTask_ActionInvokeTaskis aligned with standard validation logic. No problems here.
5795-5934: Code forTask_ActionStatusTaskis properly structured and mirrors the generated pattern. No issues.
| func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { | ||
| oa := a.actions[actionId] | ||
| if oa == nil { | ||
| return v2.BatonActionStatus_BATON_ACTION_STATUS_UNKNOWN, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("action id %s not found", actionId)) | ||
| } | ||
|
|
||
| return oa.Status, oa.Rv, nil, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return the stored error from GetActionStatus.
The method sets oa.Err in other parts of the code but ignores it here, making it impossible for callers to detect errors from completed actions.
func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) {
oa := a.actions[actionId]
if oa == nil {
return v2.BatonActionStatus_BATON_ACTION_STATUS_UNKNOWN, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("action id %s not found", actionId))
}
- return oa.Status, oa.Rv, nil, nil
+ return oa.Status, oa.Rv, oa.Annos, oa.Err
}📝 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 (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { | |
| oa := a.actions[actionId] | |
| if oa == nil { | |
| return v2.BatonActionStatus_BATON_ACTION_STATUS_UNKNOWN, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("action id %s not found", actionId)) | |
| } | |
| return oa.Status, oa.Rv, nil, nil | |
| } | |
| func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { | |
| oa := a.actions[actionId] | |
| if oa == nil { | |
| return v2.BatonActionStatus_BATON_ACTION_STATUS_UNKNOWN, nil, nil, status.Error(codes.NotFound, fmt.Sprintf("action id %s not found", actionId)) | |
| } | |
| return oa.Status, oa.Rv, oa.Annos, oa.Err | |
| } |
…ls around setting action statuses.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.go(1 hunks)pkg/actions/actions_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/actions/actions.go
[failure] 207-207:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 210-210:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 212-212:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 223-223:
Error return value of oa.SetStatus is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (2)
pkg/actions/actions_test.go (1)
36-39: Potential nil-pointer panic due to missing map key.
Ifargs.Fields["dn"]is absent, callingargs.Fields["dn"].GetKind()can panic. Safely retrieve the field before callingGetKind():-func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { - _, ok := args.Fields["dn"].GetKind().(*structpb.Value_StringValue) - if !ok { +func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { + dnVal, ok := args.Fields["dn"] + if !ok || dnVal == nil { return nil, nil, fmt.Errorf("missing dn") }pkg/actions/actions.go (1)
59-64: Consider concurrency safety for action storage and retrieval.
As noted by the TODO, shared maps and fields withinActionManagercan cause data races when used across multiple goroutines. Using locks or sync.Map can help ensure thread-safe access.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/actions.go(1 hunks)pkg/actions/actions_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/actions/actions.go
[failure] 209-209:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 212-212:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 214-214:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 225-225:
Error return value of oa.SetStatus is not checked (errcheck)
[failure] 207-207:
lostcancel: the cancel function returned by context.WithTimeoutCause should be called, not discarded, to avoid a context leak (govet)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (2)
pkg/actions/actions_test.go (1)
35-36: Potential nil-pointer panic due to missing map key
Sinceargs.Fields["dn"]might benil, calling.GetKind()here can cause a panic. You should confirm the field exists and is non-nil before attempting to retrieve the kind.
[duplicate_comment, critical_issue]A possible fix:
-func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { - _, ok := args.Fields["dn"].GetKind().(*structpb.Value_StringValue) - if !ok { - return nil, nil, fmt.Errorf("missing dn") - } +func testActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { + dnVal, ok := args.Fields["dn"] + if !ok || dnVal == nil { + return nil, nil, fmt.Errorf("missing dn") + } + _, ok2 := dnVal.GetKind().(*structpb.Value_StringValue) + if !ok2 { + return nil, nil, fmt.Errorf("missing dn") + }pkg/actions/actions.go (1)
59-63: Concurrency caution
Plain maps are used in an environment where multiple goroutines might access them concurrently. This can cause data races when reading/writing toa.actionsanda.schemas.
pkg/actions/actions.go
Outdated
| // If handler takes longer than an hour, return status failed. | ||
| handlerCtx, _ := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out")) | ||
| go func() { | ||
| oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check the error return value from SetStatus
Ignoring the error can mask invalid state transitions and introduce subtle bugs.
go func() {
- oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING)
+ if err := oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING); err != nil {
+ // handle or log the error
+ }
oa.Rv, oa.Annos, oa.Err = handler(handlerCtx, args)
if oa.Err == nil {
- oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE)
+ if err := oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE); err != nil {
+ // handle or log the error
+ }
} else {
- oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED)
+ if err := oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED); err != nil {
+ // handle or log the error
+ }
}
...
- oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED)
+ if err := oa.SetStatus(v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED); err != nil {
+ // handle or log the error
+ }Also applies to: 212-212, 214-214, 225-225
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 209-209:
Error return value of oa.SetStatus is not checked (errcheck)
pkg/actions/actions.go
Outdated
| // If handler exits within a second, return result. | ||
| // If handler takes longer than 10 seconds, return status pending. | ||
| // If handler takes longer than an hour, return status failed. | ||
| handlerCtx, _ := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid context leak by calling the cancel function
The cancel function returned by context.WithTimeoutCause is currently discarded, leading to a possible context leak. You should defer its invocation or ensure it is properly closed.
- handlerCtx, _ := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out"))
+ handlerCtx, cancel := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out"))
+ defer cancel()📝 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.
| handlerCtx, _ := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out")) | |
| handlerCtx, cancel := context.WithTimeoutCause(ctx, 1*time.Hour, errors.New("action handler timed out")) | |
| defer cancel() |
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 207-207:
lostcancel: the cancel function returned by context.WithTimeoutCause should be called, not discarded, to avoid a context leak (govet)
…y. Cancel context if we finish before timeout.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/actions/actions.go (1)
189-196:⚠️ Potential issueReturn
oa.Errfor better error propagation.This method currently returns
(status, rv, nil, nil)even ifoa.Erris set. Returning the stored error would allow callers to detect and handle any errors from completed actions more reliably. In line with a previous suggestion, consider updating the return signature to includeoa.Err.-func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { - // ... - return oa.Status, oa.Rv, nil, nil +func (a *ActionManager) GetActionStatus(ctx context.Context, actionId string) (v2.BatonActionStatus, *structpb.Struct, annotations.Annotations, error) { + // ... + return oa.Status, oa.Rv, oa.Annos, oa.Err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/actions/actions.go(1 hunks)
🔇 Additional comments (1)
pkg/actions/actions.go (1)
208-223: Ensure status consistency during asynchronous invocation.Currently, a single goroutine updates
OutstandingActiontoRUNNING, then sets it toCOMPLETEorFAILED. Meanwhile, the main function checks status after 10 seconds (line 227) or if the context completes (line 229). Consider these scenarios:
- Callers might see an outdated status if the operation is mid-update.
- Without a lock accompanying reads of
oa.Status, there could be a race with the goroutine that sets the status.A recommended approach is to store and retrieve
OutstandingActionbehind a mutex or to switch to thread-safe structures.
| // TODO: use syncmaps or some other sort of locking mechanism | ||
| type ActionManager struct { | ||
| schemas map[string]*v2.BatonActionSchema // map of action name to schema | ||
| handlers map[string]ActionHandler | ||
| actions map[string]*OutstandingAction // map of actions IDs | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding concurrency-safety for the map fields.
You have a TODO comment referencing sync maps or locking, but the code is already spawning a goroutine (line 211 onward) that writes to a.actions concurrently. Reading or writing to a.actions unprotected can lead to a data race if accessed from multiple goroutines. Struct-level locking (e.g., a sync.Mutex) or sync.Map would help ensure safe concurrent access to the actions map.
TODO:
- Async handler support.- Unit tests for ActionManager.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Taskmessage to include new task types for action management.Bug Fixes