Adds identity ID for secrets #293
Conversation
WalkthroughThe pull request introduces a systematic optimization across multiple validation files in the CodeRabbit project. The primary change involves modifying the initialization of error message slices from a standard declaration to a more memory-efficient approach using Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pb/c1/connector/v2/annotation_external_ticket.pb.validate.go (1)
76-76: Performance optimization: Pre-allocating slice capacity.The change optimizes memory allocation by pre-allocating the slice capacity based on the expected number of error messages. This reduces memory reallocations during slice growth.
Consider applying this optimization pattern to other similar slice initializations in the codebase for consistent performance benefits.
pb/c1/utls/v1/tls.pb.validate.go (1)
79-79: Optimization: Pre-allocate slice capacity.The change from
var msgs []stringtomsgs := make([]string, 0, len(m))is a performance optimization that:
- Pre-allocates the exact capacity needed
- Avoids slice growth and reallocation during append operations
- Maintains zero initial length for proper append behavior
This pattern is consistently applied across all validation error types in the generated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (29)
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(10 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(19 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(28 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/types/resource/secret_trait.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- pb/c1/connector/v2/config.pb.validate.go
- pb/c1/connector/v2/asset.pb.validate.go
- pb/c1/connector/v2/event_feed.pb.validate.go
- pb/c1/connector/v2/annotation_etag.pb.validate.go
- pb/c1/connector/v2/connector.pb.validate.go
- pb/c1/ratelimit/v1/ratelimit.pb.validate.go
- pb/c1/connector/v2/entitlement.pb.validate.go
- pb/c1/connector/v2/resource.pb.validate.go
- pb/c1/reader/v2/grant.pb.validate.go
- pb/c1/connector/v2/ticket.pb.validate.go
- pb/c1/connector/v2/grant.pb.validate.go
- pb/c1/connectorapi/baton/v1/baton.pb.validate.go
🧰 Additional context used
🪛 GitHub Check: buf-lint-and-breaking-change-detection
proto/c1/connector/v2/annotation_trait.proto
[failure] 101-101:
Field "5" with name "identity_id" on message "SecretTrait" changed option "json_name" from "createdById" to "identityId".
[failure] 101-101:
Field "5" on message "SecretTrait" changed name from "created_by_id" to "identity_id".
🔇 Additional comments (17)
pkg/types/resource/secret_trait.go (1)
34-38: LGTM! Verify dependent code for breaking changes.The renaming from
created_by_idtoidentity_idis a good semantic improvement. However, this is a breaking change that requires verification of dependent code.Let's verify the impact on dependent code:
✅ Verification successful
Breaking changes verified - all references updated consistently ✅
The renaming from
created_by_idtoidentity_idhas been consistently applied across the codebase, including proto definitions and Go implementations. No lingering references to the old naming were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all references to the old function name and field # to ensure they've been updated consistently echo "Searching for any remaining references to CreatedById..." rg -i "createdbyid|created.?by.?id" --type go echo "Searching for any remaining proto definitions..." rg -i "created.?by.?id" --type protoLength of output: 346
Script:
#!/bin/bash # Find and search proto files echo "=== Searching proto files ===" fd -e proto . -x rg -i "created.?by.?id|identity.?id" {} echo -e "\n=== Searching Go files for WithSecret functions ===" rg "WithSecret\w+ID" --type go -A 2 echo -e "\n=== Searching for IdentityId field usage ===" ast-grep --pattern 'IdentityId = $_'Length of output: 719
pb/c1/connector/v2/annotation_request.pb.validate.go (1)
75-75: LGTM! Performance optimization for error handling.Pre-allocating the slice with the exact capacity needed reduces memory allocations during slice growth.
pb/c1/c1z/v1/annotation_sync_details.pb.validate.go (1)
75-75: LGTM! Consistent performance optimization.The same slice pre-allocation optimization is applied here, maintaining consistency across the codebase.
pb/c1/connector/v2/annotation_v1_identifier.pb.validate.go (1)
75-75: LGTM! Consistent performance optimization.The same slice pre-allocation optimization is applied here, maintaining consistency across validation files.
pb/c1/connector/v2/annotation_entitlement.pb.validate.go (1)
105-105: LGTM! Consistent with the optimization pattern.The change follows the same optimization pattern, improving performance by pre-allocating slice capacity.
pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go (1)
133-133: LGTM! Consistent with the optimization pattern.The change follows the same optimization pattern, improving performance by pre-allocating slice capacity.
pb/c1/connector/v2/annotation_resource_tree.pb.validate.go (1)
76-76: LGTM! Consistent with the optimization pattern.Both changes in this file follow the same optimization pattern, improving performance by pre-allocating slice capacity.
Also applies to: 178-178
pb/c1/reader/v2/entitlement.pb.validate.go (1)
88-88: LGTM! Optimized slice initialization.The change improves performance by pre-allocating the slice capacity based on the expected number of error messages, reducing memory allocations during append operations.
Also applies to: 226-226
pb/c1/reader/v2/resource.pb.validate.go (1)
79-79: LGTM! Consistent slice optimization pattern.The slice initialization changes maintain consistency with the optimization pattern across all error types in the file.
Also applies to: 221-221, 408-408, 543-543
pb/c1/c1z/v1/diff.pb.validate.go (1)
175-175: LGTM! Consistent slice optimization pattern.The slice initialization changes maintain consistency with the optimization pattern across all error types in the file.
Also applies to: 377-377, 578-578, 765-765
pb/c1/connector/v2/annotation_trait.pb.validate.go (2)
334-334: LGTM! Consistent slice optimization pattern.The slice initialization changes maintain consistency with the optimization pattern across all error types in the file.
Also applies to: 491-491, 619-619, 852-852, 1096-1096, 1260-1260, 1386-1386, 1488-1488, 1592-1592
1055-1079: LGTM! Consistent field renaming from CreatedById to IdentityId.The changes correctly update all validation logic and error messages to reflect the new field name while maintaining the same validation rules.
Run the following script to verify the consistency of the field renaming across the codebase:
✅ Verification successful
Field renaming is complete and type-safe
The changes correctly rename the field from
created_by_idtoidentity_idwhile maintaining proper type safety withResourceId. The changes are consistent across proto definitions, generated code, and validation logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to created_by_id have been renamed to identity_id echo "Checking for any remaining references to created_by_id..." rg -i "created_by_id" echo "Verifying the usage of identity_id..." rg -i "identity_id"Length of output: 644
Script:
#!/bin/bash # Description: Verify ResourceId type consistency echo "Checking ResourceId type usage..." ast-grep --pattern 'ResourceId' echo -e "\nChecking ResourceId imports..." rg -A 1 "import.*ResourceId"Length of output: 1301
proto/c1/connector/v2/annotation_trait.proto (1)
101-101: Field rename improves semantic clarity.The change from
created_by_idtoidentity_idbetter represents the field's purpose, shifting from a creator-specific association to a more generic identity relationship.Run the following script to identify potential impacts:
✅ Verification successful
Field rename is safe to proceed
The verification shows no existing usage of
created_by_idin the codebase, indicating this is either a new field or an isolated change. The semantic improvement fromcreated_by_idtoidentity_idcan be safely implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to the old field name in the codebase # Search for direct references to the old field name echo "Searching for direct references to 'created_by_id'..." rg -i 'created_by_id' --type proto --type go # Search for JSON field name usage echo "Searching for JSON field name 'createdById'..." rg -i 'createdById' --type go --type jsonLength of output: 348
Script:
#!/bin/bash # Search for all variants of the old field name with context echo "Searching for snake_case variant..." rg -i "created_by_id" -C 2 echo -e "\nSearching for camelCase variant..." rg -i "createdById" -C 2 echo -e "\nSearching for proto files containing either variant..." fd -e proto -x grep -l -i -E "created_by_id|createdById" {} \;Length of output: 410
🧰 Tools
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 101-101:
Field "5" with name "identity_id" on message "SecretTrait" changed option "json_name" from "createdById" to "identityId".
[failure] 101-101:
Field "5" on message "SecretTrait" changed name from "created_by_id" to "identity_id".pb/c1/connector/v2/annotation_external_link.pb.validate.go (1)
120-120: LGTM! Efficient slice initialization.The change optimizes memory allocation by pre-allocating the slice capacity based on the known number of errors, reducing the need for slice growth and reallocation during append operations.
pb/c1/connector/v2/annotation_ratelimit.pb.validate.go (1)
109-109: LGTM! Consistent optimization pattern.The slice initialization optimization is consistently applied, following the same pattern as other files for efficient memory management.
pb/c1/connector/v2/annotation_grant.pb.validate.go (2)
103-103: LGTM! Systematic performance optimization.The slice capacity pre-allocation optimization has been consistently applied across all MultiError types in the file, demonstrating a systematic approach to memory efficiency.
Also applies to: 205-205, 336-336, 436-436, 538-538
Line range hint
1-1: Note: Changes in generated code.These optimizations are part of the generated code from protoc-gen-validate. While the changes improve performance, any modifications should be made to the generator rather than directly to these files.
| @@ -98,5 +98,5 @@ | |||
| google.protobuf.Timestamp created_at = 2; | |||
| google.protobuf.Timestamp expires_at = 3; | |||
| google.protobuf.Timestamp last_used_at = 4; | |||
| connector.v2.ResourceId created_by_id = 5; | |||
| connector.v2.ResourceId identity_id = 5; | |||
There was a problem hiding this comment.
Breaking Change Alert: Field Rename
The following breaking changes were detected:
- Proto field name changed from
created_by_idtoidentity_id - JSON field name changed from
createdByIdtoidentityId
These changes will impact API consumers and may require coordination for migration.
🧰 Tools
🪛 GitHub Check: buf-lint-and-breaking-change-detection
[failure] 101-101:
Field "5" with name "identity_id" on message "SecretTrait" changed option "json_name" from "createdById" to "identityId".
[failure] 101-101:
Field "5" on message "SecretTrait" changed name from "created_by_id" to "identity_id".
santhosh-c1
force-pushed
the
sb/rename-created-by-secret
branch
from
def6cce to
41be65f
Compare
santhosh-c1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
proto/c1/connector/v2/annotation_trait.proto (1)
101-102: Document the semantic change.The rename from
created_by_idtoidentity_idsuggests a semantic shift from tracking creation to general identity association. This change should be documented.Consider:
- Adding field comments to clarify the purpose of
identity_id- Updating relevant documentation
- Adding migration guides for API consumers
Would you like me to help draft the documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (29)
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(10 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(19 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(28 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/types/resource/secret_trait.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- pb/c1/c1z/v1/annotation_sync_details.pb.validate.go
- pb/c1/utls/v1/tls.pb.validate.go
- pb/c1/connector/v2/annotation_entitlement.pb.validate.go
- pb/c1/connector/v2/annotation_request.pb.validate.go
- pb/c1/connector/v2/annotation_v1_identifier.pb.validate.go
- pb/c1/connector/v2/annotation_external_ticket.pb.validate.go
- pb/c1/connector/v2/annotation_external_link.pb.validate.go
- pb/c1/connector/v2/annotation_ratelimit.pb.validate.go
- pkg/types/resource/secret_trait.go
- pb/c1/connector/v2/annotation_grant.pb.validate.go
- pb/c1/reader/v2/resource.pb.validate.go
- pb/c1/connector/v2/annotation_resource_tree.pb.validate.go
- pb/c1/connector/v2/annotation_etag.pb.validate.go
- pb/c1/connector/v2/event_feed.pb.validate.go
- pb/c1/connector_wrapper/v1/connector_wrapper.pb.validate.go
- pb/c1/connector/v2/asset.pb.validate.go
- pb/c1/reader/v2/entitlement.pb.validate.go
- pb/c1/connector/v2/connector.pb.validate.go
- pb/c1/connector/v2/config.pb.validate.go
- pb/c1/connector/v2/entitlement.pb.validate.go
- pb/c1/c1z/v1/diff.pb.validate.go
- pb/c1/reader/v2/grant.pb.validate.go
- pb/c1/ratelimit/v1/ratelimit.pb.validate.go
- pb/c1/connector/v2/resource.pb.validate.go
- pb/c1/connector/v2/grant.pb.validate.go
- pb/c1/connector/v2/ticket.pb.validate.go
- pb/c1/connector/v2/annotation_trait.pb.validate.go
- pb/c1/connectorapi/baton/v1/baton.pb.validate.go
🔇 Additional comments (1)
proto/c1/connector/v2/annotation_trait.proto (1)
101-102: Breaking Change: Field Rename Impact.The field rename from
created_by_idtoidentity_idis a breaking change that affects API consumers.Let's analyze the impact across the codebase:
| connector.v2.ResourceId created_by_id = 5; | ||
| connector.v2.ResourceId identity_id = 6; |
There was a problem hiding this comment.
Incomplete field migration detected.
The PR's objective is to rename created_by_id to identity_id, but both fields are currently present. This could lead to confusion and inconsistent usage.
To complete the migration:
- Remove the old field
created_by_id - Reuse field number 5 for
identity_idto maintain backward compatibility:
- connector.v2.ResourceId created_by_id = 5;
- connector.v2.ResourceId identity_id = 6;
+ connector.v2.ResourceId identity_id = 5;📝 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.
| connector.v2.ResourceId created_by_id = 5; | |
| connector.v2.ResourceId identity_id = 6; | |
| connector.v2.ResourceId identity_id = 5; |
There was a problem hiding this comment.
We want both in this case.
There was a problem hiding this comment.
@santhosh-c1 I understand now. Thank you for the clarification. Both fields serve distinct purposes in the SecretTrait message.
✏️ Learnings added
Learnt from: santhosh-c1
PR: ConductorOne/baton-sdk#293
File: proto/c1/connector/v2/annotation_trait.proto:101-102
Timestamp: 2025-01-28T10:01:11.131Z
Learning: In the SecretTrait message, both `created_by_id` and `identity_id` fields coexist:
- `created_by_id`: Tracks the identity of who created the secret
- `identity_id`: Associates the secret with an identity that owns/uses it
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Based on the comprehensive review of the changes, here are the updated release notes:
Performance Optimization
Field Renaming
SecretTraitto useidentity_idinstead ofcreated_by_id.Validation Improvements
These changes are primarily internal optimizations that enhance system performance and provide more flexible identity tracking.