(WIP) Baton Actions #302
(WIP) Baton Actions #302
Conversation
|
Warning Rate limit exceeded@ggreer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
WalkthroughThese changes introduce robust validation logic for Protocol Buffers message types and add a new RPC service for action management. In two 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)
Poem
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
from
868fbc8 to
baaed3e
Compare
Summary by CodeRabbit