Resource lookup service & protos for more details create account followups #292
Conversation
WalkthroughThis pull request integrates a new account creation status feature across the connector, API, task management, and provisioning layers. It adds new fields, option functions, and interfaces to enable account creation status handling; introduces a new gRPC service with corresponding proto messages; and extends validation logic and task processing to support the new task type. Additionally, obsolete account creation schema validations are removed, and CLI, runner, and default configurations are updated to support on-demand status retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant Builder
participant AccountStatusManager
participant GRPCServer
participant ConnectorService
CLI->>Runner: Execute command with account creation status flag
Runner->>Builder: Initialize runner config (accountCreationTaskId set)
Builder->>AccountStatusManager: Request status using task ID
AccountStatusManager->>GRPCServer: Invoke GetAccountCreationStatus RPC
GRPCServer->>ConnectorService: Forward request to AccountCreationStatusService
ConnectorService-->>GRPCServer: Return status response
GRPCServer-->>AccountStatusManager: Relay response
AccountStatusManager-->>Builder: Provide account creation status
Builder-->>Runner: Deliver CreateAccountResponse
sequenceDiagram
participant TaskManager
participant TaskHandler
participant ConnectorClient
participant Logger
TaskManager->>TaskHandler: Dispatch GetAccountCreationStatus task
TaskHandler->>ConnectorClient: Request account creation status
ConnectorClient-->>TaskHandler: Return status data or error
TaskHandler->>Logger: Log the outcome of the request
TaskHandler-->>TaskManager: Finish task processing with result
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pb/c1/connector/v2/resource.pb.validate.go (2)
2239-2378: ConnectorAccountCreationSchema logic and map field validation.
This block enforces field map iteration and embedded validation checks. Everything appears logically consistent. One suggestion is to add clarity in documentation about possible map key usage and any constraints.
5885-5967: ActionRequiredResult next-step logic.
The new next-step logic forRequiresAdditionalInputvs.PollForConfirmationis well-structured. Unit tests or integration tests verifying transitions between these states would improve confidence in correctness.pb/c1/connector/v2/annotation_external_link.pb.validate.go (1)
Line range hint
75-75: Summary: Consider a better approach to implement these optimizations.Instead of manually modifying these generated files, I recommend:
- Create a PR in the protoc-gen-validate repository to implement this optimization.
- Once merged, update your protoc-gen-validate dependency to get the optimized code generation.
- Regenerate your proto files with the updated generator.
This approach ensures the optimization is properly maintained and automatically applied to all generated files.
Also applies to: 75-75, 76-76, 120-120
proto/c1/connector/v2/resource.proto (2)
7-8: LGTM! Consider enhancing the service documentation.The new imports and service definition are well-structured. The service's purpose in the account creation flow is clearly documented.
Consider adding an example response in the service documentation to illustrate a typical lookup result:
// ResourceLookupService is used in conjunction with create account flows to confirm confirmation or status of created accounts // For example, github would send an invite as part of its flow and return "poll for invite acceptance" with the token +// Example response: +// { +// "resource": { +// "id": {"resource_type": "user", "resource": "123"}, +// "display_name": "john.doe", +// "external_id": {"id": "456", "link": "https://github.com/users/456"} +// } +// } service ResourceLookupService {Also applies to: 79-84
173-212: Add validation rules for field properties.The account creation schema is well-structured, but consider adding validation rules for:
- Field order values (to ensure they're non-negative)
- Display name length
- Description length
- Default value constraints
Apply these validation rules:
message ConnectorAccountCreationSchema { map<string, Field> field_map = 1; message Field { - string display_name = 1; + string display_name = 1 [(validate.rules).string = { + min_bytes: 1 + max_bytes: 1024 + }]; bool required = 2; - string description = 3; + string description = 3 [(validate.rules).string = { + max_bytes: 4096 + ignore_empty: true + }]; string placeholder = 4; - int32 order = 5; + int32 order = 5 [(validate.rules).int32 = { + gte: 0 + }];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (32)
internal/connector/connector.go(2 hunks)pb/c1/c1z/v1/annotation_sync_details.pb.validate.go(1 hunks)pb/c1/c1z/v1/diff.pb.validate.go(4 hunks)pb/c1/connector/v2/annotation_entitlement.pb.validate.go(1 hunks)pb/c1/connector/v2/annotation_etag.pb.validate.go(3 hunks)pb/c1/connector/v2/annotation_external_link.pb.validate.go(1 hunks)pb/c1/connector/v2/annotation_external_ticket.pb.validate.go(1 hunks)pb/c1/connector/v2/annotation_grant.pb.validate.go(5 hunks)pb/c1/connector/v2/annotation_ratelimit.pb.validate.go(1 hunks)pb/c1/connector/v2/annotation_request.pb.validate.go(1 hunks)pb/c1/connector/v2/annotation_resource_tree.pb.validate.go(2 hunks)pb/c1/connector/v2/annotation_trait.pb.validate.go(9 hunks)pb/c1/connector/v2/annotation_v1_identifier.pb.validate.go(1 hunks)pb/c1/connector/v2/asset.pb.validate.go(5 hunks)pb/c1/connector/v2/config.pb.validate.go(9 hunks)pb/c1/connector/v2/connector.pb.validate.go(12 hunks)pb/c1/connector/v2/entitlement.pb.validate.go(3 hunks)pb/c1/connector/v2/event_feed.pb.validate.go(6 hunks)pb/c1/connector/v2/grant.pb.validate.go(9 hunks)pb/c1/connector/v2/resource.pb.validate.go(32 hunks)pb/c1/connector/v2/ticket.pb.validate.go(28 hunks)pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go(1 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(35 hunks)pb/c1/ratelimit/v1/ratelimit.pb.validate.go(12 hunks)pb/c1/reader/v2/entitlement.pb.validate.go(2 hunks)pb/c1/reader/v2/grant.pb.validate.go(6 hunks)pb/c1/reader/v2/resource.pb.validate.go(4 hunks)pb/c1/utls/v1/tls.pb.validate.go(1 hunks)pkg/sync/syncer_test.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/connector.proto(0 hunks)proto/c1/connector/v2/resource.proto(4 hunks)
💤 Files with no reviewable changes (1)
- proto/c1/connector/v2/connector.proto
✅ Files skipped from review due to trivial changes (16)
- pb/c1/reader/v2/resource.pb.validate.go
- pb/c1/connector/v2/event_feed.pb.validate.go
- pb/c1/connector/v2/annotation_resource_tree.pb.validate.go
- pb/c1/reader/v2/grant.pb.validate.go
- pb/c1/connector/v2/entitlement.pb.validate.go
- pkg/sync/syncer_test.go
- pb/c1/connector/v2/config.pb.validate.go
- pb/c1/connector/v2/asset.pb.validate.go
- pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go
- pb/c1/connector/v2/annotation_etag.pb.validate.go
- pb/c1/connectorapi/baton/v1/baton.pb.validate.go
- pb/c1/connector/v2/annotation_trait.pb.validate.go
- pb/c1/connector/v2/grant.pb.validate.go
- pb/c1/connector/v2/ticket.pb.validate.go
- pb/c1/ratelimit/v1/ratelimit.pb.validate.go
- pb/c1/connector/v2/connector.pb.validate.go
🔇 Additional comments (22)
pb/c1/connector/v2/resource.pb.validate.go (6)
181-181: Proactively initializing the slice with capacity is good.
All these lines appear to switch from a zero-capacity slice tomsgs := make([]string, 0, len(m)). This is a beneficial micro-optimization to reduce unnecessary allocations, especially when appending many error messages.Also applies to: 377-377, 567-567, 954-954, 1119-1119, 1250-1250, 1386-1386, 1580-1580, 1779-1779, 1945-1945, 2173-2173, 2516-2516, 2773-2773, 2995-2995, 3174-3174, 3297-3297, 3551-3551, 3757-3757, 3945-3945, 4053-4053, 4217-4217, 4332-4332, 4435-4435, 4540-4540
637-677: Consider adding test coverage for ResourceLookupServiceLookupResourceRequest.
This new request validation logic looks correct (checkingLookupTokenlength bounds). However, verifying edge cases (empty or overly largeLookupToken) in corresponding tests would be helpful.
679-693: MultiError pattern is consistent with the codebase.
This chunk introduces multi-error handling for ResourceLookupServiceLookupResourceRequest. The pattern neatly aggregates validation errors. Everything here aligns with the existing multi-error approach.
754-813: ResourceLookupServiceLookupResourceResponse validation.
This newly added validation for the embeddedResourceis correct. Consider unit tests to confirm the nested resource validations produce the expected errors on invalid inputs.
4606-4932: ConnectorAccountCreationSchema_Field expansions.
These new subfields (StringField,BoolField,IntField,MapField, etc.) add more flexibility to account-creation schema definitions. Ensure all usage sites handle these new variants appropriately.
5660-5751: PollForConfirmation flow.
The polling interval and timeout checks look good. It might be beneficial to confirm that time units are documented and handled consistently in the calling code. Otherwise, we risk confusion if either is incorrectly measured.pkg/types/types.go (2)
38-38: New ResourceLookupServiceClient capability.
AddingconnectorV2.ResourceLookupServiceClientto theConnectorClientinterface provides richer communication with the resource lookup functionality. This change appears beneficial and follows the same pattern as the other client interfaces.
21-21:⚠️ Potential issueNew ResourceLookupServiceServer interface requirement.
By extending theConnectorServerto includeconnectorV2.ResourceLookupServiceServer, all implementers must now provide the service. Ensure existing connectors are updated or they may break.pb/c1/connector/v2/annotation_request.pb.validate.go (1)
75-75: Improved slice creation in RequestIdMultiError.
Initializing the slice with capacitylen(m)is a positive shift for performance. This is consistent with similar changes in other multi-error types.pb/c1/c1z/v1/annotation_sync_details.pb.validate.go (1)
75-75:⚠️ Potential issueConsider updating protoc-gen-validate instead of manually modifying generated code.
While the optimization to pre-allocate the slice capacity is beneficial, this file is generated by protoc-gen-validate and shouldn't be manually modified as indicated by the header comment. Consider contributing this optimization to the protoc-gen-validate project instead.
Let's verify if this is a generated file and if similar changes exist in other generated files:
pb/c1/connector/v2/annotation_v1_identifier.pb.validate.go (1)
75-75: Same issue with modifying generated code.pb/c1/connector/v2/annotation_external_ticket.pb.validate.go (1)
76-76: Same issue with modifying generated code.pb/c1/connector/v2/annotation_external_link.pb.validate.go (1)
120-120: Same issue with modifying generated code.pb/c1/connector/v2/annotation_entitlement.pb.validate.go (1)
105-105: LGTM! Verify if this is a generated change.The slice pre-allocation optimization is a good practice. However, since this is a generated file, we should verify that this change was made by the protoc-gen-validate generator rather than manual editing.
✅ Verification successful
Confirmed: This is a generated change by protoc-gen-validate
The file contains explicit generation markers and the pattern appears in 27 other .pb.validate.go files, all generated by the same tool. Evidence:
- File header: "// Code generated by protoc-gen-validate. DO NOT EDIT."
- Same pattern found in multiple generated files under pb/ directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if this is a generated change by looking for similar patterns in other generated files # Look for similar changes in other generated files rg -l 'msgs := make\(\[\]string, 0, len\(m\)\)' 'pb/**/*.pb.validate.go'Length of output: 137
Script:
#!/bin/bash # First, let's verify the repository structure and find our target file echo "=== Checking repository structure ===" fd -e go echo -e "\n=== Checking if the file exists and looking for generation markers ===" cat pb/c1/connector/v2/annotation_entitlement.pb.validate.go echo -e "\n=== Looking for similar patterns in all .go files ===" rg "msgs := make\(\[\]string, 0," -lLength of output: 82236
Script:
#!/bin/bash # Check if file exists and look for generation markers if [ -f "pb/c1/connector/v2/annotation_entitlement.pb.validate.go" ]; then echo "=== File exists, checking content ===" head -n 20 "pb/c1/connector/v2/annotation_entitlement.pb.validate.go" else echo "File does not exist" fi echo -e "\n=== Searching for similar patterns ===" rg -l "msgs := make\(\[\]string, 0," --type goLength of output: 1945
pb/c1/connector/v2/annotation_ratelimit.pb.validate.go (1)
109-109: LGTM! Consistent with other generated files.The slice pre-allocation optimization follows the same pattern seen in other generated files, further suggesting this is a generator-level change.
pb/c1/reader/v2/entitlement.pb.validate.go (1)
88-88: LGTM! Consistent optimization pattern across error types.The slice pre-allocation optimization is consistently applied across multiple error types in the generated code.
Also applies to: 226-226
internal/connector/connector.go (1)
44-44: LGTM! Clean addition of the new service client.The ResourceLookupServiceClient is properly integrated into the connectorClient struct, following the established pattern.
proto/c1/connector/v2/resource.proto (2)
86-95: LGTM! Well-structured request/response messages.The message definitions are clean and focused. The lookup token validation rules are reasonable, and the response structure is appropriately simple.
238-241: LGTM! Well-structured next step options.The oneof usage for next_step is clean and provides good flexibility for different account creation scenarios.
pb/c1/connector/v2/annotation_grant.pb.validate.go (1)
103-103: LGTM! Performance optimization for error handling.The changes consistently optimize error message handling by pre-allocating slice capacity, which reduces memory allocations during error aggregation.
Also applies to: 205-205, 336-336, 436-436, 538-538
pb/c1/c1z/v1/diff.pb.validate.go (1)
175-175: Same optimization pattern as in annotation_grant.pb.validate.goAlso applies to: 377-377, 578-578, 765-765
pb/c1/utls/v1/tls.pb.validate.go (1)
79-79: Same optimization pattern as in annotation_grant.pb.validate.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/connectorbuilder/connectorbuilder.go (1)
941-962: Enhance error handling with more specific error types.While the implementation is solid, consider wrapping the error from
LookupResourcewith additional context to help with debugging:resource, annos, err := b.resourceLookupManager.LookupResource(ctx, request.LookupToken) if err != nil { b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) - return nil, err + return nil, fmt.Errorf("error looking up resource with token %s: %w", request.LookupToken, err) }This change would provide more context in error messages while preserving the original error for error unwrapping.
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (1)
739-740: Consider reducing code duplication in Error() implementations.The same error string concatenation logic is duplicated across multiple error types. Consider refactoring to use a common helper function:
+ // commonErrorString creates a concatenated error string from multiple errors + func commonErrorString(errs []error) string { + msgs := make([]string, 0, len(errs)) + for _, err := range errs { + msgs = append(msgs, err.Error()) + } + return strings.Join(msgs, "; ") + } // Error returns a concatenation of all the error messages it wraps. func (m TaskMultiError) Error() string { - msgs := make([]string, 0, len(m)) - for _, err := range m { - msgs = append(msgs, err.Error()) - } - return strings.Join(msgs, "; ") + return commonErrorString(m) }This would:
- Reduce code duplication
- Make maintenance easier
- Keep the error string format consistent
Also applies to: 1026-1027, 1164-1165, 1277-1278, 1500-1501, 1670-1671, 1840-1841, 2085-2086, 2222-2223, 2476-2477, 2615-2616, 2718-2719, 2822-2823, 2958-2959, 3092-3093, 3226-3227, 3391-3392, 3614-3615, 3777-3778, 3969-3970, 4073-4078, 4204-4205, 4335-4336, 4529-4530, 4723-4724, 4859-4860, 4995-4996, 5131-5132, 5269-5270, 5406-5407, 5597-5598, 5770-5771, 5892-5893, 6054-6055, 6235-6236, 6414-6415
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
pb/c1/connector/v2/resource.pb.validate.go(32 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(37 hunks)pkg/connectorbuilder/connectorbuilder.go(5 hunks)pkg/tasks/c1api/lookup_resource.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)proto/c1/connector/v2/resource.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
🔇 Additional comments (17)
pkg/types/tasks/tasks.go (1)
21-22: LGTM!The new task type
LookupResourceTypeis correctly added with a consistent naming convention and proper placement in the enumeration.Also applies to: 85-85
pkg/tasks/c1api/lookup_resource.go (2)
18-26: LGTM!The interface and struct definitions are clean and follow the established patterns.
51-56: LGTM!The constructor follows the established pattern and correctly initializes the task handler.
pkg/tasks/tasks.go (1)
45-46: LGTM!The new task type is correctly integrated into both
IsandGetTypefunctions, following the established patterns.Also applies to: 90-91
pkg/tasks/c1api/manager.go (1)
246-247: LGTM!The new task type is correctly integrated into the
Processmethod, following the established patterns.pkg/connectorbuilder/connectorbuilder.go (3)
62-64: LGTM! Well-defined interface.The
ResourceLookupManagerinterface is well-designed, following Go conventions and maintaining consistency with other interfaces in the codebase.
96-96: LGTM! Field addition is consistent.The
resourceLookupManagerfield is properly integrated into thebuilderImplstruct, maintaining consistency with other manager fields.
288-288: LGTM! Proper initialization and validation.The initialization and validation of the
resourceLookupManagerfollows the established pattern, with proper error handling for duplicate implementations.Also applies to: 353-359
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (2)
681-721: LGTM! Task_LookupResource validation case is well-implemented.The implementation:
- Correctly handles nil checks
- Properly delegates to the embedded message validation
- Maintains consistency with other task type validations
4035-4064: Consider adding validation rules for LookupToken field.The
Task_LookupResourceTaskvalidation implementation currently has no validation rules for theLookupTokenfield. Consider adding validation constraints such as:
- Minimum/maximum length requirements
- Format validation (e.g., regex pattern for expected token format)
- Non-empty validation
This would help prevent invalid or malicious token values from being processed.
proto/c1/connectorapi/baton/v1/baton.proto (1)
133-133: LGTM!The new task type is correctly integrated into the
task_typeoneof field with an appropriate field number.proto/c1/connector/v2/resource.proto (3)
79-84: LGTM! Well-documented service definition.The service documentation clearly explains its purpose in the context of create account flows and invite acceptance scenarios.
86-97: LGTM! Well-structured messages with appropriate validation.The request and response messages are well-defined with appropriate validation rules for the lookup token.
229-234: Add validation rules for polling parameters.The polling configuration needs validation rules to ensure reasonable values for intervals and prevent excessive token/description lengths.
pb/c1/connector/v2/resource.pb.validate.go (3)
181-181: Improved slice initialization with pre-allocated capacity.The standardization of slice initialization using
make([]string, 0, len(m))across all error types is a good performance optimization that avoids unnecessary slice reallocations.Also applies to: 377-377, 567-567, 721-721, 890-890, 1022-1022, 1187-1187, 1318-1318, 1454-1454, 1648-1648, 1847-1847, 2013-2013, 2241-2241, 2389-2389, 2584-2584, 2841-2841, 2955-2955, 3063-3063, 3242-3242, 3365-3365, 3619-3619, 4013-4013, 4121-4121, 4285-4285, 4400-4400, 4503-4503, 4608-4608, 4934-4934, 5044-5044, 5154-5154, 5260-5260, 5373-5373, 5525-5525, 5661-5661, 5829-5829, 6055-6055, 6162-6162
637-711: Well-structured validation for ResourceLookupService messages.The validation implementation for ResourceLookupService messages follows best practices:
- Appropriate length validation for LookupToken (1-8192 bytes)
- Proper handling of embedded message validation
- Comprehensive validation of optional fields
Also applies to: 788-880
Line range hint
2307-5590: Comprehensive validation for ConnectorAccountCreationSchema.The validation implementation for ConnectorAccountCreationSchema is thorough and well-structured:
- Type-safe validation for all field types (String, Bool, Int, Map)
- Proper handling of oneof fields and embedded messages
- Consistent validation patterns across all schema components
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/tasks/local/resource_lookuper.go (2)
39-53: Consider adding debug logging.While the implementation is correct, consider adding debug logging to improve observability of the resource lookup process.
func (m *localResourceLookuper) Process(ctx context.Context, task *v1.Task, cc types.ConnectorClient) error { + log.Debug().Msg("starting resource lookup process") accountManager := resource_lookup.NewResourceLookupper(cc, m.lookupToken) err := accountManager.Run(ctx) if err != nil { + log.Error().Err(err).Msg("failed to run resource lookup") return err } err = accountManager.Close(ctx) if err != nil { + log.Error().Err(err).Msg("failed to close resource lookup") return err } + log.Debug().Msg("completed resource lookup process") return nil }
55-56: Fix incorrect documentation.The comment refers to "NewGranter" but the function is "NewResourceLookerUpper".
-// NewGranter returns a task manager that queues a sync task. +// NewResourceLookerUpper returns a task manager that queues a resource lookup task. func NewResourceLookerUpper(ctx context.Context, lookupToken string) tasks.Manager {pkg/resource_lookup/resource_lookup.go (2)
34-36: Consider documenting the empty Close method.The
Closemethod appears to be part of an interface implementation but doesn't perform any cleanup. Consider adding a comment explaining why cleanup is unnecessary or implement the required cleanup logic.+// Close implements the closer interface. No cleanup is required for ResourceFinder. func (p *ResourceFinder) Close(ctx context.Context) error { return nil }
18-32: Enhance error handling and logging in Run method.The error handling could be improved by:
- Including more context in the error message
- Adding debug logging before the lookup attempt
- Including the lookup token in the error log (safely)
func (p *ResourceFinder) Run(ctx context.Context) error { l := ctxzap.Extract(ctx) + l.Debug("Attempting resource lookup", zap.String("lookup_token_length", fmt.Sprintf("%d", len(p.lookupToken)))) resource, err := p.connector.LookupResource(ctx, &v2.ResourceLookupServiceLookupResourceRequest{ LookupToken: p.lookupToken, Annotations: nil, }) if err != nil { - l.Error("Failed to lookup resource", zap.Error(err)) + l.Error("Resource lookup failed", + zap.Error(err), + zap.String("lookup_token_length", fmt.Sprintf("%d", len(p.lookupToken))), + ) - return err + return fmt.Errorf("resource lookup failed: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/connector/connector.go(4 hunks)internal/connector/noop_provisioner.go(1 hunks)pkg/cli/commands.go(1 hunks)pkg/connectorrunner/runner.go(5 hunks)pkg/resource_lookup/resource_lookup.go(1 hunks)pkg/tasks/local/resource_lookuper.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/connector/noop_provisioner.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/connector/connector.go
🧰 Additional context used
🪛 GitHub Check: go-test (1.22.x, ubuntu-latest)
pkg/tasks/local/resource_lookuper.go
[failure] 5-5:
"sync" imported and not used
[failure] 8-8:
"google.golang.org/protobuf/types/known/structpb" imported and not used
[failure] 11-11:
"github.com/conductorone/baton-sdk/pkg/provisioner" imported and not used
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o)
🪛 GitHub Check: go-lint
pkg/tasks/local/resource_lookuper.go
[failure] 5-5:
"sync" imported and not used
[failure] 8-8:
"google.golang.org/protobuf/types/known/structpb" imported and not used
[failure] 11-11:
"github.com/conductorone/baton-sdk/pkg/provisioner" imported and not used
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o) (typecheck)
[failure] 5-5:
"sync" imported and not used
[failure] 8-8:
"google.golang.org/protobuf/types/known/structpb" imported and not used
[failure] 11-11:
"github.com/conductorone/baton-sdk/pkg/provisioner" imported and not used
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o)) (typecheck)
🪛 GitHub Actions: ci
pkg/tasks/local/resource_lookuper.go
[error] 5-5: "sync" imported and not used
🔇 Additional comments (8)
pkg/tasks/local/resource_lookuper.go (2)
21-27: LGTM!The helper methods are correctly implemented for local resource lookup functionality.
29-37: LGTM!The method correctly implements one-time task creation with proper thread safety using
sync.Once.🧰 Tools
🪛 GitHub Check: go-test (1.22.x, ubuntu-latest)
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o)🪛 GitHub Check: go-lint
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o) (typecheck)
[failure] 31-31:
m.o undefined (type *localResourceLookuper has no field or method o)) (typecheck)pkg/resource_lookup/resource_lookup.go (1)
20-23: Consider utilizing annotations in the lookup request.The
Annotationsfield is currently set to nil. Consider documenting why annotations are not used or provide a way to pass annotations through the lookup request.pkg/cli/commands.go (1)
150-156: LGTM! Clean integration of lookup resource functionality.The implementation follows the established pattern for command options and maintains consistency with other similar features.
pkg/connectorrunner/runner.go (4)
294-294: LGTM! Clean addition of lookup resource configuration fields.The new fields
lookupResourceEnabledandlookupResourceTokenare well-placed and follow the existing structure.Also applies to: 307-307
431-437: LGTM! Well-structured option function.The
WithOnDemandLookupResourcefunction follows the established pattern and correctly sets up the configuration.
485-491: LGTM! Clean implementation of lookup resource enablement.The
WithLookupResourceEnabledfunction is simple and focused, following the pattern of other similar functions.
617-619: LGTM! Proper integration of resource lookup task manager.The initialization of the resource lookup manager follows the established pattern and is well-integrated into the existing switch logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/connectorrunner/runner.go (2)
431-438: Add validation for the lookup token parameter.While the implementation follows the established Option pattern, consider adding validation to ensure the
lookupTokenparameter is not empty.func WithOnDemandLookupResource(lookupToken string) Option { return func(ctx context.Context, cfg *runnerConfig) error { + if lookupToken == "" { + return errors.New("lookupToken must not be empty") + } cfg.onDemand = true cfg.lookupResourceEnabled = true cfg.lookupResourceToken = lookupToken return nil } }
431-438: Consider rate limiting for resource lookup operations.Since resource lookup is a new API operation, consider whether it should be subject to rate limiting like other operations in the codebase. This would help prevent potential abuse and ensure system stability.
Also applies to: 611-613
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cli/commands.go(1 hunks)pkg/connectorrunner/runner.go(3 hunks)pkg/field/defaults.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/commands.go
🔇 Additional comments (4)
pkg/connectorrunner/runner.go (2)
307-308: LGTM! New configuration fields are well-structured.The new fields
lookupResourceTokenandlookupResourceEnabledfollow the established naming conventions and are appropriately typed for their purpose.
611-613: LGTM! Resource lookup case is well-implemented.The new case for resource lookup follows the established pattern and is correctly placed within the switch statement. The condition checking for a non-empty token provides a good guard.
pkg/field/defaults.go (2)
24-26: LGTM! Field definition follows established patterns.The new
lookupResourceFieldis well-defined and consistent with other field definitions in the file. The description clearly explains its purpose, and the field is correctly marked as hidden and persistent.
24-26: Verify the field usage in resource lookup service.Since this field is part of the new resource lookup service implementation, let's verify its usage across the codebase.
| google.protobuf.Timestamp timeout = 2; | ||
| string polling_token = 3; | ||
| string description = 4; | ||
| } |
There was a problem hiding this comment.
This a hint to tell c1 how often it should call LookupResource, and how long it should do so before giving up, right? If so, shouldn't timeout be a duration so that a connector running on a machine with an incorrect clock doesn't mess this up?
proto/c1/connector/v2/resource.proto
Outdated
|
|
||
| // ResourceLookupService is used in conjunction with create account flows to confirm confirmation or status of created accounts | ||
| // For example, github would send an invite as part of its flow and return "poll for invite acceptance" with the token | ||
| service ResourceLookupService { |
There was a problem hiding this comment.
I don't want to create extra work for you, but since this will only be used as part of account creation, can we call it something like AccountCreationStatusService (and call the RPC GetAccountCreationStatus or something similar)?
I don't want anyone writing connectors to be misled by the name. Based on the name, I originally thought it was for more general functionality like GetResource() would be.
There was a problem hiding this comment.
Definitely, we can do that
proto/c1/connector/v2/resource.proto
Outdated
| } | ||
|
|
||
| message ResourceLookupServiceLookupResourceResponse { | ||
| Resource resource = 1; |
There was a problem hiding this comment.
This response format seems to assume that no other steps will be needed after a PollForConfirmation is returned in CreateAccountResponse. Either a resource will be created or it will be an error.
I think this response should be the same or very similar to CreateAccountResponse. That would let us have multiple steps that need to be polled for in series (eg: account got created, then invite got accepted). It would also allow us to return an ActionRequiredResult for things like an invite expiring, an invite being declined, etc. And it would let us return encrypted credentials once the account is created.
There was a problem hiding this comment.
I think this response should be the same or very similar to CreateAccountResponse
Agreed, this is a great point
|
Just dropping this account provisioning document here for reference |
ggreer
force-pushed
the
logan/resource-lookup-service
branch
from
dc41b43 to
6c6e045
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
internal/connector/connector.go (1)
190-193:⚠️ Potential issueFix inconsistent provisioning behavior for ResourceLookupService.
When provisioning is disabled, the ResourceLookupService is still registered with the main server (cw.server) instead of using the noop handler like other services. This breaks the pattern established by other provisioning-dependent services.
Apply this diff to maintain consistency:
if cw.resourceLookupEnabled { connectorV2.RegisterResourceLookupServiceServer(server, cw.server) + } else { + noop := &noopProvisioner{} + connectorV2.RegisterResourceLookupServiceServer(server, noop) }
🧹 Nitpick comments (5)
pkg/resource_lookup/resource_lookup.go (3)
13-16: Add documentation for the ResourceFinder struct.The struct lacks documentation explaining its purpose and usage.
Add a doc comment explaining that this struct is responsible for looking up resources using an opaque token:
+// ResourceFinder performs resource lookups using an opaque token through a connector client. type ResourceFinder struct { lookupToken string connector types.ConnectorClient }
18-32: Add error wrapping and improve error handling in Run method.The error handling could be improved to provide more context.
Apply this diff to wrap errors with additional context:
func (p *ResourceFinder) Run(ctx context.Context) error { l := ctxzap.Extract(ctx) resource, err := p.connector.LookupResource(ctx, &v2.ResourceLookupServiceLookupResourceRequest{ LookupToken: p.lookupToken, Annotations: nil, }) if err != nil { l.Error("Failed to lookup resource", zap.Error(err)) - return err + return fmt.Errorf("failed to lookup resource: %w", err) } l.Debug("Resource lookup result", zap.Any("resource", resource), zap.Any("annotations", resource.GetAnnotations())) return nil }
34-36: Document the purpose of the empty Close method.The Close method implementation is empty without explanation.
Add a doc comment explaining why this method exists and returns nil:
+// Close implements io.Closer but has no cleanup requirements. func (p *ResourceFinder) Close(ctx context.Context) error { return nil }pkg/connectorbuilder/connectorbuilder.go (2)
65-67: Add documentation for ResourceLookupManager interface.The interface lacks documentation explaining its purpose and contract.
Add comprehensive documentation:
+// ResourceLookupManager defines the contract for looking up resources using opaque tokens. +// Implementations should handle the translation of lookup tokens to actual resources +// and provide appropriate annotations for tracking the lookup process. type ResourceLookupManager interface { LookupResource(ctx context.Context, lookupToken string) (*v2.Resource, annotations.Annotations, error) }
1004-1025: Improve error handling in LookupResource method.The error handling could be more descriptive and include tracing.
Apply this diff to improve error handling and add tracing:
func (b *builderImpl) LookupResource(ctx context.Context, request *v2.ResourceLookupServiceLookupResourceRequest) (*v2.ResourceLookupServiceLookupResourceResponse, error) { + ctx, span := tracer.Start(ctx, "builderImpl.LookupResource") + defer span.End() + start := b.nowFunc() tt := tasks.LookupResourceType l := ctxzap.Extract(ctx) if b.resourceLookupManager == nil { l.Error("error: connector does not have resource lookup manager configured") b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) return nil, status.Error(codes.Unimplemented, "connector does not have resource lookup manager configured") } + if request.LookupToken == "" { + l.Error("error: lookup token is empty") + b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) + return nil, status.Error(codes.InvalidArgument, "lookup token cannot be empty") + } + resource, annos, err := b.resourceLookupManager.LookupResource(ctx, request.LookupToken) if err != nil { + l.Error("error: resource lookup failed", zap.Error(err)) b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) - return nil, err + return nil, fmt.Errorf("resource lookup failed: %w", err) } b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) return &v2.ResourceLookupServiceLookupResourceResponse{ Resource: resource, Annotations: annos, }, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
internal/connector/connector.go(4 hunks)internal/connector/noop_provisioner.go(1 hunks)pb/c1/connector/v2/connector.pb.validate.go(0 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/cli/commands.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(5 hunks)pkg/connectorrunner/runner.go(4 hunks)pkg/field/defaults.go(2 hunks)pkg/resource_lookup/resource_lookup.go(1 hunks)pkg/sync/syncer_test.go(2 hunks)pkg/tasks/c1api/lookup_resource.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/local/resource_lookuper.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/connector.proto(0 hunks)proto/c1/connector/v2/resource.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
💤 Files with no reviewable changes (2)
- proto/c1/connector/v2/connector.proto
- pb/c1/connector/v2/connector.pb.validate.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/field/defaults.go
- pkg/sync/syncer_test.go
- internal/connector/noop_provisioner.go
- pkg/types/types.go
- pkg/types/tasks/tasks.go
- pkg/tasks/tasks.go
- pkg/tasks/c1api/manager.go
- pkg/cli/commands.go
- proto/c1/connectorapi/baton/v1/baton.proto
- pkg/tasks/local/resource_lookuper.go
- pkg/tasks/c1api/lookup_resource.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/resource_lookup/resource_lookup.go
[error] 1-1: var-naming: don't use an underscore in package name
(revive)
🪛 GitHub Check: go-lint
pkg/resource_lookup/resource_lookup.go
[failure] 1-1:
var-naming: don't use an underscore in package name (revive)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (4)
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (2)
681-721: LGTM! The new task type validation case is properly integrated.The validation case for
Task_LookupResourcefollows the established patterns and is correctly integrated into the task type switch statement.
4035-4137: LGTM! The validation implementation for LookupResourceTask is correctly structured.The validation implementation for
Task_LookupResourceTaskfollows the standard validation patterns:
- Proper error types and interfaces
- Consistent error message formatting
- No validation rules for the
LookupTokenfield, which is appropriate as it's an opaque tokenproto/c1/connector/v2/resource.proto (2)
79-84: Rename service to be more specific to account creation.Based on previous feedback, the service name should be more specific to its use case to avoid confusion. Consider renaming to
AccountCreationStatusServicewith a method likeGetAccountCreationStatus.
183-183: Add validation rules for the order field.The
orderfield should have validation rules to prevent negative values and set a reasonable maximum.
| @@ -0,0 +1,43 @@ | |||
| package resource_lookup | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename package to follow Go naming conventions.
The package name contains an underscore which violates Go naming conventions.
Rename the package from resource_lookup to resourcelookup.
🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 1-1: var-naming: don't use an underscore in package name
(revive)
🪛 GitHub Check: go-lint
[failure] 1-1:
var-naming: don't use an underscore in package name (revive)
| func NewResourceLookupper(c types.ConnectorClient, lookupToken string) *ResourceFinder { | ||
| return &ResourceFinder{ | ||
| connector: c, | ||
| lookupToken: lookupToken, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to NewResourceLookupper.
The constructor should validate its inputs.
Add validation for the connector client and lookup token:
func NewResourceLookupper(c types.ConnectorClient, lookupToken string) *ResourceFinder {
+ if c == nil {
+ panic("connector client cannot be nil")
+ }
+ if lookupToken == "" {
+ panic("lookup token cannot be empty")
+ }
return &ResourceFinder{
connector: c,
lookupToken: lookupToken,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewResourceLookupper(c types.ConnectorClient, lookupToken string) *ResourceFinder { | |
| return &ResourceFinder{ | |
| connector: c, | |
| lookupToken: lookupToken, | |
| } | |
| } | |
| func NewResourceLookupper(c types.ConnectorClient, lookupToken string) *ResourceFinder { | |
| if c == nil { | |
| panic("connector client cannot be nil") | |
| } | |
| if lookupToken == "" { | |
| panic("lookup token cannot be empty") | |
| } | |
| return &ResourceFinder{ | |
| connector: c, | |
| lookupToken: lookupToken, | |
| } | |
| } |
pkg/connectorrunner/runner.go
Outdated
| func WithOnDemandLookupResource(lookupToken string) Option { | ||
| return func(ctx context.Context, cfg *runnerConfig) error { | ||
| cfg.onDemand = true | ||
| cfg.lookupResourceToken = lookupToken | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in WithOnDemandLookupResource option.
The option function should validate the lookup token to prevent issues downstream.
Apply this diff to add validation:
func WithOnDemandLookupResource(lookupToken string) Option {
return func(ctx context.Context, cfg *runnerConfig) error {
+ if lookupToken == "" {
+ return fmt.Errorf("lookup token cannot be empty")
+ }
cfg.onDemand = true
cfg.lookupResourceToken = lookupToken
return nil
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithOnDemandLookupResource(lookupToken string) Option { | |
| return func(ctx context.Context, cfg *runnerConfig) error { | |
| cfg.onDemand = true | |
| cfg.lookupResourceToken = lookupToken | |
| return nil | |
| } | |
| } | |
| func WithOnDemandLookupResource(lookupToken string) Option { | |
| return func(ctx context.Context, cfg *runnerConfig) error { | |
| if lookupToken == "" { | |
| return fmt.Errorf("lookup token cannot be empty") | |
| } | |
| cfg.onDemand = true | |
| cfg.lookupResourceToken = lookupToken | |
| return nil | |
| } | |
| } |
| message PollForConfirmation { | ||
| google.protobuf.Duration polling_interval = 1; | ||
| google.protobuf.Timestamp timeout = 2; | ||
| string polling_token = 3; | ||
| string description = 4; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Duration for timeout and add validation rules.
- The
timeoutfield should be aDurationinstead ofTimestampto avoid issues with incorrect machine clocks. - Add validation rules for polling parameters to ensure reasonable values.
Apply this diff:
message PollForConfirmation {
- google.protobuf.Duration polling_interval = 1;
+ google.protobuf.Duration polling_interval = 1 [(validate.rules).duration = {
+ gte: {seconds: 1}
+ lte: {seconds: 3600} // Max 1 hour
+ }];
- google.protobuf.Timestamp timeout = 2;
+ google.protobuf.Duration timeout = 2 [(validate.rules).duration = {
+ gt: {seconds: 0}
+ lte: {seconds: 86400} // Max 24 hours
+ }];
- string polling_token = 3;
+ string polling_token = 3 [(validate.rules).string = {
+ min_bytes: 1
+ max_bytes: 8192
+ }];
- string description = 4;
+ string description = 4 [(validate.rules).string = {
+ max_bytes: 4096
+ ignore_empty: true
+ }];
}📝 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.
| message PollForConfirmation { | |
| google.protobuf.Duration polling_interval = 1; | |
| google.protobuf.Timestamp timeout = 2; | |
| string polling_token = 3; | |
| string description = 4; | |
| } | |
| message PollForConfirmation { | |
| google.protobuf.Duration polling_interval = 1 [(validate.rules).duration = { | |
| gte: {seconds: 1} | |
| lte: {seconds: 3600} // Max 1 hour | |
| }]; | |
| google.protobuf.Duration timeout = 2 [(validate.rules).duration = { | |
| gt: {seconds: 0} | |
| lte: {seconds: 86400} // Max 24 hours | |
| }]; | |
| string polling_token = 3 [(validate.rules).string = { | |
| min_bytes: 1 | |
| max_bytes: 8192 | |
| }]; | |
| string description = 4 [(validate.rules).string = { | |
| max_bytes: 4096 | |
| ignore_empty: true | |
| }]; | |
| } |
proto/c1/connector/v2/resource.proto
Outdated
| message ResourceLookupServiceLookupResourceResponse { | ||
| Resource resource = 1; | ||
| repeated google.protobuf.Any annotations = 2; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ResourceLookupServiceLookupResourceResponse to match CreateAccountResponse.
The response format should be similar to CreateAccountResponse to support multiple polling steps and handle various scenarios like invite expiration, declined invites, etc.
Apply this diff:
message ResourceLookupServiceLookupResourceResponse {
- Resource resource = 1;
+ oneof result {
+ CreateAccountResponse.SuccessResult success = 1;
+ CreateAccountResponse.ActionRequiredResult action_required = 2;
+ }
repeated google.protobuf.Any annotations = 2;
+ repeated EncryptedData encrypted_data = 3;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/connector/connector.go (1)
190-192:⚠️ Potential issueFix inconsistent service registration pattern.
The service registration for
getAccountCreationStatusEnabledshould follow the same pattern as other features by using the noop handler when disabled.if cw.getAccountCreationStatusEnabled { connectorV2.RegisterResourceManagerServiceServer(server, cw.server) + } else { + noop := &noopProvisioner{} + connectorV2.RegisterResourceManagerServiceServer(server, noop) }
🧹 Nitpick comments (7)
pkg/tasks/local/account_creation_status.go (3)
27-35: Add context cancellation handling.While the implementation is clean, it should handle context cancellation to ensure proper cleanup.
func (m *accountCreationStatuser) Next(ctx context.Context) (*v1.Task, time.Duration, error) { + if ctx.Err() != nil { + return nil, 0, ctx.Err() + } var task *v1.Task m.o.Do(func() { task = &v1.Task{ TaskType: &v1.Task_GetAccountCreationStatus{}, } }) return task, 0, nil }
37-51: Enhance error handling and add logging.The error handling could be improved with better error wrapping and logging.
+import ( + "fmt" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" +) func (m *accountCreationStatuser) Process(ctx context.Context, task *v1.Task, cc types.ConnectorClient) error { + if ctx.Err() != nil { + return ctx.Err() + } + + l := ctxzap.Extract(ctx) accountManager := provisioner.NewAccountCreationStatuser(cc, m.taskId) err := accountManager.Run(ctx) if err != nil { + l.Error("failed to run account manager", zap.Error(err)) - return err + return fmt.Errorf("failed to run account manager: %w", err) } err = accountManager.Close(ctx) if err != nil { + l.Error("failed to close account manager", zap.Error(err)) - return err + return fmt.Errorf("failed to close account manager: %w", err) } return nil }
53-57: Add taskId validation.The constructor should validate that taskId is not empty.
func NewAccountCreationStatuser(ctx context.Context, taskId string) tasks.Manager { + if taskId == "" { + return nil + } return &accountCreationStatuser{ taskId: taskId, } }pkg/tasks/c1api/account_creation_status.go (2)
28-49: Add context cancellation handling.While the error handling and logging are good, the method should handle context cancellation.
func (g *AccountCreationStatusTaskHandler) HandleTask(ctx context.Context) error { + if ctx.Err() != nil { + return g.helpers.FinishTask(ctx, nil, nil, errors.Join(ctx.Err(), ErrTaskNonRetryable)) + } + l := ctxzap.Extract(ctx).With(zap.String("task_id", g.task.Id), zap.Stringer("task_type", tasks.GetType(g.task))) t := g.task.GetGetAccountCreationStatus() if t == nil || t.GetTaskId() == "" { l.Error( "task ID was nil or empty", ) return g.helpers.FinishTask(ctx, nil, nil, errors.Join(errors.New("malformed get account creation status task"), ErrTaskNonRetryable)) } cc := g.helpers.ConnectorClient() resp, err := cc.GetAccountCreationStatus(ctx, &v2.GetAccountCreationStatusRequest{ TaskId: t.GetTaskId(), }) if err != nil { l.Error("failed looking up resource", zap.Error(err)) return g.helpers.FinishTask(ctx, nil, nil, errors.Join(err, ErrTaskNonRetryable)) } return g.helpers.FinishTask(ctx, resp, resp.GetAnnotations(), nil) }
51-56: Add input validation.The constructor should validate that task and helpers are not nil.
func newAccountCreationStatusTaskHandler(task *v1.Task, helpers accountCreationStatusHelpers) tasks.TaskHandler { + if task == nil || helpers == nil { + return nil + } return &AccountCreationStatusTaskHandler{ task: task, helpers: helpers, } }pkg/provisioner/provisioner.go (2)
285-303: Enhance error handling and logging.While the implementation is good, it could be improved with better context handling and more informative logging.
func (p *Provisioner) getAccountCreationStatus(ctx context.Context) error { ctx, span := tracer.Start(ctx, "Provisioner.getAccountCreationStatus") defer span.End() + if ctx.Err() != nil { + return ctx.Err() + } + l := ctxzap.Extract(ctx) resp, err := p.connector.GetAccountCreationStatus(ctx, &v2.GetAccountCreationStatusRequest{ TaskId: p.createAccountTaskId, Annotations: nil, }) if err != nil { - l.Error("Failed to find create account task", zap.Error(err)) + l.Error("failed to get account creation status", + zap.String("task_id", p.createAccountTaskId), + zap.Error(err)) return err } - l.Debug("Create account status result", zap.Any("resource", resp), zap.Any("annotations", resp.GetAnnotations())) + l.Debug("account creation status retrieved", + zap.String("task_id", p.createAccountTaskId), + zap.Any("status", resp.GetStatus()), + zap.Any("resource", resp.GetResource()), + zap.Any("annotations", resp.GetAnnotations())) return nil }
395-400: Add input validation.The constructor should validate its inputs.
func NewAccountCreationStatuser(c types.ConnectorClient, taskId string) *Provisioner { + if c == nil { + return nil + } + if taskId == "" { + return nil + } return &Provisioner{ connector: c, createAccountTaskId: taskId, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
internal/connector/connector.go(4 hunks)pb/c1/connector/v2/resource.pb.validate.go(5 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/cli/commands.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(5 hunks)pkg/connectorrunner/runner.go(4 hunks)pkg/field/defaults.go(2 hunks)pkg/provisioner/provisioner.go(4 hunks)pkg/sync/syncer_test.go(2 hunks)pkg/tasks/c1api/account_creation_status.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/local/account_creation_status.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)proto/c1/connector/v2/resource.proto(4 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/types/tasks/tasks.go
- pkg/sync/syncer_test.go
- pkg/types/types.go
- pkg/tasks/tasks.go
- proto/c1/connectorapi/baton/v1/baton.proto
- pkg/cli/commands.go
- pkg/field/defaults.go
- pkg/tasks/c1api/manager.go
🧰 Additional context used
🪛 Buf (1.47.2)
proto/c1/connector/v2/resource.proto
8-8: Import "google/protobuf/timestamp.proto" is unused.
(IMPORT_USED)
82-82: "c1.connector.v2.CreateAccountResponse" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
82-82: RPC response type "CreateAccountResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/resource.proto
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
[failure] 82-82:
"c1.connector.v2.CreateAccountResponse" is used as the request or response type for multiple RPCs.
[failure] 82-82:
RPC response type "CreateAccountResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
🔇 Additional comments (30)
pb/c1/connector/v2/resource.pb.validate.go (13)
637-650: Auto-generated validation code looks correct.
652-709: Auto-generated validation code looks correct.
711-783: Auto-generated validation code looks correct.
2135-2282: Auto-generated validation code looks correct.
4502-4828: Auto-generated validation code looks correct.
4829-4938: Auto-generated validation code looks correct.
4939-5047: Auto-generated validation code looks correct.
5049-5156: Auto-generated validation code looks correct.
5158-5266: Auto-generated validation code looks correct.
5268-5419: Auto-generated validation code looks correct.
5556-5571: Auto-generated validation code looks correct.
5573-5723: Auto-generated validation code looks correct.
5781-5866: Auto-generated validation code looks correct.pkg/tasks/local/account_creation_status.go (1)
14-25: LGTM! Clean and focused implementation.The struct is minimal with clear methods. Good use of
sync.Oncefor ensuring single task creation.pkg/tasks/c1api/account_creation_status.go (1)
18-21: LGTM! Well-defined interface.The interface is minimal and focused with clear method signatures.
pkg/provisioner/provisioner.go (2)
40-40: LGTM! Clean field addition.The new field is well-placed and appropriately named.
88-89: LGTM! Consistent implementation.The new case follows the existing pattern and is well-integrated.
internal/connector/connector.go (2)
46-46: LGTM! Consistent field additions.The new fields follow the existing pattern and are well-integrated.
Also applies to: 64-64
103-108: LGTM! Clean option implementation.The option function follows the existing pattern and is well-implemented.
pkg/connectorrunner/runner.go (3)
307-307: LGTM!The field name follows Go naming conventions and is appropriately placed in the config struct.
574-576: LGTM!The code follows the established pattern for enabling capabilities in wrapper options.
613-615: LGTM!The initialization follows the established pattern for task managers and is correctly placed in the switch block.
pkg/connectorbuilder/connectorbuilder.go (3)
65-67: LGTM!The interface is well-defined with a clear method signature that follows Go conventions.
99-99: LGTM!The field follows Go naming conventions and is appropriately placed in the struct.
374-379: LGTM!The initialization follows the established pattern and includes proper duplicate checking.
pb/c1/connectorapi/baton/v1/baton.pb.validate.go (1)
681-721: LGTM!The integration of the GetAccountCreationStatus task type validation follows the established pattern and includes proper error handling and validation delegation.
proto/c1/connector/v2/resource.proto (4)
79-83: LGTM! Well-documented service definition.The service is well-documented and reuses the existing
CreateAccountResponsetype, which is appropriate as it contains all the necessary fields for both initial creation and status checks.🧰 Tools
🪛 Buf (1.47.2)
82-82: "c1.connector.v2.CreateAccountResponse" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
82-82: RPC response type "CreateAccountResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 82-82:
"c1.connector.v2.CreateAccountResponse" is used as the request or response type for multiple RPCs.
[failure] 82-82:
RPC response type "CreateAccountResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
85-91: LGTM! Clear and focused message definition.The message has appropriate validation rules and a focused structure.
169-208: Add validation rules for the order field.The
orderfield in theFieldmessage should have validation rules to prevent negative values and set a reasonable maximum.Apply this diff:
string description = 3; string placeholder = 4; - int32 order = 5; + int32 order = 5 [(validate.rules).int32 = { + gte: 0 + lte: 1000 + }];
223-228: Add validation rules for polling parameters.The polling parameters should have validation rules to ensure reasonable values.
Apply this diff:
message PollForConfirmation { - google.protobuf.Duration polling_interval = 1; + google.protobuf.Duration polling_interval = 1 [(validate.rules).duration = { + gte: {seconds: 1} + lte: {seconds: 3600} // Max 1 hour + }]; - google.protobuf.Duration timeout = 2; + google.protobuf.Duration timeout = 2 [(validate.rules).duration = { + gt: {seconds: 0} + lte: {seconds: 86400} // Max 24 hours + }]; - string task_id = 3; + string task_id = 3 [(validate.rules).string = { + min_bytes: 1 + max_bytes: 8192 + }]; - string description = 4; + string description = 4 [(validate.rules).string = { + max_bytes: 4096 + ignore_empty: true + }]; }
| func WithOnDemandGetAccountCreationStatus(taskId string) Option { | ||
| return func(ctx context.Context, cfg *runnerConfig) error { | ||
| cfg.onDemand = true | ||
| cfg.accountCreationTaskId = taskId | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for taskId parameter.
The function should validate that the taskId is not empty to prevent issues downstream.
Apply this diff to add validation:
func WithOnDemandGetAccountCreationStatus(taskId string) Option {
return func(ctx context.Context, cfg *runnerConfig) error {
+ if taskId == "" {
+ return fmt.Errorf("taskId cannot be empty")
+ }
cfg.onDemand = true
cfg.accountCreationTaskId = taskId
return nil
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WithOnDemandGetAccountCreationStatus(taskId string) Option { | |
| return func(ctx context.Context, cfg *runnerConfig) error { | |
| cfg.onDemand = true | |
| cfg.accountCreationTaskId = taskId | |
| return nil | |
| } | |
| } | |
| func WithOnDemandGetAccountCreationStatus(taskId string) Option { | |
| return func(ctx context.Context, cfg *runnerConfig) error { | |
| if taskId == "" { | |
| return fmt.Errorf("taskId cannot be empty") | |
| } | |
| cfg.onDemand = true | |
| cfg.accountCreationTaskId = taskId | |
| return nil | |
| } | |
| } |
| func (b *builderImpl) GetAccountCreationStatus(ctx context.Context, request *v2.GetAccountCreationStatusRequest) (*v2.CreateAccountResponse, error) { | ||
| start := b.nowFunc() | ||
| tt := tasks.GetAccountCreationStatusType | ||
| l := ctxzap.Extract(ctx) | ||
| if b.accountCreationStatusManager == nil { | ||
| l.Error("error: connector does not have an account creation status manager configured") | ||
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | ||
| return nil, status.Error(codes.Unimplemented, "connector does not have an account creation status manager configured") | ||
| } | ||
|
|
||
| resource, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId) | ||
| if err != nil { | ||
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | ||
| return nil, err | ||
| } | ||
|
|
||
| b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) | ||
| return &v2.CreateAccountResponse{ | ||
| Result: &v2.CreateAccountResponse_Success{ | ||
| Success: &v2.CreateAccountResponse_SuccessResult{ | ||
| Resource: resource, | ||
| }, | ||
| }, | ||
| Annotations: annos, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add resource existence validation.
The method should validate that the returned resource exists before constructing the success response.
Apply this diff to add validation:
func (b *builderImpl) GetAccountCreationStatus(ctx context.Context, request *v2.GetAccountCreationStatusRequest) (*v2.CreateAccountResponse, error) {
start := b.nowFunc()
tt := tasks.GetAccountCreationStatusType
l := ctxzap.Extract(ctx)
if b.accountCreationStatusManager == nil {
l.Error("error: connector does not have an account creation status manager configured")
b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start))
return nil, status.Error(codes.Unimplemented, "connector does not have an account creation status manager configured")
}
resource, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId)
if err != nil {
b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start))
return nil, err
}
+ if resource == nil {
+ b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start))
+ return nil, status.Error(codes.NotFound, "resource not found")
+ }
+
b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start))
return &v2.CreateAccountResponse{
Result: &v2.CreateAccountResponse_Success{
Success: &v2.CreateAccountResponse_SuccessResult{
Resource: resource,
},
},
Annotations: annos,
}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *builderImpl) GetAccountCreationStatus(ctx context.Context, request *v2.GetAccountCreationStatusRequest) (*v2.CreateAccountResponse, error) { | |
| start := b.nowFunc() | |
| tt := tasks.GetAccountCreationStatusType | |
| l := ctxzap.Extract(ctx) | |
| if b.accountCreationStatusManager == nil { | |
| l.Error("error: connector does not have an account creation status manager configured") | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.Unimplemented, "connector does not have an account creation status manager configured") | |
| } | |
| resource, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId) | |
| if err != nil { | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, err | |
| } | |
| b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) | |
| return &v2.CreateAccountResponse{ | |
| Result: &v2.CreateAccountResponse_Success{ | |
| Success: &v2.CreateAccountResponse_SuccessResult{ | |
| Resource: resource, | |
| }, | |
| }, | |
| Annotations: annos, | |
| }, nil | |
| } | |
| func (b *builderImpl) GetAccountCreationStatus(ctx context.Context, request *v2.GetAccountCreationStatusRequest) (*v2.CreateAccountResponse, error) { | |
| start := b.nowFunc() | |
| tt := tasks.GetAccountCreationStatusType | |
| l := ctxzap.Extract(ctx) | |
| if b.accountCreationStatusManager == nil { | |
| l.Error("error: connector does not have an account creation status manager configured") | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.Unimplemented, "connector does not have an account creation status manager configured") | |
| } | |
| resource, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId) | |
| if err != nil { | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, err | |
| } | |
| if resource == nil { | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.NotFound, "resource not found") | |
| } | |
| b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) | |
| return &v2.CreateAccountResponse{ | |
| Result: &v2.CreateAccountResponse_Success{ | |
| Success: &v2.CreateAccountResponse_SuccessResult{ | |
| Resource: resource, | |
| }, | |
| }, | |
| Annotations: annos, | |
| }, nil | |
| } |
| // Validate checks the field values on Task_AccountCreationStatusTask with the | ||
| // rules defined in the proto definition for this message. If any rules are | ||
| // violated, the first error encountered is returned, or nil if there are no violations. | ||
| func (m *Task_AccountCreationStatusTask) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on Task_AccountCreationStatusTask with | ||
| // the rules defined in the proto definition for this message. If any rules | ||
| // are violated, the result is a list of violation errors wrapped in | ||
| // Task_AccountCreationStatusTaskMultiError, or nil if none found. | ||
| func (m *Task_AccountCreationStatusTask) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *Task_AccountCreationStatusTask) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| // no validation rules for TaskId | ||
|
|
||
| if len(errors) > 0 { | ||
| return Task_AccountCreationStatusTaskMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Task_AccountCreationStatusTaskMultiError is an error wrapping multiple | ||
| // validation errors returned by Task_AccountCreationStatusTask.ValidateAll() | ||
| // if the designated constraints aren't met. | ||
| type Task_AccountCreationStatusTaskMultiError []error | ||
|
|
||
| // Error returns a concatenation of all the error messages it wraps. | ||
| func (m Task_AccountCreationStatusTaskMultiError) Error() string { | ||
| msgs := make([]string, 0, len(m)) | ||
| for _, err := range m { | ||
| msgs = append(msgs, err.Error()) | ||
| } | ||
| return strings.Join(msgs, "; ") | ||
| } | ||
|
|
||
| // AllErrors returns a list of validation violation errors. | ||
| func (m Task_AccountCreationStatusTaskMultiError) AllErrors() []error { return m } | ||
|
|
||
| // Task_AccountCreationStatusTaskValidationError is the validation error | ||
| // returned by Task_AccountCreationStatusTask.Validate if the designated | ||
| // constraints aren't met. | ||
| type Task_AccountCreationStatusTaskValidationError struct { | ||
| field string | ||
| reason string | ||
| cause error | ||
| key bool | ||
| } | ||
|
|
||
| // Field function returns field value. | ||
| func (e Task_AccountCreationStatusTaskValidationError) Field() string { return e.field } | ||
|
|
||
| // Reason function returns reason value. | ||
| func (e Task_AccountCreationStatusTaskValidationError) Reason() string { return e.reason } | ||
|
|
||
| // Cause function returns cause value. | ||
| func (e Task_AccountCreationStatusTaskValidationError) Cause() error { return e.cause } | ||
|
|
||
| // Key function returns key value. | ||
| func (e Task_AccountCreationStatusTaskValidationError) Key() bool { return e.key } | ||
|
|
||
| // ErrorName returns error name. | ||
| func (e Task_AccountCreationStatusTaskValidationError) ErrorName() string { | ||
| return "Task_AccountCreationStatusTaskValidationError" | ||
| } | ||
|
|
||
| // Error satisfies the builtin error interface | ||
| func (e Task_AccountCreationStatusTaskValidationError) Error() string { | ||
| cause := "" | ||
| if e.cause != nil { | ||
| cause = fmt.Sprintf(" | caused by: %v", e.cause) | ||
| } | ||
|
|
||
| key := "" | ||
| if e.key { | ||
| key = "key for " | ||
| } | ||
|
|
||
| return fmt.Sprintf( | ||
| "invalid %sTask_AccountCreationStatusTask.%s: %s%s", | ||
| key, | ||
| e.field, | ||
| e.reason, | ||
| cause) | ||
| } | ||
|
|
||
| var _ error = Task_AccountCreationStatusTaskValidationError{} | ||
|
|
||
| var _ interface { | ||
| Field() string | ||
| Reason() string | ||
| Key() bool | ||
| Cause() error | ||
| ErrorName() string | ||
| } = Task_AccountCreationStatusTaskValidationError{} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if TaskId validation is needed for AccountCreationStatusTask.
The TaskId field has no validation rules defined, unlike similar fields in other task types that often have regex pattern validation. Consider if validation rules should be added for consistency.
Run the following script to check TaskId validation patterns in other task types:
🏁 Script executed:
#!/bin/bash
# Description: Check TaskId validation patterns in other task types
# Look for TaskId regex patterns in the file
rg -A 2 '_TaskId_Pattern = regexp.MustCompile'Length of output: 1304
Missing TaskId Regex Validation for Consistency
The gathered output reveals that other task validations enforce a regex pattern (e.g., "^[a-zA-Z0-9]{27}$") on their TaskId fields, whereas the new Task_AccountCreationStatusTask lacks any such check. Please verify if this omission is intentional or if the TaskId should be validated similarly for consistency.
- Other tasks (e.g., BatonServiceHelloResponse, BatonServiceHeartbeatResponse, BatonServiceFinishTaskRequest) include regex validation for TaskId.
- Consider adding a regex validation (such as
"^[a-zA-Z0-9]{27}$") forTask_AccountCreationStatusTaskif the consistency requirement holds.
proto/c1/connector/v2/resource.proto
Outdated
| import "google/protobuf/duration.proto"; | ||
| import "google/protobuf/timestamp.proto"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The google/protobuf/timestamp.proto import is not used in this file.
Apply this diff:
import "google/protobuf/duration.proto";
-import "google/protobuf/timestamp.proto";📝 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.
| import "google/protobuf/duration.proto"; | |
| import "google/protobuf/timestamp.proto"; | |
| import "google/protobuf/duration.proto"; |
🧰 Tools
🪛 Buf (1.47.2)
8-8: Import "google/protobuf/timestamp.proto" is unused.
(IMPORT_USED)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 8-8:
Import "google/protobuf/timestamp.proto" is unused.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pkg/connectorbuilder/connectorbuilder.go(6 hunks)proto/c1/connector/v2/resource.proto(3 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
proto/c1/connector/v2/resource.proto
81-81: RPC response type "AccountCreationStatusResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/resource.proto
[failure] 81-81:
RPC response type "AccountCreationStatusResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (7)
pkg/connectorbuilder/connectorbuilder.go (3)
65-67: LGTM!The new interface and struct field are well-defined and properly integrated into the existing codebase.
Also applies to: 99-99
374-379: LGTM!The initialization logic follows the established pattern for manager initialization, including proper duplicate checking.
1004-1029: Add resource existence validation.The method should validate that the returned resource exists before constructing the success response.
Apply this diff to add validation:
func (b *builderImpl) GetAccountCreationStatus(ctx context.Context, request *v2.GetAccountCreationStatusRequest) (*v2.AccountCreationStatusResponse, error) { start := b.nowFunc() tt := tasks.GetAccountCreationStatusType l := ctxzap.Extract(ctx) if b.accountCreationStatusManager == nil { l.Error("error: connector does not have an account creation status manager configured") b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) return nil, status.Error(codes.Unimplemented, "connector does not have an account creation status manager configured") } resource, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId) if err != nil { b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) return nil, err } + if resource == nil { + b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) + return nil, status.Error(codes.NotFound, "resource not found") + } + b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) return &v2.AccountCreationStatusResponse{ Result: &v2.AccountCreationStatusResponse_Success{ Success: &v2.CreateAccountSuccessResult{ Resource: resource, }, }, Annotations: annos, }, nil }proto/c1/connector/v2/resource.proto (4)
168-207: Add validation rules for the order field.The
orderfield in theFieldmessage should have validation rules to prevent negative values and set a reasonable maximum.Apply this diff:
message ConnectorAccountCreationSchema { map<string, Field> field_map = 1; message Field { string display_name = 1; bool required = 2; string description = 3; string placeholder = 4; - int32 order = 5; + int32 order = 5 [(validate.rules).int32 = { + gte: 0 + lte: 1000 + }];
221-226: Add validation rules for polling parameters.Add validation rules for polling parameters to ensure reasonable values.
Apply this diff:
message PollForConfirmation { - google.protobuf.Duration polling_interval = 1; + google.protobuf.Duration polling_interval = 1 [(validate.rules).duration = { + gte: {seconds: 1} + lte: {seconds: 3600} // Max 1 hour + }]; - google.protobuf.Duration timeout = 2; + google.protobuf.Duration timeout = 2 [(validate.rules).duration = { + gt: {seconds: 0} + lte: {seconds: 86400} // Max 24 hours + }]; - string task_id = 3; + string task_id = 3 [(validate.rules).string = { + min_bytes: 1 + max_bytes: 8192 + }]; - string description = 4; + string description = 4 [(validate.rules).string = { + max_bytes: 4096 + ignore_empty: true + }]; }
238-245: LGTM!The response message structure properly matches
CreateAccountResponse, allowing for consistent handling of account creation status responses.
247-254: LGTM!The changes to
CreateAccountResponsemaintain consistency withAccountCreationStatusResponsewhile preserving backward compatibility.
| // AccountCreationStatusService is used in conjunction with create account flows to confirm confirmation or status of created accounts | ||
| // For example, github would send an invite as part of its flow and return "poll for invite acceptance" with a task id | ||
| service AccountCreationStatusService { | ||
| rpc GetAccountCreationStatus(GetAccountCreationStatusRequest) returns (AccountCreationStatusResponse); | ||
| } | ||
|
|
||
| message GetAccountCreationStatusRequest { | ||
| string task_id = 1 [(validate.rules).string = { | ||
| min_bytes: 1 | ||
| max_bytes: 8192 | ||
| }]; | ||
| repeated google.protobuf.Any annotations = 2; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename response type to follow standard naming convention.
The response type should be named GetAccountCreationStatusResponse or AccountCreationStatusServiceGetAccountCreationStatusResponse to follow the standard naming convention.
Apply this diff:
-service AccountCreationStatusService {
- rpc GetAccountCreationStatus(GetAccountCreationStatusRequest) returns (AccountCreationStatusResponse);
+service AccountCreationStatusService {
+ rpc GetAccountCreationStatus(GetAccountCreationStatusRequest) returns (GetAccountCreationStatusResponse);
}📝 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.
| // AccountCreationStatusService is used in conjunction with create account flows to confirm confirmation or status of created accounts | |
| // For example, github would send an invite as part of its flow and return "poll for invite acceptance" with a task id | |
| service AccountCreationStatusService { | |
| rpc GetAccountCreationStatus(GetAccountCreationStatusRequest) returns (AccountCreationStatusResponse); | |
| } | |
| message GetAccountCreationStatusRequest { | |
| string task_id = 1 [(validate.rules).string = { | |
| min_bytes: 1 | |
| max_bytes: 8192 | |
| }]; | |
| repeated google.protobuf.Any annotations = 2; | |
| } | |
| // AccountCreationStatusService is used in conjunction with create account flows to confirm confirmation or status of created accounts | |
| // For example, github would send an invite as part of its flow and return "poll for invite acceptance" with a task id | |
| service AccountCreationStatusService { | |
| rpc GetAccountCreationStatus(GetAccountCreationStatusRequest) returns (GetAccountCreationStatusResponse); | |
| } | |
| message GetAccountCreationStatusRequest { | |
| string task_id = 1 [(validate.rules).string = { | |
| min_bytes: 1 | |
| max_bytes: 8192 | |
| }]; | |
| repeated google.protobuf.Any annotations = 2; | |
| } |
🧰 Tools
🪛 Buf (1.47.2)
81-81: RPC response type "AccountCreationStatusResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
(RPC_RESPONSE_STANDARD_NAME)
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 81-81:
RPC response type "AccountCreationStatusResponse" should be named "GetAccountCreationStatusResponse" or "AccountCreationStatusServiceGetAccountCreationStatusResponse".
ggreer
force-pushed
the
logan/resource-lookup-service
branch
from
8f96700 to
d7c665a
Compare
…esponse instead of a separate message.
… the only other option is having a CreateAccountResponse in a GetAccountCreationStatusResponse.
ggreer
force-pushed
the
logan/resource-lookup-service
branch
from
4c1f005 to
41a3fcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/connector/connector.go (1)
190-192:⚠️ Potential issueFix inconsistent service registration behavior.
The service registration for
ResourceManagerServiceServeris inconsistent with the pattern used by other services. When the feature is disabled, it should use the noop handler.Apply this diff to maintain consistency:
if cw.getAccountCreationStatusEnabled { connectorV2.RegisterResourceManagerServiceServer(server, cw.server) + } else { + noop := &noopProvisioner{} + connectorV2.RegisterResourceManagerServiceServer(server, noop) }pkg/connectorrunner/runner.go (1)
430-436:⚠️ Potential issueAdd validation for taskId parameter.
The function should validate that the taskId is not empty to prevent issues downstream.
Apply this diff to add validation:
func WithOnDemandGetAccountCreationStatus(taskId string) Option { return func(ctx context.Context, cfg *runnerConfig) error { + if taskId == "" { + return fmt.Errorf("taskId cannot be empty") + } cfg.onDemand = true cfg.accountCreationTaskId = taskId return nil } }pb/c1/connectorapi/baton/v1/baton.pb.validate.go (1)
4035-4138: 🛠️ Refactor suggestionAdd TaskId validation pattern for consistency.
The TaskId field in
Task_AccountCreationStatusTasklacks the standard validation pattern"^[a-zA-Z0-9]{27}$"that is consistently used across other task types in the codebase.Apply this diff to add the validation:
func (m *Task_AccountCreationStatusTask) validate(all bool) error { if m == nil { return nil } var errors []error - // no validation rules for TaskId + if !_Task_AccountCreationStatusTask_TaskId_Pattern.MatchString(m.GetTaskId()) { + err := Task_AccountCreationStatusTaskValidationError{ + field: "TaskId", + reason: "value does not match regex pattern \"^[a-zA-Z0-9]{27}$\"", + } + if !all { + return err + } + errors = append(errors, err) + } if len(errors) > 0 { return Task_AccountCreationStatusTaskMultiError(errors) } return nil } +var _Task_AccountCreationStatusTask_TaskId_Pattern = regexp.MustCompile("^[a-zA-Z0-9]{27}$")
🧹 Nitpick comments (2)
pkg/provisioner/provisioner.go (2)
285-303: Add error details to the log message.The error log message could be more descriptive to help with debugging.
Apply this diff to improve error logging:
- l.Error("Failed to find create account task", zap.Error(err)) + l.Error("Failed to get account creation status", zap.String("taskId", p.createAccountTaskId), zap.Error(err))
395-400: Consider adding dbPath parameter for consistency.Other constructors in this file include
dbPathparameter, but this one doesn't. While it's not used in the current implementation, consider adding it for consistency with other constructors.Apply this diff to maintain consistency:
-func NewAccountCreationStatuser(c types.ConnectorClient, taskId string) *Provisioner { +func NewAccountCreationStatuser(c types.ConnectorClient, dbPath string, taskId string) *Provisioner { return &Provisioner{ + dbPath: dbPath, connector: c, createAccountTaskId: taskId, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource_grpc.pb.gois excluded by!**/*.pb.gopb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (20)
internal/connector/connector.go(4 hunks)internal/connector/noop_provisioner.go(1 hunks)pb/c1/connector/v2/connector.pb.validate.go(0 hunks)pb/c1/connectorapi/baton/v1/baton.pb.validate.go(2 hunks)pkg/cli/commands.go(2 hunks)pkg/connectorbuilder/connectorbuilder.go(5 hunks)pkg/connectorrunner/runner.go(4 hunks)pkg/field/defaults.go(2 hunks)pkg/provisioner/provisioner.go(4 hunks)pkg/sync/syncer_test.go(2 hunks)pkg/tasks/c1api/account_creation_status.go(1 hunks)pkg/tasks/c1api/manager.go(1 hunks)pkg/tasks/local/account_creation_status.go(1 hunks)pkg/tasks/tasks.go(2 hunks)pkg/types/tasks/tasks.go(2 hunks)pkg/types/types.go(2 hunks)pkg/uhttp/authcredentials_test.go(2 hunks)proto/c1/connector/v2/connector.proto(0 hunks)proto/c1/connector/v2/resource.proto(3 hunks)proto/c1/connectorapi/baton/v1/baton.proto(2 hunks)
💤 Files with no reviewable changes (2)
- proto/c1/connector/v2/connector.proto
- pb/c1/connector/v2/connector.pb.validate.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/connector/noop_provisioner.go
- pkg/types/tasks/tasks.go
- pkg/sync/syncer_test.go
- pkg/types/types.go
- pkg/field/defaults.go
- pkg/tasks/tasks.go
- pkg/tasks/c1api/manager.go
- proto/c1/connectorapi/baton/v1/baton.proto
- pkg/tasks/c1api/account_creation_status.go
- pkg/tasks/local/account_creation_status.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/uhttp/authcredentials_test.go
[error] 126-126: unhandled-error: Unhandled error in call to function net/http.Client.Get
(revive)
[error] 193-193: unhandled-error: Unhandled error in call to function net/http.Client.Get
(revive)
🔇 Additional comments (15)
pkg/cli/commands.go (2)
162-167: LGTM!The implementation follows the established pattern for handling command-line flags and correctly integrates with the runner configuration.
301-302: LGTM!The implementation correctly enables account creation status functionality when the flag is set.
internal/connector/connector.go (1)
46-46: LGTM!The new service client is properly integrated into the
connectorClientstruct.pkg/connectorrunner/runner.go (1)
574-576: LGTM!The implementation correctly enables the feature and creates the appropriate task manager.
Also applies to: 613-614
pkg/connectorbuilder/connectorbuilder.go (4)
65-67: LGTM! Well-defined interface.The interface follows Go conventions and has a clear, single responsibility.
99-99: LGTM! Consistent field addition.The new field follows the established pattern for manager fields in the struct.
374-379: LGTM! Proper initialization and duplicate check.The implementation follows the established pattern for manager initialization and duplicate checking.
1014-1018: Add resource existence validation.The method should validate that the returned resource exists before constructing the response.
Apply this diff to add validation:
result, annos, err := b.accountCreationStatusManager.GetAccountCreationStatus(ctx, request.TaskId) if err != nil { b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) return nil, err } + + if result == nil { + b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) + return nil, status.Error(codes.NotFound, "resource not found") + }proto/c1/connector/v2/resource.proto (5)
7-7: LGTM!The
duration.protoimport is correctly added to support the new polling functionality.
78-82: LGTM!The service is well-documented and follows the naming convention suggested in past review comments, making it clear that this is specifically for account creation status.
168-207: Add validation rules for the order field.The
orderfield in theFieldmessage should have validation rules to prevent negative values and set a reasonable maximum.Apply this diff:
message ConnectorAccountCreationSchema { map<string, Field> field_map = 1; message Field { string display_name = 1; bool required = 2; string description = 3; string placeholder = 4; - int32 order = 5; + int32 order = 5 [(validate.rules).int32 = { + gte: 0 + lte: 1000 + }];
221-227: Add validation rules for polling parameters.The polling parameters should have validation rules to ensure reasonable values.
Apply this diff:
message PollForConfirmation { - google.protobuf.Duration polling_interval = 1; + google.protobuf.Duration polling_interval = 1 [(validate.rules).duration = { + gte: {seconds: 1} + lte: {seconds: 3600} // Max 1 hour + }]; - google.protobuf.Duration timeout = 2; + google.protobuf.Duration timeout = 2 [(validate.rules).duration = { + gt: {seconds: 0} + lte: {seconds: 86400} // Max 24 hours + }]; - string task_id = 3; + string task_id = 3 [(validate.rules).string = { + min_bytes: 1 + max_bytes: 8192 + }]; - string description = 4; + string description = 4 [(validate.rules).string = { + max_bytes: 4096 + ignore_empty: true + }];
239-246: LGTM!The response format correctly matches
CreateAccountResponseas suggested in past review comments, supporting multiple polling steps and various scenarios.pkg/uhttp/authcredentials_test.go (2)
126-126: LGTM!The
nolintdirectives are correctly configured for test code where error handling is not required.🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 126-126: unhandled-error: Unhandled error in call to function net/http.Client.Get
(revive)
193-193: LGTM!The
nolintdirectives are correctly configured for test code where error handling is not required.🧰 Tools
🪛 golangci-lint (1.62.2)
[error] 193-193: unhandled-error: Unhandled error in call to function net/http.Client.Get
(revive)
| switch r := result.(type) { | ||
| case *v2.CreateAccountSuccessResult: | ||
| rv.Result = &v2.GetAccountCreationStatusResponse_Success{Success: r} | ||
| case *v2.CreateAccountActionRequiredResult: | ||
| rv.Result = &v2.GetAccountCreationStatusResponse_ActionRequired{ActionRequired: r} | ||
| default: | ||
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | ||
| return nil, status.Error(codes.Unimplemented, fmt.Sprintf("unknown result type: %T", result)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type switch robustness.
The type switch should handle nil results explicitly to prevent potential panics.
Apply this diff to improve the type switch:
switch r := result.(type) {
case *v2.CreateAccountSuccessResult:
rv.Result = &v2.GetAccountCreationStatusResponse_Success{Success: r}
case *v2.CreateAccountActionRequiredResult:
rv.Result = &v2.GetAccountCreationStatusResponse_ActionRequired{ActionRequired: r}
+ case nil:
+ b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start))
+ return nil, status.Error(codes.Internal, "result is nil")
default:
b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start))
return nil, status.Error(codes.Unimplemented, fmt.Sprintf("unknown result type: %T", result))
}📝 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.
| switch r := result.(type) { | |
| case *v2.CreateAccountSuccessResult: | |
| rv.Result = &v2.GetAccountCreationStatusResponse_Success{Success: r} | |
| case *v2.CreateAccountActionRequiredResult: | |
| rv.Result = &v2.GetAccountCreationStatusResponse_ActionRequired{ActionRequired: r} | |
| default: | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.Unimplemented, fmt.Sprintf("unknown result type: %T", result)) | |
| } | |
| switch r := result.(type) { | |
| case *v2.CreateAccountSuccessResult: | |
| rv.Result = &v2.GetAccountCreationStatusResponse_Success{Success: r} | |
| case *v2.CreateAccountActionRequiredResult: | |
| rv.Result = &v2.GetAccountCreationStatusResponse_ActionRequired{ActionRequired: r} | |
| case nil: | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.Internal, "result is nil") | |
| default: | |
| b.m.RecordTaskFailure(ctx, tt, b.nowFunc().Sub(start)) | |
| return nil, status.Error(codes.Unimplemented, fmt.Sprintf("unknown result type: %T", result)) | |
| } |
Create a new resource lookup service
Add protos for better create account followups
The resource lookup request is specifically for using opaque tokens from create responses to find things we don't already know exist
This is different from GetResource or SearchResource
Summary by CodeRabbit
New Features
Bug Fixes
Refactor