Resource Actions #564
Resource Actions #564
Conversation
WalkthroughThis PR introduces resource-scoped actions support to the Baton SDK. It adds protobuf message definitions for resource types, extends action schemas with resource filtering, refactors the ActionManager to support both global and resource-scoped action registration and invocation, introduces new provider interfaces for integration, adds CLI support for resource-scoped operations, and generates comprehensive validation code for new message types. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ActionManager
participant GlobalRegistry
participant ResourceRegistry
participant Handler
rect rgb(200, 220, 240)
Note over Client,Handler: Global Action Registration
Client->>ActionManager: Register(ctx, schema, handler)
ActionManager->>GlobalRegistry: Store schema & handler
ActionManager-->>Client: Success
end
rect rgb(220, 240, 200)
Note over Client,Handler: Resource-Scoped Action Registration
Client->>ActionManager: GetTypeRegistry(ctx, resourceTypeID)
ActionManager-->>Client: ResourceRegistry
Client->>ResourceRegistry: Register(ctx, schema, handler)
ResourceRegistry->>ActionManager: RegisterResourceAction(resourceTypeID, schema, handler)
ActionManager-->>Client: Success
end
rect rgb(240, 220, 200)
Note over Client,Handler: Resource-Scoped Action Invocation
Client->>ActionManager: InvokeAction(ctx, name, resourceTypeID, args)
alt Has resourceTypeID
ActionManager->>ResourceRegistry: invokeResourceAction()
else No resourceTypeID
ActionManager->>GlobalRegistry: invokeGlobalAction()
end
ResourceRegistry->>Handler: Execute with args
Handler-->>ResourceRegistry: Result + status
ResourceRegistry->>ActionManager: Return aggregated result
ActionManager-->>Client: (actionID, status, output, annotations, error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
proto/c1/connector/v2/action.proto
Outdated
| } | ||
|
|
||
| message ResourceActionSchema { | ||
| string resource_type_id = 1; // Required: the resource type this action applies to |
There was a problem hiding this comment.
Why not add resource_type_id to BatonActionSchema, add resource_id to InvokeActionRequest, etc? Then we could reuse most of the existing protos.
There was a problem hiding this comment.
@ggreer ResourceActions and BatonActions are now the same object. No new protos required.
proto/c1/connector/v2/action.proto
Outdated
| string action_id = 1; | ||
| BatonActionStatus status = 2; | ||
| google.protobuf.Struct response = 3; | ||
| repeated google.protobuf.Any annotations = 4; |
There was a problem hiding this comment.
Where is EncryptedData returned? If it's in the response struct, then EncryptionConfig should probably be in the args struct.
There was a problem hiding this comment.
The intention was to have the encrypted data live in the returned response. I don't think EncryptionConfig should be part of the args. I don't think of it as input to the action, but more of instructions for how the connector should handle the input.
I'll think on this some more.
proto/c1/connector/v2/action.proto
Outdated
| ACTION_TYPE_USER_SUSPEND = 11; | ||
| ACTION_TYPE_USER_RESUME = 12; | ||
| ACTION_TYPE_USER_LOCK = 13; | ||
| ACTION_TYPE_USER_UNLOCK = 14; |
There was a problem hiding this comment.
Having more specific user actions like disable, suspend, and lock (and enable, resume, unlock) seems a little premature.
Right now we tend to use action types as tags (which is why action_type is repeated in BatonActionSchema). We use the action name & description to differentiate in the UI between similar actions such as suspend, disable, and lock. If we're going to make more specific user action types, we should probably have more specific user statuses (and add support for those in c1).
There was a problem hiding this comment.
Yes, these are an expansion of the tags for action_type for a chunk of the actions we know we need to implement. Relying on the action name/description isn't sufficient, especially if we want to implement generic automation steps like 'Lock User'. Ideally the end user doesn't need to know what specific action should be run, and we can just pick the one that is tagged accordingly.
There was a problem hiding this comment.
I think we should omit most of these action types for now, and only add them once we decide to deprecate grant/revoke (since that's what most of these are supposed to replace).
There was a problem hiding this comment.
@ggreer they aren't meant to replace, there is a use case in things like automations where we want to be able to perform these actions before c1 has synced the objects so the typical grant/revoke flow doesn't work since C1 doesn't know about the resource.
There was a problem hiding this comment.
Why wouldn't C1 just call GetResource() after the resource was created so we could use regular grant/revoke instead of these custom actions? Or the create action would return a resource that we'd then be able to use for grant/revoke/etc.
| type ResourceActionProvider interface { | ||
| // ResourceActions returns the schemas and handlers for all resource actions | ||
| // supported by this resource type. | ||
| ResourceActions(ctx context.Context) ([]*v2.ResourceActionSchema, map[string]ResourceActionHandler, error) |
There was a problem hiding this comment.
I'd prefer having resource syncers call some helper like RegisterAction(ctx, actionName string, schema *v2.ResourceActionSchema, handler ResourceActionHandler), similar to how global actions are registered. Then we'd avoid having to keep the name in the action schema the same as the key in the map of handlers.
There was a problem hiding this comment.
example now:
func (o *groupBuilder) registerCreateGroupAction(ctx context.Context, registry actions.ResourceTypeActionRegistry) error {
err := registry.Register(ctx, &v2.ResourceActionSchema{
Name: "create",
Arguments: []*config.Field{
{Name: "name", DisplayName: "Group Name", Field: &config.Field_StringField{}, IsRequired: true},
{Name: "admins", DisplayName: "Admins", Field: &config.Field_ResourceIdListField{}},
{Name: "members", DisplayName: "Members", Field: &config.Field_ResourceIdListField{}},
},
ReturnTypes: []*config.Field{
{Name: "success", Field: &config.Field_BoolField{}},
{Name: "resource", Field: &config.Field_ResourceField{}},
},
ActionType: []v2.ActionType{v2.ActionType_ACTION_TYPE_RESOURCE_CREATE},
}, o.handleCreateGroupAction)
return err
}
Note that the registry passed to each resource builder knows the resource type so it doesn't need to be specified now.
| credentialManagers map[string]CredentialManagerLimited | ||
| eventFeeds map[string]EventFeed | ||
| accountManagers map[string]AccountManagerLimited // NOTE(kans): currently unused | ||
| resourceActionManager *actions.ResourceActionManager |
There was a problem hiding this comment.
A recent strategy we have used for the connector builder is to apply a shim when we instantiate the struct so that their is a single way to do anything on the builder itself. For instance, we don't have a ResourceSyncer and a _ ResourceSyncerV2_. - addTargetedSyncer wraps the V1 version with newResourceSyncerV1toV2. Otherwise, we have deeply branching logic within the gRPC handlers. Is it possible to encapsulate the logic into a single interface on this side? We could store the global actions under an empty key or the equivalent.
There was a problem hiding this comment.
@kans the resourceActionManager logic has been fully merged with the pre-existing ActionManager interface. I think this satisfies what you were asking for.
jirwin
force-pushed
the
jirwin/resource-actions
branch
from
1fbeac4 to
b5d877c
Compare
phoebesimon
force-pushed
the
jirwin/resource-actions
branch
from
b5d877c to
001a2aa
Compare
jirwin
force-pushed
the
jirwin/resource-actions
branch
from
b45bbf3 to
6350ab8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/tasks/local/action_invoker.go (1)
20-27: Remove the unuseddbPathfield fromlocalActionInvokerstruct.The
dbPathfield is accepted in theNewActionInvokerconstructor and stored in the struct, but it is never used in any method (GetTempDir,ShouldDebug,Next, orProcess). Unlike other manager types in this package that actively usedbPath, the action invoker has no need for it. Removing it will eliminate dead code and reduce confusion.pkg/actions/actions.go (1)
591-618: Unbuffered channel can cause goroutine leak.When the function returns early via the 1-second timeout (line 613) or context cancellation (line 615), the goroutine at line 607 will block forever on
done <- struct{}{}since the channel is unbuffered and no one is receiving.Consider using a buffered channel:
- done := make(chan struct{}) + done := make(chan struct{}, 1)
🧹 Nitpick comments (3)
pkg/tasks/local/action_invoker.go (1)
70-75: Consider log verbosity for response payload.Using
zap.Any("response", resp.GetResponse())could log potentially large or sensitive payloads. For a local debugging tool this is likely acceptable, but consider whetherDebuglevel would be more appropriate, or limiting the logged response size.proto/c1/config/v1/config.proto (1)
73-81: Documentation of intentional duplication is appreciated.The comment explaining the duplication to avoid import cycles is helpful for future maintainers. Consider adding a reference to the original
Resourceproto location (e.g.,c1/connector/v2/resource.proto) so developers know where the canonical definition lives.pkg/actions/args.go (1)
408-482: Inconsistent marshaling between single and list return fields.
NewResourceIdReturnField(line 410) usesconfig.ResourceIdFieldwith fieldsresource_type_idandresource_id, whileNewResourceIdListReturnField(line 469) marshalsv2.ResourceIddirectly, which has different JSON field names (resource_typeandresource).This inconsistency could cause issues when deserializing with
GetResourceIDArgwhich expects specific field names.Consider using consistent types:
func NewResourceIdListReturnField(key string, resourceIDs []*v2.ResourceId) (ReturnField, error) { listValues := make([]*structpb.Value, len(resourceIDs)) for i, resourceId := range resourceIDs { - jsonBytes, err := protojson.Marshal(resourceId) + basicResourceId := &config.ResourceIdField{ + ResourceTypeId: resourceId.ResourceType, + ResourceId: resourceId.Resource, + } + jsonBytes, err := protojson.Marshal(basicResourceId) if err != nil { return ReturnField{}, fmt.Errorf("failed to marshal resource id: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
pb/c1/config/v1/config.pb.validate.go(2 hunks)pb/c1/connector/v2/action.pb.validate.go(2 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/actions/actions.go(9 hunks)pkg/actions/actions_test.go(6 hunks)pkg/actions/args.go(1 hunks)pkg/actions/args_test.go(1 hunks)pkg/cli/commands.go(1 hunks)pkg/connectorbuilder/actions.go(6 hunks)pkg/connectorbuilder/connectorbuilder.go(6 hunks)pkg/connectorbuilder/connectorbuilder_test.go(4 hunks)pkg/connectorbuilder/connectorbuilder_v2_test.go(1 hunks)pkg/connectorbuilder/resource_actions.go(1 hunks)pkg/connectorbuilder/resource_syncer.go(2 hunks)pkg/connectorrunner/runner.go(5 hunks)pkg/field/defaults.go(2 hunks)pkg/ratelimit/grpc.go(1 hunks)pkg/tasks/c1api/actions.go(3 hunks)pkg/tasks/local/action_invoker.go(4 hunks)pkg/tasks/local/action_schema_list.go(1 hunks)proto/c1/config/v1/config.proto(1 hunks)proto/c1/connector/v2/action.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_v2_test.gopkg/connectorbuilder/connectorbuilder.gopkg/connectorbuilder/connectorbuilder_test.gopkg/actions/actions.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_v2_test.gopkg/connectorbuilder/connectorbuilder.gopkg/connectorbuilder/connectorbuilder_test.gopkg/actions/actions.go
📚 Learning: 2025-09-02T15:06:42.596Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 410
File: pkg/sync/syncer.go:650-652
Timestamp: 2025-09-02T15:06:42.596Z
Learning: In the baton-sdk codebase, the syncIDClientWrapper middleware automatically adds ActiveSync annotations to all connector requests that support annotations. This eliminates the need to manually add ActiveSync annotations to individual ListResourceTypes, ListResources, ListEntitlements, ListGrants, and other connector calls in pkg/sync/syncer.go.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.gopkg/actions/actions.go
🧬 Code graph analysis (10)
pkg/cli/commands.go (1)
pkg/connectorrunner/runner.go (2)
WithActionsEnabled(563-568)WithOnDemandListActionSchemas(501-510)
pkg/actions/actions_test.go (1)
pb/c1/connector/v2/action.pb.go (1)
BatonActionStatus_BATON_ACTION_STATUS_COMPLETE(35-35)
pkg/tasks/c1api/actions.go (2)
pb/c1/connector/v2/action.pb.go (2)
ListActionSchemasRequest_builder(1041-1047)InvokeActionRequest_builder(471-481)pkg/annotations/annotations.go (1)
Annotations(12-12)
pkg/actions/args_test.go (1)
pkg/actions/args.go (25)
GetStringArg(14-30)GetIntArg(34-50)GetBoolArg(54-70)GetResourceIDArg(75-113)GetStringSliceArg(117-142)GetStructArg(146-162)RequireStringArg(166-172)RequireResourceIDArg(176-182)GetResourceIdListArg(196-242)SetResourceFieldArg(338-364)GetResourceFieldArg(247-272)GetResourceListFieldArg(301-334)NewReturnValues(491-503)NewStringReturnField(378-380)NewNumberReturnField(388-390)NewBoolReturnField(383-385)NewResourceReturnField(393-406)NewResourceIdReturnField(409-425)NewStringListReturnField(428-434)NewNumberListReturnField(437-443)NewResourceListReturnField(446-463)NewResourceIdListReturnField(466-482)NewListReturnField(485-487)NewReturnField(373-375)RequireResourceIdListArg(186-192)
pkg/field/defaults.go (2)
pkg/field/fields.go (2)
StringField(201-217)BoolField(179-199)pkg/field/field_options.go (5)
WithHidden(77-82)WithDescription(47-53)WithPersistent(129-135)WithExportTarget(103-111)WithDefaultValue(62-68)
pkg/connectorbuilder/connectorbuilder.go (2)
pkg/connectorbuilder/actions.go (2)
ActionManager(17-43)GlobalActionProvider(48-50)pkg/actions/actions.go (2)
ActionManager(105-118)NewActionManager(120-128)
pkg/connectorrunner/runner.go (10)
pkg/synccompactor/compactor.go (1)
Option(51-51)internal/connector/connector.go (1)
Option(104-104)pkg/field/validation.go (1)
Option(335-335)pkg/tasks/local/syncer.go (1)
Option(30-30)pkg/dotc1z/manager/local/local.go (1)
Option(25-25)pkg/dotc1z/manager/s3/s3.go (1)
Option(29-29)pkg/us3/s3.go (1)
Option(58-58)pkg/logging/logging.go (1)
Option(16-16)pkg/tasks/local/action_invoker.go (1)
NewActionInvoker(86-93)pkg/tasks/local/action_schema_list.go (1)
NewListActionSchemas(73-77)
pkg/connectorbuilder/connectorbuilder_test.go (3)
pb/c1/connector/v2/resource.pb.go (9)
EncryptionConfig(2440-2451)EncryptionConfig(2464-2464)ResourceType(126-136)ResourceType(149-149)ResourceType_builder(229-238)ResourceId(2630-2637)ResourceId(2650-2650)Resource(2715-2727)Resource(2740-2740)pkg/actions/actions.go (2)
ActionRegistry(91-98)NewActionManager(120-128)pkg/connectorbuilder/resource_syncer.go (2)
ResourceSyncer(34-39)ResourceType(30-32)
pkg/connectorbuilder/actions.go (2)
pb/c1/connector/v2/action.pb.go (2)
BatonActionSchema(210-223)BatonActionSchema(236-236)pkg/actions/actions.go (1)
ActionRegistry(91-98)
pkg/actions/args.go (1)
pb/c1/config/v1/config.pb.go (4)
Resource(1148-1157)Resource(1170-1170)ResourceIdField(1408-1414)ResourceIdField(1427-1427)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (48)
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (1)
5525-5525: Auto-generated validation file additions look appropriate.This is an auto-generated protobuf validation file (as indicated by the header on line 1). The changes on line 5525 and line 5832 add documentation comments for the new
ResourceTypeIdfield inTask_ActionListSchemasTaskandTask_ActionInvokeTaskvalidation functions, respectively. These comments follow the established pattern used throughout the file for fields without validation constraints (see lines 59, 61, 63, 3353, 5629, 5767, 5936–5938, etc.).Since this file is generated and should not be hand-edited, the implementation is correct. The comments document that there are no validation rules for the new resource-scoped action fields, which is consistent with their optional nature in the feature design.
pkg/ratelimit/grpc.go (1)
66-82: LGTM! Guards against empty resource type descriptors align with resource-scoping semantics.The conditional checks ensure rate limit descriptors are only created when resource type values are present, preventing meaningless empty descriptors. The pattern is consistent across both the
hasResourceandhasResourceTypeblocks, and the comment correctly documents the newListActionSchemassupport.proto/c1/connectorapi/baton/v1/baton.proto (1)
115-132: LGTM! Well-structured additions for resource-scoped actions.The new
resource_type_idfields are appropriately added as optional strings with clear documentation. Field numbers don't conflict with existing fields, and the changes maintain backward compatibility.pkg/tasks/local/action_invoker.go (2)
57-65: LGTM! Proper conditional handling of optional resource type.The conditional check before setting
ResourceTypeIdcorrectly avoids sending empty strings in the request, maintaining clean API semantics for optional fields.
84-93: Constructor properly extended for resource-scoped actions.The signature change and documentation are clear, and the caller in
pkg/connectorrunner/runner.go:791has been updated to pass the newresourceTypeIDparameter.proto/c1/config/v1/config.proto (2)
64-69: LGTM! Field oneof extended with resource-related types.The new field types (105-108) are appropriately added to support resource-scoped actions. The distinction between ID fields and full resource fields provides good flexibility for different use cases.
91-99: Clarify intended relationship betweenResourceIdListField.resource_type_idand item-level types.
ResourceIdListFieldhas both a list ofResourceIdFielditems (each with their ownresource_type_id) and a top-levelresource_type_id. This could lead to ambiguity:
- Is the top-level
resource_type_ida filter/constraint for all items?- Can items have different types than the parent field specifies?
Consider adding a comment documenting the intended semantics to prevent confusion.
pkg/connectorbuilder/connectorbuilder_v2_test.go (1)
101-101: LGTM! Test assertion appropriately relaxed.The change from asserting a specific ID value to asserting non-empty is appropriate if the action ID is now generated dynamically (e.g., by the unified ActionManager). This maintains test coverage while accommodating the implementation change.
pkg/cli/commands.go (1)
268-282: LGTM! CLI extended to support resource-scoped actions.The new
invoke-action-resource-typeandlist-action-schemas-resource-typeflags follow the established pattern for on-demand commands. Both flags are properly registered inpkg/field/defaults.go(lines 175 and 188 respectively) and the inline comments correctly clarify their optional nature.pkg/field/defaults.go (2)
175-220: LGTM! Well-structured field definitions for resource-scoped actions.The new field definitions follow the established pattern consistently, using appropriate field constructors with standard options (hidden, persistent, no export). The separation between global action fields and resource-scoped action fields is clear, and the default value for
invokeResourceActionArgsFieldproperly mirrors the existinginvokeActionArgsField.
352-358: LGTM! Proper integration of new fields.The new fields are correctly appended to the
DefaultFieldsslice, ensuring they are available throughout the SDK via the default field mechanism.pkg/connectorbuilder/resource_syncer.go (1)
371-403: LGTM! Clean integration of resource-scoped actions.The changes properly integrate resource action registration during syncer setup:
- Context parameter is now named and used appropriately
- Type assertion for
ResourceActionProviderfollows Go conventions- Error messages include the resource type ID for clear diagnostics
- Proper error propagation maintains the existing error handling pattern
pb/c1/connector/v2/action.pb.validate.go (1)
337-371: Generated validation code is correct.The validation logic for the new
ResourceTypeIdandEncryptionConfigsfields follows the expected pattern for protoc-gen-validate generated code. The no-op validations (no rules) are appropriate when the proto definition doesn't specify validation constraints.Also applies to: 1294-1294
pkg/tasks/c1api/actions.go (3)
39-45: LGTM! Clean builder pattern usage with resource type filtering.The conversion to using
reqBuilderwith conditionalResourceTypeIdsetting is well-implemented and maintains backward compatibility (empty string is not set).
128-136: LGTM! Consistent implementation of resource type support.The action invocation follows the same pattern as list schemas, properly using the builder pattern and conditionally setting
ResourceTypeIdonly when non-empty.
185-185: Nice catch! Log message correction.Changed from "ActionInvoke response" to "ActionStatus response" to accurately reflect the action status flow.
pkg/actions/actions_test.go (3)
127-160: LGTM! Tests updated to match new API signatures.All test invocations correctly updated to use the new API:
Registerinstead ofRegisterActionListActionSchemas(ctx, "")with empty resource type for global actionsInvokeAction(ctx, action, "", input, nil)with resource type and encryption parametersThe test logic and validations remain unchanged, confirming backward compatibility.
162-199: LGTM! Async action tests properly updated.The async action handler tests consistently use the updated API signatures, maintaining the same test flow for status checking and completion verification.
201-261: LGTM! Goroutine leak tests updated consistently.Both test cases (normal completion and context cancellation) properly use the new API signatures while maintaining the original goroutine leak detection logic.
pkg/connectorbuilder/resource_actions.go (1)
1-27: LGTM! Well-designed public interface for resource-scoped actions.The new types establish a clean contract for resource-scoped action handling:
ResourceActionHandlerproperly includesresourceIDparameter to distinguish from global actionsResourceActionProviderinterface enables resource builders to register their actions during sync setup- Clear documentation explains the purpose of each type
- Follows Go interface design conventions
pkg/tasks/local/action_schema_list.go (1)
1-77: LGTM! Clean implementation of action schema listing task.The new task manager properly supports optional resource type filtering:
localListActionSchemasfollows the task manager pattern used elsewheresync.Onceensures the task is generated only once (appropriate for one-shot operations)- Conditional setting of
ResourceTypeIdmaintains backward compatibility- Logging differentiates between filtered and unfiltered results for better observability
- Public constructor
NewListActionSchemasprovides a clean APIpkg/connectorrunner/runner.go (3)
287-294: LGTM! Config structs extended for resource-scoped actions.The addition of
resourceTypeIDtoinvokeActionConfigand the newlistActionSchemasConfigproperly support optional resource type filtering. The fields are clearly commented as optional, maintaining backward compatibility.
484-510: LGTM! Well-documented option constructors for resource-scoped actions.Both
WithOnDemandInvokeActionandWithOnDemandListActionSchemasproperly:
- Include clear documentation explaining the optional
resourceTypeIDparameter- Set all required config fields
- Follow the established Option pattern
- Maintain consistency with other on-demand options
764-794: LGTM! Proper on-demand mode validation and task manager creation.The validation logic correctly identifies that
listActionSchemasConfigdoesn't requirec1zPath(it only filters by resource type), while other configs likeinvokeActionConfigdo require it. The task manager creation at lines 791-794 properly passes theresourceTypeIDparameter to the constructors.pkg/actions/args_test.go (1)
1-1103: Comprehensive test coverage for argument helpers - LGTM!The test file provides thorough coverage for all argument extraction and return value helpers. Good use of table-driven tests with proper edge case coverage including:
- Nil/empty inputs handling
- Type mismatch scenarios
- Alternative field name support (resource_type vs resource_type_id)
- Round-trip serialization tests for Resource types
pkg/connectorbuilder/connectorbuilder_test.go (5)
490-522: Updated test mock aligns with new interface signatures.The
testCustomActionManagerimplementation correctly adapts to the expanded interface withresourceTypeIDandencryptionConfigsparameters. TheGetTypeRegistryreturning(nil, nil)is appropriate for test scenarios that don't require resource-scoped action registration.
1169-1179: Enhanced test coverage for action status retrieval.Good addition to verify the action status flow using the ID returned from
InvokeAction. This ensures the action lifecycle is properly tested.
1181-1241: Comprehensive tests for GlobalActionProvider interface.The test properly exercises the new
GlobalActionProviderinterface including:
- Action registration via registry
- Schema listing and retrieval
- Action invocation with response validation
- Status tracking using the returned action ID
1243-1319: Resource-scoped action provider tests look good.The
testResourceActionProviderSyncerproperly implements bothResourceSyncerand action registration through theResourceActionsmethod. Tests verify:
- Resource type filtering in
ListActionSchemas- Resource type ID is set on returned schemas
- Action invocation with resource type scope
1321-1374: Good coverage for HasActions and backward compatibility.Tests verify:
HasActions()returns false when no actions registeredHasActions()returns true after global or resource-scoped registration- Legacy
RegisterActionManagerinterface continues to work through the compatibility wrapperproto/c1/connector/v2/action.proto (3)
40-42: Resource-scoped action schema field added correctly.The optional
resource_type_idfield enables scoping actions to specific resource types while maintaining backward compatibility with existing global actions.
52-87: Expanded ActionType enum provides well-defined action categories.The new action types are logically organized by trait:
- Generic resource operations (create, delete, mutate, enable, disable)
- User-specific operations (suspend, lock, password management, etc.)
- Group, Role, App, and Secret operations
This aligns with the PR objective of introducing "well-known action types aligned with resource traits."
93-97: InvokeActionRequest extended for resource-scoped invocation and encryption.The additions enable:
resource_type_id: Targeting actions to specific resource typesencryption_configs: Supporting transparent secret field handling as described in PR objectivespkg/connectorbuilder/connectorbuilder.go (3)
101-103: ActionManager initialization is well-structured.Creating the concrete
ActionManagertype and passing it through ensures:
- The concrete type is available for registration operations
- The interface type is stored for dispatch operations
This separation allows type-safe registration while maintaining interface abstraction.
144-154: Proper ordering of action manager registration.Legacy interfaces are handled first via
addActionManager, followed by the newGlobalActionProviderinterface. This ensures backward compatibility while supporting the new registration pattern.
404-406: HasActions() check is more appropriate than nil check.Since
actionManageris always initialized (line 102-103), checkingHasActions()correctly determines whether any actions have been registered rather than whether the manager exists.pkg/connectorbuilder/actions.go (4)
14-43: Well-designed unified ActionManager interface.The interface provides a clear contract for action management with:
- Resource-type-aware schema listing
- Resource-scoped action invocation with encryption support
- Registry access for per-resource-type action registration
- Capability checking via
HasActions()Good documentation explaining the behavior of each method.
45-50: GlobalActionProvider interface is clean and focused.This provides a simple, idiomatic way for connectors to register global actions without implementing the full deprecated
CustomActionManagerinterface.
190-197: Legacy action bridging correctly adapts to new registry.The
registerLegacyActionfunction properly wraps legacyCustomActionManageractions:
- Extracts the action name from schema
- Creates an
ActionHandlerthat delegates to the legacyInvokeAction- Registers via the new registry pattern
Note: The empty string for
resourceTypeIDandnilforencryptionConfigsin the legacy handler (line 193) is appropriate since legacy actions are global and don't support encryption features.
199-236: Comprehensive legacy interface bridging.The
addActionManagerfunction handles both deprecated patterns:
- Direct
CustomActionManagerimplementationRegisterActionManagerLimitedfactory patternBoth paths extract schemas and re-register them via the new registry, enabling seamless migration.
pkg/actions/actions.go (4)
89-102: LGTM!The
ActionRegistryinterface design is clean with proper deprecation notices for backward compatibility.
104-128: LGTM!The
ActionManagerstruct is well-organized with clear separation between global and resource-scoped actions, and proper initialization.
135-175: LGTM!The action lifecycle management with proper mutex handling and cleanup of old completed/failed actions is well implemented.
231-296: LGTM!The
RegisterResourceActionmethod has comprehensive validation and proper duplicate detection for both action names and action types.pb/c1/config/v1/config.pb.validate.go (1)
1-2: Auto-generated code - no review needed.This file is generated by
protoc-gen-validateas indicated by the header comment. The validation logic follows standard patterns for the newResource,ResourceField,ResourceListField,ResourceIdField, andResourceIdListFieldmessage types.pkg/actions/args.go (3)
12-70: LGTM!The basic getter functions follow a consistent pattern with proper nil checks and type assertions.
72-113: LGTM!Good approach with fallback field names for compatibility between different serialization formats.
489-503: LGTM!The
NewReturnValueshelper provides clean ergonomics for constructing action return values with a success indicator.
pkg/actions/actions.go
Outdated
| // decryptSecretFields decrypts secret fields in the args struct based on the schema. | ||
| func (a *ActionManager) decryptSecretFields( | ||
| ctx context.Context, | ||
| schema []*config.Field, | ||
| args *structpb.Struct, | ||
| encryptionConfigs []*v2.EncryptionConfig, | ||
| ) (*structpb.Struct, error) { | ||
| if args == nil || len(encryptionConfigs) == 0 { | ||
| return args, nil | ||
| } | ||
|
|
||
| l := ctxzap.Extract(ctx) | ||
| // Create a copy of the struct | ||
| result := proto.Clone(args).(*structpb.Struct) | ||
|
|
||
| // Build a map of secret field names | ||
| secretFields := make(map[string]bool) | ||
| for _, field := range schema { | ||
| if field.GetIsSecret() { | ||
| secretFields[field.GetName()] = true | ||
| } | ||
| } | ||
|
|
||
| if len(secretFields) == 0 { | ||
| return result, nil | ||
| } | ||
|
|
||
| // Decrypt secret fields | ||
| for fieldName := range secretFields { | ||
| fieldValue, ok := result.Fields[fieldName] | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| // The field value should be an EncryptedData message encoded as a struct | ||
| encryptedDataStruct, ok := fieldValue.GetKind().(*structpb.Value_StructValue) | ||
| if !ok { | ||
| l.Warn("secret field is not a struct, skipping decryption", zap.String("field", fieldName)) | ||
| continue | ||
| } | ||
|
|
||
| // Convert struct to EncryptedData | ||
| encryptedDataBytes, err := encryptedDataStruct.StructValue.MarshalJSON() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal encrypted data for field %s: %w", fieldName, err) | ||
| } | ||
|
|
||
| var encryptedData v2.EncryptedData | ||
| if err := json.Unmarshal(encryptedDataBytes, &encryptedData); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal encrypted data for field %s: %w", fieldName, err) | ||
| } | ||
|
|
||
| // For decryption, we need the private key which should be available from context | ||
| // or from the encryption config. For now, we'll try to get it from context. | ||
| // If decryption is not possible, we'll pass through the encrypted value. | ||
| // The handler can decide how to handle it, or we can enhance this later. | ||
|
|
||
| // Try to get private key from context (similar to how accounts.go does it) | ||
| // For now, if we can't decrypt, we'll skip and let the handler deal with it | ||
| // In a full implementation, you'd extract the key ID from encryptedData and | ||
| // match it with a private key from context or config | ||
| l.Debug("skipping decryption for secret field - decryption from encryption configs not yet implemented", zap.String("field", fieldName)) | ||
| // For now, pass through the encrypted value as-is | ||
| // TODO: Implement proper decryption using private key from context or config | ||
| } | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the actual state of the function and check for context
cd /repo && find . -name "actions.go" -type f | grep -E "(actions|pkg)" | head -5Repository: ConductorOne/baton-sdk
Length of output: 121
🏁 Script executed:
# Get file info and read the full function context
wc -l pkg/actions/actions.goRepository: ConductorOne/baton-sdk
Length of output: 92
🏁 Script executed:
# Read the decryptSecretFields function and surrounding code
sed -n '400,480p' pkg/actions/actions.goRepository: ConductorOne/baton-sdk
Length of output: 2953
🏁 Script executed:
# Check if encryptSecretFields exists and how it works
rg -A 30 "func.*encryptSecretFields" pkg/actions/Repository: ConductorOne/baton-sdk
Length of output: 1587
🏁 Script executed:
# Search for callers of decryptSecretFields
rg "decryptSecretFields" --type goRepository: ConductorOne/baton-sdk
Length of output: 363
🏁 Script executed:
# Check git log for context on this function
git log --oneline -20 -- pkg/actions/actions.goRepository: ConductorOne/baton-sdk
Length of output: 154
🏁 Script executed:
# Look for any related issues or documentation
rg -l "decryption|decrypt" --type go | head -10Repository: ConductorOne/baton-sdk
Length of output: 502
🏁 Script executed:
# Get the full encryptSecretFields implementation to see if it actually encrypts
sed -n '474,550p' pkg/actions/actions.goRepository: ConductorOne/baton-sdk
Length of output: 2314
🏁 Script executed:
# Find the InvokeAction method to see how decryptSecretFields result is used
rg -B 5 -A 20 "decryptSecretFields\(ctx" pkg/actions/actions.goRepository: ConductorOne/baton-sdk
Length of output: 1078
🏁 Script executed:
# Check if there are any guards, feature flags, or conditionals around decryption
rg -B 10 -A 5 "decryption.*not yet implemented" pkg/actions/Repository: ConductorOne/baton-sdk
Length of output: 1338
🏁 Script executed:
# Look at the PR or commit context
git show 67e0018 --statRepository: ConductorOne/baton-sdk
Length of output: 50379
🏁 Script executed:
# Check if there are any tests for decryptSecretFields
rg "decryptSecretFields|EncryptSecretFields" --type go -A 5 | head -60Repository: ConductorOne/baton-sdk
Length of output: 922
🏁 Script executed:
# Look for test files related to actions
find . -name "*actions*test*" -type fRepository: ConductorOne/baton-sdk
Length of output: 95
🏁 Script executed:
# Check the test file to understand expected behavior
head -100 pkg/actions/actions_test.goRepository: ConductorOne/baton-sdk
Length of output: 2606
🏁 Script executed:
# Check if there are specific tests for encryption/decryption
rg -A 30 "TestEncrypt|TestDecrypt|encrypt.*secret|decrypt.*secret" pkg/actions/actions_test.goRepository: ConductorOne/baton-sdk
Length of output: 48
🏁 Script executed:
# Look for any TODO or FIXME comments related to decryption in the codebase
rg "TODO.*decrypt|FIXME.*decrypt" --type goRepository: ConductorOne/baton-sdk
Length of output: 168
Secret field decryption is not implemented.
The decryptSecretFields function currently skips decryption and passes encrypted data through unchanged (lines 467-469). While encryptSecretFields is fully implemented and functional, this creates a critical asymmetry: action handlers will receive encrypted secret fields instead of plaintext values, breaking the intended encryption-decryption flow.
The function's TODO comment explicitly acknowledges this limitation, but it prevents the encryption feature from working end-to-end. Implement decryption logic using the private key from encryption configs to complete this feature.
| done := make(chan struct{}) | ||
|
|
||
| // Invoke handler in goroutine | ||
| go func() { | ||
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) | ||
| handlerCtx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Hour, errors.New("action handler timed out")) | ||
| defer cancel() | ||
| var oaErr error | ||
| oa.Rv, oa.Annos, oaErr = handler(handlerCtx, decryptedArgs) | ||
| if oaErr == nil { | ||
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE) | ||
| } else { | ||
| oa.SetError(ctx, oaErr) | ||
| } | ||
| done <- struct{}{} | ||
| }() | ||
|
|
||
| // Wait for completion or timeout | ||
| select { | ||
| case <-done: | ||
| // Encrypt secret output fields | ||
| encryptedResponse, err := a.encryptSecretFields(ctx, schema.GetReturnTypes(), oa.Rv, encryptionConfigs) | ||
| if err != nil { | ||
| return oa.Id, oa.Status, oa.Rv, oa.Annos, fmt.Errorf("failed to encrypt secret fields: %w", err) | ||
| } | ||
| return oa.Id, oa.Status, encryptedResponse, oa.Annos, nil | ||
| case <-time.After(1 * time.Second): | ||
| return oa.Id, oa.Status, oa.Rv, oa.Annos, nil | ||
| case <-ctx.Done(): | ||
| oa.SetError(ctx, ctx.Err()) | ||
| return oa.Id, oa.Status, oa.Rv, oa.Annos, ctx.Err() | ||
| } |
There was a problem hiding this comment.
Same unbuffered channel issue and unencrypted output on timeout.
-
The
donechannel on line 673 has the same unbuffered channel issue asinvokeGlobalAction. -
When the function returns via the 1-second timeout (lines 699-700), the response
oa.Rvis returned without encryption. If the handler completes after the timeout, the response stored inoa.Rvwill contain plaintext secret fields. WhenGetActionStatusis later called, this unencrypted data could be exposed.
- done := make(chan struct{})
+ done := make(chan struct{}, 1)For the encryption issue, consider encrypting the output within the goroutine after the handler completes, or document that secret field encryption only occurs for synchronously completed actions.
📝 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.
| done := make(chan struct{}) | |
| // Invoke handler in goroutine | |
| go func() { | |
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) | |
| handlerCtx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Hour, errors.New("action handler timed out")) | |
| defer cancel() | |
| var oaErr error | |
| oa.Rv, oa.Annos, oaErr = handler(handlerCtx, decryptedArgs) | |
| if oaErr == nil { | |
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE) | |
| } else { | |
| oa.SetError(ctx, oaErr) | |
| } | |
| done <- struct{}{} | |
| }() | |
| // Wait for completion or timeout | |
| select { | |
| case <-done: | |
| // Encrypt secret output fields | |
| encryptedResponse, err := a.encryptSecretFields(ctx, schema.GetReturnTypes(), oa.Rv, encryptionConfigs) | |
| if err != nil { | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, fmt.Errorf("failed to encrypt secret fields: %w", err) | |
| } | |
| return oa.Id, oa.Status, encryptedResponse, oa.Annos, nil | |
| case <-time.After(1 * time.Second): | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, nil | |
| case <-ctx.Done(): | |
| oa.SetError(ctx, ctx.Err()) | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, ctx.Err() | |
| } | |
| done := make(chan struct{}, 1) | |
| // Invoke handler in goroutine | |
| go func() { | |
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING) | |
| handlerCtx, cancel := context.WithTimeoutCause(context.Background(), 1*time.Hour, errors.New("action handler timed out")) | |
| defer cancel() | |
| var oaErr error | |
| oa.Rv, oa.Annos, oaErr = handler(handlerCtx, decryptedArgs) | |
| if oaErr == nil { | |
| oa.SetStatus(ctx, v2.BatonActionStatus_BATON_ACTION_STATUS_COMPLETE) | |
| } else { | |
| oa.SetError(ctx, oaErr) | |
| } | |
| done <- struct{}{} | |
| }() | |
| // Wait for completion or timeout | |
| select { | |
| case <-done: | |
| // Encrypt secret output fields | |
| encryptedResponse, err := a.encryptSecretFields(ctx, schema.GetReturnTypes(), oa.Rv, encryptionConfigs) | |
| if err != nil { | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, fmt.Errorf("failed to encrypt secret fields: %w", err) | |
| } | |
| return oa.Id, oa.Status, encryptedResponse, oa.Annos, nil | |
| case <-time.After(1 * time.Second): | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, nil | |
| case <-ctx.Done(): | |
| oa.SetError(ctx, ctx.Err()) | |
| return oa.Id, oa.Status, oa.Rv, oa.Annos, ctx.Err() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/actions/args.go (1)
194-242: Consider extracting shared resource ID parsing logic.Lines 217-234 duplicate the field extraction logic from
GetResourceIDArg(lines 91-107). Extracting a helper would reduce duplication:func extractResourceIdFromStruct(s *structpb.Struct) (*v2.ResourceId, bool) { resourceTypeID, ok := GetStringArg(s, "resource_type_id") if !ok { resourceTypeID, ok = GetStringArg(s, "resource_type") if !ok { return nil, false } } resourceID, ok := GetStringArg(s, "resource_id") if !ok { resourceID, ok = GetStringArg(s, "resource") if !ok { return nil, false } } return &v2.ResourceId{ResourceType: resourceTypeID, Resource: resourceID}, true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/actions/args.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/actions/args.go (1)
pb/c1/config/v1/config.pb.go (4)
Resource(1148-1157)Resource(1170-1170)ResourceIdField(1408-1414)ResourceIdField(1427-1427)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-lint
🔇 Additional comments (5)
pkg/actions/args.go (5)
14-70: LGTM! Well-structured primitive arg extraction.The nil-safety checks and consistent return patterns are good. Using the comma-ok idiom for type assertions prevents panics.
72-113: LGTM! Good backward compatibility handling.Supporting alternative field names (
resource_type_id/resource_typeandresource_id/resource) is a pragmatic approach for handling schema variations.
115-192: LGTM! Consistent helper implementations.The fail-fast behavior in
GetStringSliceArg(returning false if any element isn't a string) is the right approach. TheRequire*wrappers provide clean ergonomics for mandatory fields.
274-320: Previously identified nil pointer issues are addressed.The nil checks for
resource.Id,resource.ParentResourceId,basicResource.ResourceId, andbasicResource.ParentResourceIdcorrectly prevent the previously flagged panics. The current callers within this file handle top-level nil checks appropriately.
508-527: LGTM! Clean utility helpers.
NewListReturnFieldandNewReturnValuesprovide good ergonomics for constructing action return values. The hardcoded"success"key inNewReturnValuesestablishes a consistent contract.
jirwin
force-pushed
the
jirwin/resource-actions
branch
from
5af96fe to
dd6a43e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/actions/actions.go (1)
591-591: Unbuffered channel can cause goroutine leak on timeout.Both
invokeGlobalAction(line 591) andinvokeResourceAction(line 673) use unbuffered channels:done := make(chan struct{}). If the 1-second timeout is hit before the handler completes, the goroutine will block forever ondone <- struct{}{}(lines 607 and 687) since no one is receiving.- done := make(chan struct{}) + done := make(chan struct{}, 1)Apply this fix to both line 591 and line 673.
Also applies to: 673-673
♻️ Duplicate comments (6)
pkg/actions/actions.go (2)
406-473: Decryption not yet implemented.The
decryptSecretFieldsfunction logs a debug message and passes through encrypted values unchanged. This means action handlers for resource-scoped actions will receive encrypted data instead of plaintext for secret fields, breaking the intended flow.The TODO on line 469 acknowledges this. Ensure this is tracked for implementation before the feature is used in production.
699-700: Response not encrypted on timeout path.When the 1-second timeout is hit (lines 699-700),
oa.Rvis returned without encryption. If the handler completes after the timeout and stores sensitive data inoa.Rv, subsequentGetActionStatuscalls could expose unencrypted secrets.Consider either:
- Encrypting within the goroutine after handler completion
- Documenting that secret field encryption only occurs for synchronously completed actions
- Marking the action as requiring post-completion encryption and handling it in
GetActionStatuspkg/actions/args.go (4)
489-506: Serialization inconsistency with single-item variant.
NewResourceIdReturnField(line 434) serializes usingconfig.ResourceIdFieldproducing JSON fieldsresource_type_idandresource_id. However,NewResourceIdListReturnFielddirectly marshalsv2.ResourceId(line 493) which producesresource_typeandresource. This inconsistency could cause issues for consumers expecting uniform field names.func NewResourceIdListReturnField(key string, resourceIDs []*v2.ResourceId) (ReturnField, error) { listValues := make([]*structpb.Value, len(resourceIDs)) for i, resourceId := range resourceIDs { + if resourceId == nil { + return ReturnField{}, fmt.Errorf("resourceId at index %d cannot be nil", i) + } - jsonBytes, err := protojson.Marshal(resourceId) + basicResourceId := &config.ResourceIdField{ + ResourceTypeId: resourceId.ResourceType, + ResourceId: resourceId.Resource, + } + jsonBytes, err := protojson.Marshal(basicResourceId) if err != nil {
432-449: Add nil check for resourceId parameter.
NewResourceIdReturnFieldwill panic ifresourceIdis nil when accessingresourceId.ResourceTypeon line 435.func NewResourceIdReturnField(key string, resourceId *v2.ResourceId) (ReturnField, error) { + if resourceId == nil { + return ReturnField{}, fmt.Errorf("resourceId cannot be nil") + } basicResourceId := &config.ResourceIdField{
416-429: Add nil check for resource parameter.
NewResourceReturnFieldwill panic ifresourceis nil becauseresourceToBasicResourceis called before any validation. AlthoughresourceToBasicResourcehandles nil fields internally, passing a nilresourceitself will cause issues when accessing its fields.func NewResourceReturnField(key string, resource *v2.Resource) (ReturnField, error) { + if resource == nil { + return ReturnField{}, fmt.Errorf("resource cannot be nil") + } basicResource := resourceToBasicResource(resource)
469-487: Add nil check for list elements.If
resourcescontains a nil element,resourceToBasicResource(resource)on line 473 will cause issues.func NewResourceListReturnField(key string, resources []*v2.Resource) (ReturnField, error) { listValues := make([]*structpb.Value, len(resources)) for i, resource := range resources { + if resource == nil { + return ReturnField{}, fmt.Errorf("resource at index %d cannot be nil", i) + } basicResource := resourceToBasicResource(resource)
🧹 Nitpick comments (5)
proto/c1/config/v1/config.proto (1)
96-99: Consider field ordering in ResourceIdListField.The
resource_type_idfield is placed after thelistfield (field number 2). While this works, conventionally metadata fields likeresource_type_idare placed before collection fields for readability. This is a minor point and doesn't affect functionality.pkg/actions/args.go (1)
44-50: Potential precision loss for large integers.Converting
float64toint64can lose precision for integers larger than 2^53 (approximately 9 quadrillion). This is a limitation of JSON/structpb number representation. Consider documenting this limitation or adding validation.pkg/connectorbuilder/actions.go (1)
14-43: Interface duplication with pkg/actions.The
ActionManagerinterface defined here duplicates the one inpkg/actions/actions.go(lines 16-42 in relevant snippets). Consider using a type alias or importing frompkg/actionsto avoid potential drift.-// ActionManager defines the interface for managing actions in the connector builder. -// This is the internal interface used by the builder for dispatch. -// The *actions.ActionManager type implements this interface. -type ActionManager interface { - // ListActionSchemas returns all action schemas, optionally filtered by resource type. - // If resourceTypeID is empty, returns all actions (both global and resource-scoped). - // If resourceTypeID is set, returns only actions for that resource type. - ListActionSchemas(ctx context.Context, resourceTypeID string) ([]*v2.BatonActionSchema, annotations.Annotations, error) - // ... rest of interface -} +// ActionManager is an alias for the interface used by the builder for action dispatch. +// The *actions.ActionManager type implements this interface. +type ActionManager = actions.ActionManagerpkg/actions/actions.go (2)
308-312: Document that name parameter is ignored.The deprecated
RegisterActionmethod ignores thenameparameter and usesschema.GetName()instead. While this is mentioned in the deprecation comment, the actual method doesn't log or warn about this. Consider adding a debug log.func (r *resourceTypeActionRegistry) RegisterAction(ctx context.Context, name string, schema *v2.BatonActionSchema, handler ActionHandler) error { + if name != "" && name != schema.GetName() { + ctxzap.Extract(ctx).Debug("RegisterAction name parameter ignored, using schema name", + zap.String("provided_name", name), + zap.String("schema_name", schema.GetName())) + } return r.Register(ctx, schema, handler) }
322-341: UnregisterAction only handles global actions.This method only unregisters global actions from
a.schemasanda.handlers, but doesn't handle resource-scoped actions ina.resourceSchemasanda.resourceHandlers. If this is intentional, consider documenting it; otherwise, add support for unregistering resource-scoped actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
pb/c1/config/v1/config.pb.validate.go(2 hunks)pb/c1/connector/v2/action.pb.validate.go(2 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/actions/actions.go(9 hunks)pkg/actions/actions_test.go(6 hunks)pkg/actions/args.go(1 hunks)pkg/actions/args_test.go(1 hunks)pkg/cli/commands.go(1 hunks)pkg/connectorbuilder/actions.go(6 hunks)pkg/connectorbuilder/connectorbuilder.go(6 hunks)pkg/connectorbuilder/connectorbuilder_test.go(4 hunks)pkg/connectorbuilder/connectorbuilder_v2_test.go(1 hunks)pkg/connectorbuilder/resource_actions.go(1 hunks)pkg/connectorbuilder/resource_syncer.go(2 hunks)pkg/connectorrunner/runner.go(5 hunks)pkg/field/defaults.go(2 hunks)pkg/ratelimit/grpc.go(1 hunks)pkg/tasks/c1api/actions.go(3 hunks)pkg/tasks/local/action_invoker.go(4 hunks)pkg/tasks/local/action_schema_list.go(1 hunks)proto/c1/config/v1/config.proto(1 hunks)proto/c1/connector/v2/action.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- pb/c1/connectorapi/baton/v1/baton.pb.validate.go
- pkg/connectorbuilder/connectorbuilder_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/ratelimit/grpc.go
- pkg/actions/args_test.go
- pkg/cli/commands.go
- pb/c1/connector/v2/action.pb.validate.go
- proto/c1/connector/v2/action.proto
- proto/c1/connectorapi/baton/v1/baton.proto
- pkg/field/defaults.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_v2_test.gopkg/connectorbuilder/connectorbuilder.gopkg/actions/actions.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_v2_test.gopkg/connectorbuilder/connectorbuilder.gopkg/actions/actions.go
📚 Learning: 2025-09-02T15:06:42.596Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 410
File: pkg/sync/syncer.go:650-652
Timestamp: 2025-09-02T15:06:42.596Z
Learning: In the baton-sdk codebase, the syncIDClientWrapper middleware automatically adds ActiveSync annotations to all connector requests that support annotations. This eliminates the need to manually add ActiveSync annotations to individual ListResourceTypes, ListResources, ListEntitlements, ListGrants, and other connector calls in pkg/sync/syncer.go.
Applied to files:
pkg/connectorbuilder/resource_syncer.gopkg/actions/actions.go
🧬 Code graph analysis (12)
pkg/tasks/c1api/actions.go (1)
pb/c1/connector/v2/action.pb.go (2)
ListActionSchemasRequest_builder(1041-1047)InvokeActionRequest_builder(471-481)
pkg/actions/actions_test.go (2)
internal/connector/connector.go (1)
Register(459-500)pb/c1/connector/v2/action.pb.go (1)
BatonActionStatus_BATON_ACTION_STATUS_COMPLETE(35-35)
pkg/connectorbuilder/resource_syncer.go (2)
pkg/connectorbuilder/resource_actions.go (1)
ResourceActionProvider(23-27)vendor/google.golang.org/grpc/rpc_util.go (1)
Errorf(973-975)
pkg/tasks/local/action_invoker.go (1)
pkg/tasks/tasks.go (1)
Manager(12-17)
pkg/connectorbuilder/resource_actions.go (2)
pkg/annotations/annotations.go (1)
Annotations(12-12)pkg/actions/actions.go (1)
ActionRegistry(91-98)
pkg/connectorbuilder/connectorbuilder.go (1)
pkg/actions/actions.go (2)
ActionManager(105-118)NewActionManager(120-128)
pkg/connectorrunner/runner.go (2)
pkg/tasks/local/action_invoker.go (1)
NewActionInvoker(86-93)pkg/tasks/local/action_schema_list.go (1)
NewListActionSchemas(73-77)
pkg/tasks/local/action_schema_list.go (2)
pb/c1/connectorapi/baton/v1/baton.pb.go (4)
Task(82-113)Task(126-126)Task_builder(917-946)Task_ActionListSchemasTask_builder(3700-3706)pkg/tasks/tasks.go (1)
Manager(12-17)
pkg/connectorbuilder/actions.go (3)
pb/c1/connector/v2/action_protoopaque.pb.go (2)
BatonActionSchema(210-222)BatonActionSchema(235-235)pkg/actions/actions.go (1)
ActionRegistry(91-98)pkg/connectorbuilder/connectorbuilder.go (1)
ConnectorBuilder(50-54)
pkg/actions/args.go (1)
pb/c1/config/v1/config.pb.go (4)
Resource(1148-1157)Resource(1170-1170)ResourceIdField(1408-1414)ResourceIdField(1427-1427)
pb/c1/config/v1/config.pb.validate.go (1)
pb/c1/config/v1/config.pb.go (20)
Field_ResourceIdField(1111-1113)Field_ResourceIdField(1138-1138)Field_ResourceIdListField(1115-1117)Field_ResourceIdListField(1140-1140)Field_ResourceField(1119-1122)Field_ResourceField(1142-1142)Field_ResourceListField(1124-1126)Field_ResourceListField(1144-1144)Resource(1148-1157)Resource(1170-1170)Field(549-572)Field(585-585)ResourceField(1283-1288)ResourceField(1301-1301)ResourceListField(1351-1356)ResourceListField(1369-1369)ResourceIdField(1408-1414)ResourceIdField(1427-1427)ResourceIdListField(1479-1485)ResourceIdListField(1498-1498)
pkg/actions/actions.go (3)
pb/c1/connector/v2/action.pb.go (6)
BatonActionSchema(210-223)BatonActionSchema(236-236)ActionType_ACTION_TYPE_UNSPECIFIED(85-85)BatonActionStatus(28-28)BatonActionStatus(69-71)BatonActionStatus(73-75)pkg/connectorbuilder/actions.go (1)
ActionManager(17-43)pb/c1/config/v1/config.pb.go (2)
Field(549-572)Field(585-585)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (38)
pkg/connectorbuilder/connectorbuilder_v2_test.go (1)
101-101: LGTM!The change from asserting a specific action ID to verifying the ID is non-empty is appropriate, as action IDs are now generated dynamically by the ActionManager.
pkg/tasks/c1api/actions.go (3)
39-45: LGTM!The request builder pattern with conditional
ResourceTypeIdcorrectly enables resource-scoped action filtering when provided.
128-136: LGTM!The request builder pattern consistently applies resource-scoped action invocation when
ResourceTypeIdis provided.
185-185: LGTM!The log message correction from "ActionInvoke response" to "ActionStatus response" improves consistency and clarity.
pkg/connectorbuilder/resource_actions.go (1)
12-27: LGTM!The new
ResourceActionHandlertype andResourceActionProviderinterface provide a clean, well-documented API for resource-scoped actions.pkg/connectorrunner/runner.go (4)
287-294: LGTM!The addition of
resourceTypeIDtoinvokeActionConfigcorrectly enables resource-scoped action invocation.
484-510: LGTM!The updated
WithOnDemandInvokeActionsignature and newWithOnDemandListActionSchemasfunction correctly expose resource-type filtering capabilities for on-demand action operations.
764-769: LGTM!The validation logic correctly accounts for the new
listActionSchemasConfigcase when checking for requiredc1zPath.
790-794: LGTM!The switch cases correctly wire through
resourceTypeIDfor action invocation and handle the new list-action-schemas path.pkg/connectorbuilder/resource_syncer.go (2)
371-371: LGTM!The context parameter is now properly used in the function body for resource action registration.
390-400: LGTM!The integration of
ResourceActionProvideris well-placed and includes appropriate error handling with clear error messages.pkg/tasks/local/action_invoker.go (5)
24-26: LGTM!The addition of
resourceTypeIDfield enables resource-scoped action invocation.
42-44: LGTM!The task builder correctly includes
ResourceTypeIdto support resource-scoped actions.
57-64: LGTM!The request builder pattern with conditional
ResourceTypeIdis consistent with the broader resource-scoped action support.
70-75: LGTM!The enhanced logging provides valuable visibility into action execution with action ID, name, status, and response details.
85-92: LGTM!The updated constructor signature correctly accepts
resourceTypeIDto enable resource-scoped action invocation.pkg/actions/actions_test.go (3)
127-149: LGTM!The test updates correctly reflect the new API signatures:
Registerreplaces deprecatedRegisterActionListActionSchemasnow acceptsresourceTypeIDparameterInvokeActionnow acceptsresourceTypeIDand encryption config parameters
167-179: LGTM!The async action test correctly uses the updated API signatures for registration, listing, and invocation.
208-247: LGTM!The goroutine leak tests correctly use the updated API signatures while maintaining their validation of proper resource cleanup.
pkg/tasks/local/action_schema_list.go (1)
17-77: LGTM!The new
localListActionSchemasimplementation correctly supports optional resource-type filtering:
- Uses
sync.Oncefor single-initialization semantics- Request builder conditionally includes
ResourceTypeId- Logging appropriately differentiates filtered vs. unfiltered results
- Clean public constructor with clear documentation
proto/c1/config/v1/config.proto (2)
64-70: New resource field types added to Field oneof.The addition of
ResourceIdField,ResourceIdListField,ResourceField, andResourceListFieldas return types for actions is well-structured. The comment on lines 67-68 clarifies thatResourceFieldandResourceListFieldare intended specifically as action return types.
73-81: Duplication justified by import cycle avoidance.The comment clearly documents why
Resourceis partially duplicated from the connector package. This is a pragmatic approach to avoid proto import cycles.pkg/connectorbuilder/connectorbuilder.go (3)
101-102: Good pattern: concrete type for registration, interface for dispatch.Creating
actionMgras a concrete*actions.ActionManagerfor registration while storing it as theActionManagerinterface allows full access during setup and proper abstraction during runtime.Also applies to: 119-119
149-154: GlobalActionProvider integration is clean.The new
GlobalActionProviderinterface is properly checked and invoked, with appropriate error wrapping. This provides a cleaner registration pattern compared to the deprecated interfaces.
404-406: HasActions() replaces nil check correctly.Using
HasActions()is more semantically correct than checking for nil, as it properly handles the case where an action manager exists but has no registered actions.pkg/actions/args.go (2)
90-98: Good fallback handling for field name variations.Supporting both
resource_type_id/resource_idandresource_type/resourcefield names provides backward compatibility for different serialization formats.Also applies to: 100-107
274-296: Nil checks properly added to conversion helpers.Both
resourceToBasicResourceandbasicResourceToResourcenow correctly handle nilIdandParentResourceIdfields. This addresses the previous review concerns.Also applies to: 298-320
pkg/connectorbuilder/actions.go (2)
190-197: Legacy action bridge is well-designed.The
registerLegacyActionhelper cleanly wraps the deprecatedCustomActionManagerinvocation pattern into the newActionHandlersignature, enabling seamless migration.
199-236: Comprehensive legacy interface handling.The
addActionManagerfunction properly handles bothCustomActionManagerandRegisterActionManagerLimitedinterfaces, extracting their schemas and re-registering them through the unified registry. The nil check forcustomManageron line 222 is appropriate.pkg/actions/actions.go (3)
89-98: Clean ActionRegistry interface design.The interface provides a simple
Registermethod using schema name while maintaining backward compatibility with the deprecatedRegisterActionmethod.
104-118: Well-structured ActionManager with resource-scoped support.The separation of global actions (
schemas,handlers) from resource-scoped actions (resourceSchemas,resourceHandlers) with a sharedactionsmap for outstanding actions is a clean design. The mutex protects all state appropriately.
270-288: Good duplicate ActionType validation.The check prevents registering multiple actions with the same
ActionTypefor a resource type (except forUNSPECIFIEDandDYNAMICtypes). This prevents ambiguity in action dispatch.pb/c1/config/v1/config.pb.validate.go (6)
709-871: LGTM! Resource field type validation cases correctly generated.The four new oneof cases for resource-related field types follow the same pattern as existing field type validations, with proper typed-nil checks, conditional validation based on the
allflag, and correct error wrapping.
954-1147: LGTM! Resource validation implementation is complete and consistent.The validation logic properly handles embedded messages (ResourceId, ParentResourceId) and repeated fields (Annotations) with index-aware error reporting. All required error types and interface implementations are present.
1149-1276: LGTM! ResourceField validation correctly handles embedded Resource.The implementation follows the standard pattern for validating messages with embedded fields.
1278-1412: LGTM! ResourceListField validation properly handles repeated Resource fields.The iteration logic and index-aware error reporting match the established patterns for list field validation.
1414-1516: LGTM! ResourceIdField validation structure is correct.The absence of validation rules for ResourceTypeId and ResourceId fields is expected when no constraints are defined in the proto file. The error types are properly generated for consistency.
1518-1654: LGTM! ResourceIdListField validation correctly validates list items.The implementation properly iterates through the List field and validates each ResourceIdField item, with no validation rules for ResourceTypeId as expected.
| } | ||
|
|
||
| message InvokeActionRequest { | ||
| string name = 1; |
There was a problem hiding this comment.
What do you think about enforcing unique names across the connector? That way if there's a bug where resource type is missing for some reason, we don't invoke the wrong action. We can remove this constraint once we're more confident in the new actions.
There was a problem hiding this comment.
I don't love it since it hurts the experience a bit on the c1 side. Having consistency between actions and their names will be useful.
There was a problem hiding this comment.
Are action names shown to users? I thought we showed the display name & description. The action name was just meant to be an identifier.
There was a problem hiding this comment.
Yeah, the action names are displayed to users. I think we require a name, but not a display name in the schema, so display names aren't consistently set.
| ResourceIdField parent_resource_id = 2; | ||
| string display_name = 3; | ||
| string description = 4; | ||
| repeated google.protobuf.Any annotations = 5; |
There was a problem hiding this comment.
Do we have annotations in any other fields? I'm not sure what we'd use this for, as it's not something we can render as inputs in the UI or expose in the CLI.
If we want actions like create_resource to return resources, I think we'll want to return actual resource protos instead of this. Otherwise we'll be out of sync with the actual resource proto. eg: Missing fields like the external id, or having different validation for fields like name & description.
There was a problem hiding this comment.
This isn't an actual field, and is more a 'light' representation of the basics of a resource. There are some import cycle issues including the actual resource proto within the config proto, so this 'mirrors' part of it.
The end goal is not to have a fully synced object, but a basic one that can be used to make a placeholder object with the correct source so that it shows up immediately, but will be actually fully populated on the next sync. It has annotations so that it can include a trait or anything else.
I'll see if I can find a workaround for the import cycle, but I imagine that will result in other refactoring that will break compat.
There was a problem hiding this comment.
I refactored the resource related fields. No luck on the import cycle.
proto/c1/config/v1/config.proto
Outdated
|
|
||
| message ResourceIdListField { | ||
| repeated ResourceIdField list = 1; | ||
| string resource_type_id = 2; |
There was a problem hiding this comment.
If resource type is already in the resource ID, why have it in two places? If this is meant to be a list of resources of the same type, I'd make it:
message ResourceIdListField {
repeated string resource_ids = 1;
string resource_type_id = 2;
}
So that there's no way it could be inconsistent.
There was a problem hiding this comment.
The idea here is that the resource_type_id field acts as a configuration for the schema saying indicating that this is a list of resources of a given type. I think we'll want to move this into another PR though, so will get this cleaned up.
There was a problem hiding this comment.
This is refactored to match the other configs. Now has a rules field where you can specify the allowed resource IDs
| } | ||
| for _, existingActionType := range existingSchema.GetActionType() { | ||
| if newActionType == existingActionType { | ||
| return fmt.Errorf("action type %s already registered for resource type %s (existing action: %s)", newActionType.String(), resourceTypeID, existingName) |
There was a problem hiding this comment.
This is a new constraint. Previously we allowed actions to register the same types. eg: enable account would have types ACTION_TYPE_ACCOUNT and ACTION_TYPE_ACCOUNT_ENABLE, and disable account would have types ACTION_TYPE_ACCOUNT and ACTION_TYPE_ACCOUNT_DISABLE.
For global actions, types are like tags to help filter in the UI. You could even have the exact same action type(s) for different actions, and the name & description would differentiate them in the UI. Do we want to require unique action types for resource actions?
There was a problem hiding this comment.
Yes, this was intentional to force connector developers to build one action for each type per resource. The schema should handle varying options if that is required. The future vision is to deprecate the global actions and move them to be resource actions where it makes sense. Allowing multiples of the same type for ACTION_TYPE_ACCOUNT was quick way for us to get the lifecycle actions that we needed, but isn't sustainable in the long term.
pkg/actions/actions.go
Outdated
| // Get the plaintext string value | ||
| stringValue, ok := fieldValue.GetKind().(*structpb.Value_StringValue) | ||
| if !ok { | ||
| l.Warn("secret field is not a string, skipping encryption", zap.String("field", fieldName)) |
There was a problem hiding this comment.
I think we should error here, as it means the connector author intended to encrypt the field but sent incorrect data. The unencrypted data is still in result.Fields, so we should at least delete it.
There was a problem hiding this comment.
Yes. The secret stuff was meant so we can add support for account creation via resource actions, but it isn't fully implemented. Will see if we can pull it out for now.
proto/c1/connector/v2/action.proto
Outdated
| ACTION_TYPE_USER_SUSPEND = 11; | ||
| ACTION_TYPE_USER_RESUME = 12; | ||
| ACTION_TYPE_USER_LOCK = 13; | ||
| ACTION_TYPE_USER_UNLOCK = 14; |
There was a problem hiding this comment.
I think we should omit most of these action types for now, and only add them once we decide to deprecate grant/revoke (since that's what most of these are supposed to replace).
Add action task to proto Checkin Fix function prototype after rebase Refactor action registration Move resource id to the schema if it is required. Add helpers for working with args struct Drop bulk invoke and don't require the resource type during action registration Add helper for working with resource id lists in the schema Prevent resource actions with the same type from being added to a single resource type Don't include resource action schemas as part of the capabilities output. Add return types for a partially populated resources from actions. Also add more action arg getter and setters with tests. Lint fixes More lint fixes Unify baton actions and resource actions Don't use the optional keyword in the baton action schema proto for resource_type_id Drop more optional proto fields. Fixes after dropping the optional proto fields. Make protogen after rebase Add HasActions method to ActionManager and update action management interfaces - Introduced HasActions method to check for registered actions in ActionManager. - Updated ActionManager and GlobalActionProvider interfaces for better action management. - Refactored action registration logic to support legacy action managers. - Enhanced tests to cover new functionality and ensure backward compatibility. Refactor action registration methods for consistency and deprecation - Renamed RegisterAction to Register in ActionRegistry and ActionManager for improved clarity. - Updated all references in tests and connector builder to use the new Register method. - Marked RegisterAction as deprecated to guide future development towards the new method. - Ensured backward compatibility by maintaining the old method signatures where necessary. fix lint line-length violations Fix broken test Refactor InvokeAction method signature in ActionManager for improved readability - Reformatted the InvokeAction method signature to enhance clarity and maintainability. - Ensured consistent formatting across the ActionManager interface. Refactor resource conversion functions to handle optional fields - Updated resourceToBasicResource and basicResourceToResource functions to conditionally set ResourceId and ParentResourceId based on their presence. - Improved code readability and maintainability by reducing redundancy in struct initialization.
jirwin
force-pushed
the
jirwin/resource-actions
branch
from
dd6a43e to
044a1d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pkg/connectorbuilder/actions.go (2)
124-128: Error handling loses context.Line 127 replaces the original error from
GetActionSchemawith a generic "not found" message, discarding potentially useful debugging information.
173-177: Error handling loses context.Line 176 discards the original error from
GetActionStatus, similar to the issue inGetActionSchema.pkg/actions/actions.go (3)
580-619: Fix goroutine leak from unbuffered channel.Line 591 creates an unbuffered channel. If the 1-second timeout occurs (line 613) before the handler completes, the function returns but the goroutine continues running. When it tries to send on
done(line 607), it will block forever because no receiver is listening, leaking the goroutine.Apply this fix:
- done := make(chan struct{}) + done := make(chan struct{}, 1)
406-473: Decryption remains unimplemented.The
decryptSecretFieldsfunction still contains the TODO from previous review cycles and passes encrypted data through unchanged (lines 467-469). While past discussions indicated this might be pulled out, the presence of this incomplete implementation alongside the functionalencryptSecretFieldscreates an asymmetry: action handlers will receive encrypted inputs but can produce plaintext outputs that get encrypted.Consider either completing the decryption implementation or removing the incomplete encryption infrastructure until the full feature is ready.
622-705: Fix goroutine leak and unencrypted output on timeout.Two issues here:
Goroutine leak: Line 673 creates an unbuffered channel. When the 1-second timeout occurs (line 699), the function returns but the goroutine will eventually block on line 687 trying to send, leaking the goroutine.
Unencrypted secrets on timeout: When returning on timeout (line 700),
oa.Rvhasn't been encrypted yet. If the handler completes after timeout and writes secret fields tooa.Rv, those will remain unencrypted whenGetActionStatusis called later, potentially exposing sensitive data.Fix the channel:
- done := make(chan struct{}) + done := make(chan struct{}, 1)For the encryption issue, consider either:
- Encrypting output within the goroutine before storing in
oa.Rv, or- Documenting that secret field encryption only occurs for synchronously completed actions and requiring callers to retrieve encrypted results via
GetActionStatusafter async completion, or- Not supporting secret fields in async action responses
🧹 Nitpick comments (6)
pkg/connectorbuilder/resource_actions.go (1)
12-19: Clarify the distinction betweenResourceActionHandlerand the registry handler signature.The
ResourceActionHandlertype signature includesresourceID *v2.ResourceId, but in the test file (connectorbuilder_test.golines 1278-1284), the handler registered viaregistry.Registeruses a simpler signaturefunc(ctx context.Context, args *structpb.Struct).If
ResourceActionHandleris intended for a different use case (e.g., when the action needs the specific resource instance ID at invocation time), consider adding a doc comment clarifying when to use this type versus registering handlers directly with the registry.pkg/tasks/local/action_schema_list.go (2)
71-77: Unusedctxparameter in constructor.The
ctxparameter is not used inNewListActionSchemas. Consider removing it if not needed for consistency with similar constructors, or document its intended future use.-func NewListActionSchemas(ctx context.Context, resourceTypeID string) tasks.Manager { +func NewListActionSchemas(resourceTypeID string) tasks.Manager { return &localListActionSchemas{ resourceTypeID: resourceTypeID, } }Note: This would require updating the call site in
pkg/connectorrunner/runner.goas well.
55-66: Consider reducing log verbosity for schemas.Using
zap.Any("schemas", resp.GetSchemas())could produce very verbose logs when many schemas are registered. Consider logging only schema names or count in production, with full schema details at debug level.pkg/connectorbuilder/connectorbuilder_test.go (1)
520-522: Consider returning a proper stub or documenting why nil is acceptable.
GetTypeRegistryreturnsnil, nilwhich might cause nil pointer dereferences if callers don't handle this case. If this is intentional test behavior, a comment would help clarify.proto/c1/config/v1/config.proto (2)
73-81: Document the synchronization strategy for duplicateResourcemessage.The comment explains this is to avoid import cycles, which is reasonable. However, consider documenting how to keep these definitions in sync with
c1/connector/v2/resource.prototo prevent drift over time.
91-99: Clarify the dualresource_type_idusage inResourceIdListField.Per past review discussion, the top-level
resource_type_id(line 98) serves as schema configuration indicating "this is a list of resources of a given type," while individualResourceIdFieldentries in the list may carry their ownresource_type_id.Consider adding a comment to clarify this distinction to avoid future confusion:
message ResourceIdListField { repeated ResourceIdField list = 1; + // Schema-level constraint: when set, indicates this list should only contain + // resources of the specified type. Individual entries may have their own + // resource_type_id for validation. string resource_type_id = 2; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (23)
pb/c1/config/v1/config.pb.validate.go(2 hunks)pb/c1/connector/v2/action.pb.validate.go(2 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/actions/actions.go(9 hunks)pkg/actions/actions_test.go(6 hunks)pkg/actions/args.go(1 hunks)pkg/actions/args_test.go(1 hunks)pkg/cli/commands.go(1 hunks)pkg/connectorbuilder/actions.go(6 hunks)pkg/connectorbuilder/connectorbuilder.go(6 hunks)pkg/connectorbuilder/connectorbuilder_test.go(4 hunks)pkg/connectorbuilder/connectorbuilder_v2_test.go(1 hunks)pkg/connectorbuilder/resource_actions.go(1 hunks)pkg/connectorbuilder/resource_syncer.go(2 hunks)pkg/connectorrunner/runner.go(5 hunks)pkg/field/defaults.go(2 hunks)pkg/ratelimit/grpc.go(1 hunks)pkg/tasks/c1api/actions.go(3 hunks)pkg/tasks/local/action_invoker.go(4 hunks)pkg/tasks/local/action_schema_list.go(1 hunks)proto/c1/config/v1/config.proto(1 hunks)proto/c1/connector/v2/action.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pb/c1/connectorapi/baton/v1/baton.pb.validate.go
- pkg/ratelimit/grpc.go
- pkg/cli/commands.go
- pkg/actions/args_test.go
- pkg/tasks/c1api/actions.go
- pkg/connectorbuilder/connectorbuilder_v2_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-02T15:06:42.596Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 410
File: pkg/sync/syncer.go:650-652
Timestamp: 2025-09-02T15:06:42.596Z
Learning: In the baton-sdk codebase, the syncIDClientWrapper middleware automatically adds ActiveSync annotations to all connector requests that support annotations. This eliminates the need to manually add ActiveSync annotations to individual ListResourceTypes, ListResources, ListEntitlements, ListGrants, and other connector calls in pkg/sync/syncer.go.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.gopkg/actions/actions.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.gopkg/connectorbuilder/connectorbuilder.gopkg/actions/actions.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.gopkg/connectorbuilder/connectorbuilder.gopkg/actions/actions.go
🧬 Code graph analysis (9)
pkg/connectorbuilder/resource_syncer.go (1)
pkg/connectorbuilder/resource_actions.go (1)
ResourceActionProvider(23-27)
pkg/tasks/local/action_schema_list.go (3)
pb/c1/connectorapi/baton/v1/baton.pb.go (3)
Task(82-113)Task(126-126)Task_builder(917-946)pb/c1/connector/v2/action_protoopaque.pb.go (1)
ListActionSchemasRequest_builder(1058-1064)pb/c1/connector/v2/action.pb.go (1)
ListActionSchemasRequest_builder(1041-1047)
pkg/field/defaults.go (2)
pkg/field/fields.go (2)
StringField(201-217)BoolField(179-199)pkg/field/field_options.go (6)
WithHidden(77-82)WithDescription(47-53)WithPersistent(129-135)WithExportTarget(103-111)ExportTargetNone(97-97)WithDefaultValue(62-68)
pkg/tasks/local/action_invoker.go (2)
pb/c1/connector/v2/action_protoopaque.pb.go (1)
InvokeActionRequest_builder(478-488)pb/c1/connector/v2/action.pb.go (1)
InvokeActionRequest_builder(471-481)
pkg/connectorrunner/runner.go (2)
pkg/tasks/local/action_invoker.go (1)
NewActionInvoker(109-116)pkg/tasks/local/action_schema_list.go (1)
NewListActionSchemas(73-77)
pkg/actions/actions.go (4)
pb/c1/connector/v2/action.pb.go (2)
BatonActionSchema(210-223)BatonActionSchema(236-236)pkg/annotations/annotations.go (1)
Annotations(12-12)pb/c1/config/v1/config.pb.go (2)
Field(549-572)Field(585-585)pkg/crypto/crypto.go (1)
NewEncryptionManager(57-63)
pkg/actions/args.go (1)
pb/c1/config/v1/config.pb.go (4)
Resource(1148-1157)Resource(1170-1170)ResourceIdField(1408-1414)ResourceIdField(1427-1427)
pkg/connectorbuilder/actions.go (3)
pb/c1/connector/v2/action_protoopaque.pb.go (2)
BatonActionSchema(210-222)BatonActionSchema(235-235)pb/c1/connector/v2/action.pb.go (2)
BatonActionSchema(210-223)BatonActionSchema(236-236)pkg/actions/actions.go (1)
ActionRegistry(91-98)
pb/c1/config/v1/config.pb.validate.go (1)
pb/c1/config/v1/config.pb.go (20)
Field_ResourceIdField(1111-1113)Field_ResourceIdField(1138-1138)Field_ResourceIdListField(1115-1117)Field_ResourceIdListField(1140-1140)Field_ResourceField(1119-1122)Field_ResourceField(1142-1142)Field_ResourceListField(1124-1126)Field_ResourceListField(1144-1144)Resource(1148-1157)Resource(1170-1170)Field(549-572)Field(585-585)ResourceField(1283-1288)ResourceField(1301-1301)ResourceListField(1351-1356)ResourceListField(1369-1369)ResourceIdField(1408-1414)ResourceIdField(1427-1427)ResourceIdListField(1479-1485)ResourceIdListField(1498-1498)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (36)
pkg/field/defaults.go (1)
175-220: LGTM!The new field definitions for resource-scoped actions follow the established pattern consistently. All fields are properly configured with appropriate visibility and persistence settings, and the integration into
DefaultFieldsis correct.Also applies to: 352-358
pkg/actions/args.go (6)
14-70: LGTM!The basic argument extraction functions (
GetStringArg,GetIntArg,GetBoolArg) are well-implemented with proper nil checks and type safety. The consistent pattern of returning(value, bool)provides clear presence indication.
75-113: LGTM! Good backward compatibility support.The function handles both
resource_type_id/resource_idandresource_type/resourcefield naming conventions, providing resilience across different schema versions. The nested struct handling and type checks are properly implemented.
274-320: LGTM!The translation helpers properly handle nil
IdandParentResourceIdfields, preventing potential panics. The bidirectional conversion betweenv2.Resourceandconfig.Resourceis correctly implemented.
362-388: LGTM!The function includes proper nil checks and safely initializes the
Fieldsmap when necessary. Error messages are clear and helpful.
396-414: LGTM!The basic return field constructors are straightforward and correctly use
structpbvalue constructors.
513-527: LGTM! Excellent API ergonomics.The
NewReturnValuesfunction provides a clean, builder-style API for constructing action return values. The mandatorysuccessfield ensures consistent return structure, and the variadicfieldsparameter offers flexibility.pkg/connectorbuilder/actions.go (4)
14-50: LGTM!The new interface definitions provide a clean, unified API for action management. The separation between the internal
ActionManagerinterface and the publicGlobalActionProviderinterface is well-designed. TheresourceTypeIDparameter consistently enables resource-scoped action support.
94-115: LGTM!The
ListActionSchemasimplementation correctly extracts theresourceTypeIDfilter and delegates to the action manager with proper error handling and metrics recording.
138-164: LGTM!The
InvokeActionimplementation properly extractsresourceTypeIDandencryptionConfigsfrom the request and delegates to the action manager with appropriate error handling.
190-236: LGTM! Good backward compatibility support.The legacy action registration functions properly bridge deprecated interfaces to the new unified system. The use of empty
resourceTypeIDfor legacy actions correctly treats them as global actions, maintaining backward compatibility.proto/c1/connectorapi/baton/v1/baton.proto (1)
118-119: LGTM!The addition of optional
resource_type_idfields to bothActionListSchemasTaskandActionInvokeTaskis correct and maintains backward compatibility. The field comments clearly document their purpose.Also applies to: 131-132
pb/c1/connector/v2/action.pb.validate.go (1)
337-371: LGTM!The generated validation code correctly handles the new
EncryptionConfigsfield with proper embedded validation support for bothValidateAllandValidatemodes. The no-op comment forResourceTypeIdis appropriate for an optional string field without constraints.Also applies to: 1294-1294
pkg/connectorbuilder/resource_syncer.go (1)
371-371: LGTM!The integration of resource-scoped action registration is correctly implemented. The context parameter is now properly used, and error handling for both
GetTypeRegistryandResourceActionsis appropriate with clear error messages.Also applies to: 390-400
pkg/actions/actions_test.go (1)
127-128: LGTM!The test updates correctly reflect the new API signatures. The changes are mechanical adaptations to the updated method signatures (
Register,ListActionSchemas,InvokeAction) while maintaining the same test coverage and logic.Also applies to: 130-130, 139-139, 147-149, 167-167, 170-170, 179-179, 208-208, 214-214, 241-241, 247-247
pkg/tasks/local/action_invoker.go (2)
24-26: LGTM!The resource-type scoping is correctly implemented. The conditional inclusion of
ResourceTypeId(lines 62-64) ensures the field is only set when provided, maintaining clean separation between global and resource-scoped actions.Also applies to: 42-44, 57-64
72-77: LGTM! Improved observability and clear API.The enhanced logging (lines 72-77) provides better visibility into action invocations, and the updated
NewActionInvokersignature (lines 108-115) clearly documents the optional resource-type scoping with helpful inline comments.Also applies to: 108-115
pkg/connectorbuilder/resource_actions.go (1)
21-27: LGTM!Clean interface definition that allows resource builders to register resource-scoped actions. The design aligns with the registry pattern mentioned in past review comments.
pkg/tasks/local/action_schema_list.go (1)
30-40: LGTM!The
sync.Oncepattern correctly ensures only a single task is created, and the task properly includes the optionalResourceTypeIdfor filtering.pkg/connectorbuilder/connectorbuilder_test.go (4)
1181-1241: LGTM!Comprehensive test coverage for
GlobalActionProviderincluding schema listing, schema retrieval, action invocation, and status checking. The test properly validates the new action flow.
1243-1319: LGTM!Thorough test coverage for
ResourceActionProviderdemonstrating the resource-scoped action pattern. Tests verify filtering by resource type and proper invocation withresourceTypeID.
1321-1343: LGTM!Good test coverage for
HasActions()method verifying it correctly reports action registration status for both global and resource-scoped actions.
1345-1374: LGTM!Important backward compatibility test ensuring the legacy
RegisterActionManagerinterface continues to work correctly through the wrapper pattern.proto/c1/connector/v2/action.proto (2)
52-87: ExtensiveActionTypeenum expansion for resource lifecycle operations.The expanded action types provide good coverage for resource lifecycle operations (create/delete/mutate/enable/disable), user management, group management, role management, app management, and secret rotation. Per past review discussions, these types serve as tags for automation steps like 'Lock User' without requiring the end user to know the specific action name.
40-42: LGTM!The
resource_type_idfields onBatonActionSchemaandListActionSchemasRequestenable proper scoping and filtering of resource-specific actions. Good documentation with inline comments clarifying the optional nature of these fields.Also applies to: 133-134
proto/c1/config/v1/config.proto (1)
64-70: LGTM!The new field types properly extend the
Fieldoneof to support resource-related action arguments and return types. Good separation betweenResourceIdField(for references) andResourceField(for full resources).pkg/connectorrunner/runner.go (3)
486-512: LGTM!Good documentation explaining the optional
resourceTypeIDparameter for resource-scoped actions. The implementation correctly wires the new parameters through to the configuration structs.
774-805: Verifyc1zPathvalidation covers all cases correctly.The validation at lines 775-780 allows
c1zPathto be empty when certain configs are set. However,listActionSchemasConfigis not in this exclusion list, yet it's handled at line 804-805 without requiringc1zPath. This appears intentional since listing schemas doesn't need a C1Z file, but the condition could be clearer.Consider adding
listActionSchemasConfigto the exclusion list for clarity:if cfg.c1zPath == "" && cfg.eventFeedConfig == nil && cfg.createTicketConfig == nil && cfg.listTicketSchemasConfig == nil && cfg.getTicketConfig == nil && - cfg.bulkCreateTicketConfig == nil { + cfg.bulkCreateTicketConfig == nil && + cfg.listActionSchemasConfig == nil { return nil, errors.New("c1zPath must be set when in on-demand mode") }
801-805: LGTM!The new cases properly wire the resource-scoped action invocation and schema listing to their respective local task managers, correctly passing through the
resourceTypeIDparameter.pkg/connectorbuilder/connectorbuilder.go (2)
80-80: LGTM: Unified action manager approach.The refactor to use a single
ActionManagerinterface for both global and resource-scoped actions addresses the past review concern about avoiding deeply branching logic. The concrete type is created for registration and stored as the interface type for dispatch—a clean pattern.Also applies to: 101-102, 119-119
144-154: LGTM: GlobalActionProvider integration.The new global action registration path is clean and maintains backward compatibility. Error wrapping provides clear context.
pkg/actions/actions.go (4)
89-118: LGTM: Clean action registry design.The
ActionRegistryinterface and expandedActionManagerstruct properly separate global and resource-scoped actions while using a unified mutex for concurrency protection. The backward-compatible alias is a nice touch.
192-296: LGTM: Registration methods with proper validation.The registration methods properly validate schemas, handlers, and prevent duplicate registrations. The per-resource-type duplicate
ActionTypechecking (lines 271-288) correctly skipsUNSPECIFIEDandDYNAMICtypes, which aligns with the design decision discussed in past reviews.
298-320: LGTM: Resource type registry implementation.The
resourceTypeActionRegistryprovides a clean, scoped registry for each resource type. The delegation pattern and validation inGetTypeRegistryare appropriate.
224-228: LGTM: Schema listing and capability check.The
HasActions()method andListActionSchemas()filtering logic properly handle both global and resource-scoped actions. The behavior of returning all actions whenresourceTypeIDis empty supports discovery use cases.Also applies to: 346-379
pb/c1/config/v1/config.pb.validate.go (1)
709-1654: Generated validation code for new resource field types.This generated code adds validation support for the new
ResourceIdField,ResourceIdListField,ResourceField, andResourceListFieldtypes. The structure is consistent with existing validation patterns in this generated file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/actions/args.go (4)
417-430: Add nil check to prevent panic.The function will panic if
resourceis nil whenresourceToBasicResource(resource)is called on line 418.Apply this diff to add a nil check:
func NewResourceReturnField(key string, resource *v2.Resource) (ReturnField, error) { + if resource == nil { + return ReturnField{}, fmt.Errorf("resource cannot be nil") + } basicResource := resourceToBasicResource(resource)
433-449: Add nil check to prevent panic.The function will panic if
resourceIdis nil when accessingresourceId.ResourceTypeon line 435.Apply this diff to add a nil check:
func NewResourceIdReturnField(key string, resourceId *v2.ResourceId) (ReturnField, error) { + if resourceId == nil { + return ReturnField{}, fmt.Errorf("resourceId cannot be nil") + } basicResourceId := config.ResourceId_builder{
470-487: Add nil check for list elements to prevent panic.If
resourcescontains a nil element,resourceToBasicResource(resource)will panic on line 473.Apply this diff to add element validation:
func NewResourceListReturnField(key string, resources []*v2.Resource) (ReturnField, error) { listValues := make([]*structpb.Value, len(resources)) for i, resource := range resources { + if resource == nil { + return ReturnField{}, fmt.Errorf("resource at index %d cannot be nil", i) + } basicResource := resourceToBasicResource(resource)
489-506: Inconsistent serialization still present despite past fix.A past review comment indicated this was addressed in commits 1b62e09 to dd6a43e, but the current code on line 493 still marshals
resourceIddirectly, producing JSON fieldsresource_typeandresourceinstead ofresource_type_idandresource_idas used byNewResourceIdReturnField(line 434-437).This creates an inconsistent API where single resource IDs and lists of resource IDs use different field names.
Apply this diff to use consistent serialization:
func NewResourceIdListReturnField(key string, resourceIDs []*v2.ResourceId) (ReturnField, error) { listValues := make([]*structpb.Value, len(resourceIDs)) for i, resourceId := range resourceIDs { - jsonBytes, err := protojson.Marshal(resourceId) + if resourceId == nil { + return ReturnField{}, fmt.Errorf("resourceId at index %d cannot be nil", i) + } + basicResourceId := config.ResourceId_builder{ + ResourceTypeId: resourceId.ResourceType, + ResourceId: resourceId.Resource, + }.Build() + jsonBytes, err := protojson.Marshal(basicResourceId) if err != nil { return ReturnField{}, fmt.Errorf("failed to marshal resource id: %w", err) }
🧹 Nitpick comments (1)
proto/c1/config/v1/rules.proto (1)
198-203: Inconsistent casing in message names.
ResourceIDRulesuses uppercase "ID" whileRepeatedResourceIdRulesuses "Id". This inconsistency could cause confusion and may lead to inconsistent generated code naming.Consider aligning the casing:
-message RepeatedResourceIdRules { +message RepeatedResourceIDRules { repeated string allowed_resource_type_ids = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pb/c1/config/v1/config.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/config_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/rules.pb.gois excluded by!**/*.pb.gopb/c1/config/v1/rules_protoopaque.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
pb/c1/config/v1/config.pb.validate.go(2 hunks)pb/c1/config/v1/rules.pb.validate.go(1 hunks)pkg/actions/args.go(1 hunks)pkg/actions/args_test.go(1 hunks)proto/c1/config/v1/config.proto(1 hunks)proto/c1/config/v1/rules.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/actions/args_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pb/c1/config/v1/rules.pb.validate.go (1)
pb/c1/config/v1/rules.pb.go (4)
ResourceIDRules(1430-1435)ResourceIDRules(1448-1448)RepeatedResourceIdRules(1487-1492)RepeatedResourceIdRules(1505-1505)
pb/c1/config/v1/config.pb.validate.go (1)
pb/c1/config/v1/config.pb.go (22)
Field_ResourceIdField(1111-1113)Field_ResourceIdField(1138-1138)Field_ResourceIdSliceField(1115-1117)Field_ResourceIdSliceField(1140-1140)Field_ResourceField(1119-1122)Field_ResourceField(1142-1142)Field_ResourceSliceField(1124-1126)Field_ResourceSliceField(1144-1144)Resource(1148-1157)Resource(1170-1170)Field(549-572)Field(585-585)ResourceId(1283-1289)ResourceId(1302-1302)ResourceField(1354-1359)ResourceField(1372-1372)ResourceSliceField(1422-1427)ResourceSliceField(1440-1440)ResourceIdField(1479-1485)ResourceIdField(1498-1498)ResourceIdSliceField(1572-1578)ResourceIdSliceField(1591-1591)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (10)
proto/c1/config/v1/config.proto (3)
64-69: LGTM!The new oneof variants for resource-related field types follow the existing pattern and use sequential field numbers (105-108).
73-86: LGTM - documented design decision.The comment explaining the partial duplication to avoid import cycles is helpful. The Resource and ResourceId messages provide a lightweight representation suitable for action args/returns.
96-104: LGTM!ResourceIdField and ResourceIdSliceField are well-structured with optional rules for validation constraints.
pb/c1/config/v1/rules.pb.validate.go (1)
1-2: Generated code - no manual review required.This file is auto-generated by
protoc-gen-validate. The validation scaffolding forResourceIDRulesandRepeatedResourceIdRulesfollows the standard pattern and will be regenerated from the proto definitions.pb/c1/config/v1/config.pb.validate.go (1)
1-2: Generated code - no manual review required.This file is auto-generated by
protoc-gen-validate. The comprehensive validation scaffolding for the newResource,ResourceId,ResourceField,ResourceSliceField,ResourceIdField, andResourceIdSliceFieldmessages follows the standard pattern and will be regenerated from the proto definitions.pkg/actions/args.go (5)
14-242: LGTM! Well-structured argument extraction utilities.The Get* and Require* functions properly handle nil checks and provide flexible field name fallbacks (e.g., "resource_type_id"/"resource_type") for backward compatibility. The consistent error handling pattern across all functions is clean and maintainable.
247-358: LGTM! Resource field extraction is correct.Both single and list resource extraction functions properly handle the JSON round-trip through config.Resource and conversion via the helper functions.
274-320: LGTM! Nil-safe resource conversion helpers.The conversion functions properly handle nil resource IDs and parent resource IDs using explicit nil checks and safe Get* accessor methods. The builder pattern usage is clean and correct.
362-388: LGTM! Proper nil checks and error handling.The function correctly validates both args and resource parameters and provides clear error messages with proper error wrapping.
515-527: LGTM! Clean return value constructor.The function provides a convenient and clear way to construct action return values with the required success field and arbitrary additional fields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/connectorbuilder/actions.go (2)
122-126: Error information discarded in GetActionSchema.The original error from
GetActionSchemais replaced with a generic "not found" message, losing diagnostic information.This was flagged in a previous review. The fix would be:
actionSchema, annos, err := b.actionManager.GetActionSchema(ctx, request.GetName()) if err != nil { b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) - return nil, fmt.Errorf("error: action schema %s not found", request.GetName()) + return nil, fmt.Errorf("error: failed to get action schema %s: %w", request.GetName(), err) }
170-174: Error information discarded in GetActionStatus.Same issue as GetActionSchema - the underlying error is replaced with a generic message.
This was flagged in a previous review. The fix would be:
actionStatus, name, rv, annos, err := b.actionManager.GetActionStatus(ctx, request.GetId()) if err != nil { b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) - return nil, fmt.Errorf("error: action status for id %s not found", request.GetId()) + return nil, fmt.Errorf("error: failed to get action status for id %s: %w", request.GetId(), err) }pkg/actions/actions.go (2)
429-456: Unbuffered channel can cause goroutine leak on timeout.If the handler completes after the 1-second timeout, the goroutine will block indefinitely on
done <- struct{}{}because there's no receiver. This was flagged in a previous review.- done := make(chan struct{}) + done := make(chan struct{}, 1)
492-517: Same unbuffered channel issue in invokeResourceAction.The resource action invocation has the same goroutine leak risk on timeout.
- done := make(chan struct{}) + done := make(chan struct{}, 1)
🧹 Nitpick comments (1)
pkg/connectorbuilder/connectorbuilder_test.go (1)
519-521: Mock GetTypeRegistry returns nil registry which differs from real implementation.The mock returns
nil, nilbut the realActionManager.GetTypeRegistryreturns a valid registry. This could mask issues if tests rely on the registry being non-nil. Consider returning a proper mock registry if any test needs to exercise resource action registration through this path.func (t *testCustomActionManager) GetTypeRegistry(ctx context.Context, resourceTypeID string) (actions.ActionRegistry, error) { - return nil, nil + // Return a minimal mock or the actual ActionManager's registry if needed + return actions.NewActionManager(ctx), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector/v2/action.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/action_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
pb/c1/connector/v2/action.pb.validate.go(2 hunks)pkg/actions/actions.go(8 hunks)pkg/actions/actions_test.go(6 hunks)pkg/connectorbuilder/actions.go(6 hunks)pkg/connectorbuilder/connectorbuilder_test.go(4 hunks)proto/c1/connector/v2/action.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pb/c1/connector/v2/action.pb.validate.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-02T15:06:42.596Z
Learnt from: kans
Repo: ConductorOne/baton-sdk PR: 410
File: pkg/sync/syncer.go:650-652
Timestamp: 2025-09-02T15:06:42.596Z
Learning: In the baton-sdk codebase, the syncIDClientWrapper middleware automatically adds ActiveSync annotations to all connector requests that support annotations. This eliminates the need to manually add ActiveSync annotations to individual ListResourceTypes, ListResources, ListEntitlements, ListGrants, and other connector calls in pkg/sync/syncer.go.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter using a type switch and returns appropriate errors for nil or invalid connectors in the default case, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.go
📚 Learning: 2025-06-02T21:28:25.367Z
Learnt from: MarcusGoldschmidt
Repo: ConductorOne/baton-sdk PR: 393
File: pkg/test/integration_wrapper.go:54-54
Timestamp: 2025-06-02T21:28:25.367Z
Learning: The `connectorbuilder.NewConnector` function in the baton-sdk already validates the connector parameter and returns appropriate errors for nil or invalid connectors, so additional nil checks are not needed before calling this function.
Applied to files:
pkg/connectorbuilder/connectorbuilder_test.go
🧬 Code graph analysis (3)
pkg/actions/actions_test.go (3)
internal/connector/connector.go (1)
Register(468-509)pb/c1/connector/v2/action_protoopaque.pb.go (1)
BatonActionStatus_BATON_ACTION_STATUS_COMPLETE(35-35)pb/c1/connector/v2/action.pb.go (1)
BatonActionStatus_BATON_ACTION_STATUS_COMPLETE(35-35)
pkg/connectorbuilder/connectorbuilder_test.go (1)
pkg/actions/actions.go (2)
ActionRegistry(87-94)NewActionManager(116-124)
pkg/connectorbuilder/actions.go (3)
pb/c1/connector/v2/action_protoopaque.pb.go (2)
BatonActionSchema(210-222)BatonActionSchema(235-235)pb/c1/connector/v2/action.pb.go (2)
BatonActionSchema(210-223)BatonActionSchema(236-236)pkg/actions/actions.go (1)
ActionRegistry(87-94)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/action.proto
[failure] 6-6:
Import "c1/connector/v2/resource.proto" is unused.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (17)
pkg/actions/actions_test.go (2)
127-159: Test updates correctly align with new API signatures.The test properly exercises the updated
Register,ListActionSchemas, andInvokeActionAPIs with the newresourceTypeIDparameter (empty string for global actions). The assertions validate both successful completion and error handling paths.
201-260: Goroutine leak tests appropriately updated for new API.The goroutine leak tests now correctly use the updated method signatures. The tests validate that both normal completion and context cancellation scenarios don't leak goroutines, which is important given the async action handler implementation.
proto/c1/connector/v2/action.proto (2)
40-41: Resource type scoping fields properly added.The
resource_type_idfields are consistently added toBatonActionSchema,InvokeActionRequest, andListActionSchemasRequest, enabling the resource-scoped action functionality. The optional semantics (empty string = global) are documented appropriately.Also applies to: 93-94, 131-132
52-86: Action type enum definitions are properly generated but filtering/routing logic not yet visible in codebase.The new action types (lines 6-28) are correctly defined in the proto file and properly generated in action.pb.go. However, verification shows that while these types are used for duplicate-checking during schema registration, there is no actual filtering or routing logic based on these specific action types in the current codebase. The stated intent to "enable fine-grained automation filtering" cannot be verified from the existing code.
pkg/connectorbuilder/connectorbuilder_test.go (4)
1180-1240: Comprehensive test coverage for GlobalActionProvider.The test properly validates the full lifecycle: registration via
GlobalActions, listing schemas, retrieving individual schema, invoking the action, and checking status. The inline handler returning a result struct enables verification of the response content.
1242-1318: Resource-scoped action test validates filtering and scoping.The test correctly verifies that:
- Actions are listed when filtered by resource type
- The
resource_type_idis set on returned schemas- Invocation works with resource type specified
- Response content is properly returned
1320-1342: HasActions method tested for both global and resource-scoped scenarios.Good coverage of the
HasActions()method showing it returnsfalsewhen empty,trueafter global registration, andtrueafter resource-scoped registration viaRegisterResourceAction.
1344-1373: Backward compatibility wrapper test ensures legacy interfaces continue working.This test validates that the deprecated
RegisterActionManagerinterface still functions correctly through the new unified action management system, which is important for migration.pkg/connectorbuilder/actions.go (4)
14-42: Well-defined ActionManager interface for internal dispatch.The interface cleanly abstracts the action management operations needed by the builder. The method signatures properly support both global and resource-scoped actions through the optional
resourceTypeIDparameter.
44-49: GlobalActionProvider provides clean migration path from deprecated interfaces.This is the recommended interface for new connectors, avoiding the complexity of implementing a full
CustomActionManager. The registry pattern allows flexible action registration.
187-194: Legacy action bridge correctly wraps invocation.The
registerLegacyActionfunction properly adapts aCustomActionManageraction into the newActionHandlersignature. The empty string forresourceTypeIDcorrectly indicates a global action invocation.
196-232: addActionManager properly bridges deprecated interfaces.The function handles both
CustomActionManagerandRegisterActionManagerLimitedby extracting their schemas and re-registering them through the unified registry. This maintains backward compatibility while consolidating action management.pkg/actions/actions.go (5)
85-98: ActionRegistry interface provides clean abstraction for action registration.The interface supports both the new
Registermethod (using schema name) and the deprecatedRegisterActionfor backward compatibility. TheResourceTypeActionRegistryalias helps with migration.
100-124: ActionManager correctly initializes storage for global and resource-scoped actions.The struct properly separates global actions (
schemas,handlers) from resource-scoped actions (resourceSchemas,resourceHandlers), with a sharedactionsmap for outstanding action tracking. The mutex protects concurrent access.
226-292: RegisterResourceAction enforces unique action names and types per resource type.The implementation correctly:
- Validates inputs (resourceTypeID, schema, handler)
- Sets resourceTypeId on the schema
- Checks for duplicate action names
- Checks for conflicting ActionType values (excluding UNSPECIFIED and DYNAMIC)
Per past discussion, the ActionType uniqueness constraint is intentional to ensure one action per type per resource.
339-375: ListActionSchemas correctly handles both global and resource-scoped filtering.When
resourceTypeIDis empty, it returns all actions (global + resource-scoped). When specified, it returns only actions for that resource type. The empty slice return for unknown resource types is appropriate.
294-316: resourceTypeActionRegistry correctly delegates to RegisterResourceAction.The registry implementation properly captures the resourceTypeID and delegates to the ActionManager's
RegisterResourceActionmethod. This enables theResourceActionProviderpattern in resource syncers.
… by including the original error in the returned error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/actions/args.go (1)
498-518: Inconsistent serialization between single and list resource ID return fields.Line 505 directly marshals
v2.ResourceId, which produces JSON fieldsresource_typeandresource. However,NewResourceIdReturnField(line 440-443) usesconfig.ResourceId_builderand marshalsconfig.ResourceId, which producesresource_type_idandresource_id.While
GetResourceIdListArghas fallback logic to handle both naming conventions, this inconsistency could cause confusion for consumers expecting uniform field names across single and list return values.Apply this diff to align with the single-value version:
func NewResourceIdListReturnField(key string, resourceIDs []*v2.ResourceId) (ReturnField, error) { listValues := make([]*structpb.Value, len(resourceIDs)) for i, resourceId := range resourceIDs { if resourceId == nil { return ReturnField{}, fmt.Errorf("resource id at index %d cannot be nil", i) } - jsonBytes, err := protojson.Marshal(resourceId) + basicResourceId := config.ResourceId_builder{ + ResourceTypeId: resourceId.ResourceType, + ResourceId: resourceId.Resource, + }.Build() + jsonBytes, err := protojson.Marshal(basicResourceId) if err != nil { return ReturnField{}, fmt.Errorf("failed to marshal resource id: %w", err) }
🧹 Nitpick comments (1)
pkg/connectorbuilder/actions.go (1)
196-233: Consider using schema's resource_type_id when registering legacy actions.The
addActionManagerfunction lists all schemas (including resource-scoped ones) from legacy managers using an emptyresourceTypeID(lines 201, 222). However,registerLegacyActionthen registers all of these actions as global by passing an empty string forresourceTypeID(line 190).According to the PR objectives,
BatonActionSchemanow has aresource_type_idfield. If the schema object includes this field, consider usingschema.GetResourceTypeId()instead of an empty string when invoking the action inregisterLegacyAction. This would preserve resource-scoped behavior for any legacy actions that specified a resource type.Apply this diff to use the schema's resource type ID:
func registerLegacyAction(ctx context.Context, registry actions.ActionRegistry, schema *v2.BatonActionSchema, legacyManager CustomActionManager) error { handler := func(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { - _, _, resp, annos, err := legacyManager.InvokeAction(ctx, schema.GetName(), "", args) + _, _, resp, annos, err := legacyManager.InvokeAction(ctx, schema.GetName(), schema.GetResourceTypeId(), args) return resp, annos, err } return registry.Register(ctx, schema, handler) }Note: Verify that the proto-generated
BatonActionSchematype has aGetResourceTypeId()method before applying this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/actions/args.go(1 hunks)pkg/connectorbuilder/actions.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/actions/args.go (1)
pb/c1/config/v1/config.pb.go (6)
ResourceId(1283-1289)ResourceId(1302-1302)Resource(1148-1157)Resource(1170-1170)ResourceId_builder(1338-1343)Resource_builder(1261-1269)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: go-test (1.25.2, windows-latest)
- GitHub Check: go-lint
- GitHub Check: go-test (1.25.2, ubuntu-latest)
🔇 Additional comments (9)
pkg/actions/args.go (7)
1-10: LGTM: Clean package structure and appropriate imports.The package declaration and imports are well-organized and bring in the necessary dependencies for working with protobuf structs and the Baton SDK config/connector types.
12-242: LGTM: Robust argument extraction with good error handling.The argument getter functions follow a consistent pattern with proper nil checks, safe type assertions, and clear return semantics. The fallback logic in
GetResourceIDArgandGetResourceIdListArg(trying both "resource_type_id"/"resource_id" and "resource_type"/"resource" field names) is good for backward compatibility.
244-320: LGTM: Resource conversion helpers properly handle nil cases.The resource conversion functions now correctly handle nil
IdandParentResourceIdfields using the builder pattern and getter methods. Past nil-pointer concerns have been addressed.
322-388: LGTM: Resource list handling and setters are well-implemented.The list extraction and setter functions properly handle nil cases and follow the same marshal/unmarshal pattern as the single-value versions. Error handling is clear and consistent.
390-473: LGTM: Return field constructors are clean and type-safe.The
ReturnFieldtype and its constructors provide a type-safe API for building action return values. Nil checks in the resource constructors properly address earlier concerns.
475-496: LGTM: Resource list return field properly validates elements.The function correctly checks for nil elements before processing, addressing the concern from the previous review. Error handling is thorough and consistent.
520-539: LGTM: Clean helpers for constructing return values.
NewListReturnFieldandNewReturnValuesprovide a clean, ergonomic API for building action return structures. The consistent "success" field pattern inNewReturnValuesis good for standardization.pkg/connectorbuilder/actions.go (2)
122-126: Error wrapping improvements look good.The error handling now properly wraps underlying errors from
GetActionSchemaandGetActionStatus, preserving diagnostic information. This addresses the previous review feedback.Also applies to: 170-174
187-194: Clarify the design decision for legacy resource-scoped actions.On line 190,
registerLegacyActionpasses an empty string forresourceTypeIDwhen invoking legacy actions, which means all legacyCustomActionManageractions are registered as global actions, regardless of whether theirBatonActionSchemaspecifies aresource_type_id.Since
BatonActionSchemanow includes aresource_type_idfield, the current implementation silently ignores this field for legacy actions. If any deprecated connectors implemented resource-scoped actions usingCustomActionManager, those actions would now be invoked without resource context, potentially changing their behavior.While
CustomActionManageris deprecated and no existing implementations were found in the codebase, this design choice warrants a comment explaining whether this is intentional (e.g., treating all legacy actions as global for compatibility) or an oversight.
pkg/connectorrunner/runner.go
Outdated
| cfg.createTicketConfig == nil && | ||
| cfg.listTicketSchemasConfig == nil && | ||
| cfg.getTicketConfig == nil && | ||
| cfg.bulkCreateTicketConfig == nil { |
There was a problem hiding this comment.
did we also want to check cfg. listActionSchemasConfig == nil here?
…ActionSchemasConfig in on-demand mode.
…a builder pattern for improved clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connectorbuilder/actions.go (1)
204-241: Consider adding documentation for the expected input types.The
in interface{}parameter is very generic and relies on runtime type assertions to determine which deprecated interface is implemented. While this works correctly, adding a comment documenting thatinshould be eitherCustomActionManagerorRegisterActionManagerLimitedwould improve maintainability.Apply this diff to add clarifying documentation:
-// addActionManager handles deprecated CustomActionManager and RegisterActionManagerLimited interfaces -// by extracting their actions and registering them into the unified ActionManager. -func (b *builder) addActionManager(ctx context.Context, in interface{}, registry actions.ActionRegistry) error { +// addActionManager handles deprecated CustomActionManager and RegisterActionManagerLimited interfaces +// by extracting their actions and registering them into the unified ActionManager. +// The in parameter should implement either CustomActionManager or RegisterActionManagerLimited. +func (b *builder) addActionManager(ctx context.Context, in interface{}, registry actions.ActionRegistry) error {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connectorbuilder/actions.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connectorbuilder/actions.go (4)
pb/c1/connector/v2/action_protoopaque.pb.go (2)
BatonActionSchema(148-160)BatonActionSchema(173-173)pkg/annotations/annotations.go (1)
Annotations(12-12)pkg/actions/actions.go (1)
ActionRegistry(87-94)pkg/connectorbuilder/connectorbuilder.go (1)
ConnectorBuilder(50-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (3)
pkg/connectorbuilder/actions.go (3)
14-57: LGTM! Well-designed interface hierarchy.The new interface design cleanly separates concerns between global actions (GlobalActionProvider) and resource-scoped actions (ResourceActionProvider), while the internal ActionManager interface provides a unified dispatch surface. Documentation is comprehensive and deprecation guidance is clear.
100-193: LGTM! Clean delegation with proper error handling.The method implementations correctly delegate to the ActionManager interface and properly handle resource type scoping. The error messages are descriptive and use proper error wrapping as requested in previous reviews.
195-202: TheregisterLegacyActionfunction always passes an empty string forresourceTypeIDwhen invoking legacy actions, which means resource-scoped actions fromCustomActionManagerimplementations would lose their resource type context. However, this is code in a deprecated interface with no actual implementations found in the codebase.CustomActionManageris explicitly deprecated with clear migration guidance: useGlobalActionProviderfor global actions andResourceActionProviderfor resource-scoped actions. The newResourceActionProviderpattern correctly handles resource-scoped action registration and invocation with properresourceTypeIDhandling. No verification gap exists—the SDK is intentionally phasing out the legacy pattern in favor of better-designed alternatives.
Summary
This PR unifies and enhances the Baton SDK action system by introducing resource-scoped actions alongside the existing global actions. Key changes include:
ActionManagerto support both global actions and resource-scoped actions (keyed by resource type ID), with proper concurrency protection via mutex locksGlobalActionProviderfor registering global actions andResourceActionProviderfor registering actions scoped to specific resource types, replacing the deprecatedCustomActionManagerandRegisterActionManagerinterfacesActionRegistryinterface for consistent action registration across both global and resource-scoped contextsActionTypeenum values for resource lifecycle operations (create, delete, mutate, enable, disable), user management (suspend, lock, password reset, etc.), group management, role assignment, and secret rotationBatonActionSchemawithresource_type_idfield for scoping actions, andInvokeActionRequestto support resource-scoped invocation with encryption configsResourceIdField,ResourceIdListField,ResourceField, andResourceListFieldto the config proto for action arguments and return typesGetStringArg,GetIntArg,GetBoolArg,GetResourceIDArg,GetResourceIDListArg,SetResourceIDArg, etc.) for working with action arguments and structpb valuesActionTypefor the same resource typeNew Pattern
For registering global actions, this now mimics the pattern for resource actions.
Previously you'd implement something on the connector like this for global actions:
Now you implement them like this:
The key differences are the method name and the fact that the
Registerfunction doesn't take the schema name as its own argument, and expects the name to be defined on the schema.The old method is marked as deprecated, but will continue working as expected.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.