Conversation
WalkthroughThe pull request introduces a comprehensive implementation of a secret trait functionality across multiple files. This enhancement involves adding a new Changes
Sequence DiagramsequenceDiagram
participant Resource as Resource
participant SecretTrait as Secret Trait
participant Validator as Trait Validator
Resource->>SecretTrait: Create with options
SecretTrait-->>Resource: Configure trait
Resource->>Validator: Validate resource traits
Validator->>SecretTrait: Check validation rules
Validator-->>Resource: Return validation result
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: 4
🧹 Nitpick comments (5)
pkg/types/resource/secret_trait.go (3)
13-32: Consider adding timestamp validation.While the timestamp option functions are well-implemented, they could benefit from validation to ensure the timestamps make logical sense. For example:
ExpiresAtshould be in the futureLastUsedAtshould not be in the futureCreatedAtshould be beforeLastUsedAtandExpiresAtExample implementation for
WithSecretExpiresAt:func WithSecretExpiresAt(expiresAt time.Time) SecretTraitOption { return func(t *v2.SecretTrait) error { + if expiresAt.Before(time.Now()) { + return fmt.Errorf("expiry time must be in the future") + } t.ExpiresAt = timestamppb.New(expiresAt) return nil } }
34-39: Add input validation for ownerUserID.The function should validate that the
ownerUserIDis not empty and follows any expected format.func WithSecretCreatedByID(ownerUserID string) SecretTraitOption { return func(t *v2.SecretTrait) error { + if ownerUserID == "" { + return fmt.Errorf("ownerUserID cannot be empty") + } t.CreatedById = ownerUserID return nil } }
41-53: Enhance documentation for error handling.The function implementation is solid, but it would benefit from more detailed documentation about error handling and option application order.
+// NewSecretTrait creates a new SecretTrait with the given options. +// Options are applied in the order they are provided. +// Returns an error if any option fails to apply. +// Example: +// +// trait, err := NewSecretTrait( +// WithSecretCreatedAt(time.Now()), +// WithSecretExpiresAt(time.Now().Add(24 * time.Hour)), +// ) func NewSecretTrait(opts ...SecretTraitOption) (*v2.SecretTrait, error) { SecretTrait := &v2.SecretTrait{}pkg/types/resource/resource.go (1)
157-179: Enhance error handling in WithSecretTrait.While the implementation follows the established pattern, it could benefit from more robust error handling and logging.
func WithSecretTrait(opts ...SecretTraitOption) ResourceOption { return func(r *v2.Resource) error { rt := &v2.SecretTrait{} annos := annotations.Annotations(r.Annotations) - _, err := annos.Pick(rt) + picked, err := annos.Pick(rt) if err != nil { return fmt.Errorf("failed to pick secret trait: %w", err) } + if picked { + // Log that we're updating an existing trait + log.Debug("updating existing secret trait") + } for _, o := range opts { err := o(rt) if err != nil { - return err + return fmt.Errorf("failed to apply secret trait option: %w", err) } }proto/c1/connector/v2/resource.proto (1)
Line range hint
95-101: Consider additional security measures for secret management.The introduction of secret management capabilities should be accompanied by security considerations:
- Encryption: Consider requiring encryption for secret data in the profile field
- Access Control: Define granular permissions for secret management
- Audit Logging: Consider requiring audit logging for secret access and modifications
- Rotation: Consider adding rotation-related metadata fields
These security measures are crucial for managing sensitive data like Non Human Identity credentials.
Also applies to: 32-32, 60-60
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
pb/c1/connector/v2/annotation_trait.pb.validate.go(1 hunks)pkg/sync/syncer.go(1 hunks)pkg/types/resource/resource.go(2 hunks)pkg/types/resource/secret_trait.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connector/v2/resource.proto(1 hunks)
🔇 Additional comments (4)
pkg/types/resource/secret_trait.go (2)
1-9: LGTM! Clean and minimal imports.The imports are well-organized and contain only the necessary packages for timestamp handling and protobuf integration.
11-11: LGTM! Well-defined functional option type.The
SecretTraitOptiontype follows the functional options pattern, providing a clean and extensible way to configureSecretTraitinstances.pkg/types/resource/resource.go (1)
324-339: LGTM! Consistent resource creation implementation.The
NewSecretResourcefunction follows the established pattern for resource creation, properly handling options and error propagation.pkg/sync/syncer.go (1)
570-571: LGTM! Clean trait validation integration.The addition of
SecretTraitto the validation switch statement is clean and consistent with the existing pattern.
| // Validate checks the field values on SecretTrait 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 *SecretTrait) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on SecretTrait 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 SecretTraitMultiError, or | ||
| // nil if none found. | ||
| func (m *SecretTrait) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *SecretTrait) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetProfile()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetProfile()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetCreatedAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetCreatedAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetExpiresAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetExpiresAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetLastUsedAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetLastUsedAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // no validation rules for CreatedById | ||
|
|
||
| if len(errors) > 0 { | ||
| return SecretTraitMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // SecretTraitMultiError is an error wrapping multiple validation errors | ||
| // returned by SecretTrait.ValidateAll() if the designated constraints aren't met. | ||
| type SecretTraitMultiError []error | ||
|
|
||
| // Error returns a concatenation of all the error messages it wraps. | ||
| func (m SecretTraitMultiError) Error() string { | ||
| var msgs []string | ||
| for _, err := range m { | ||
| msgs = append(msgs, err.Error()) | ||
| } | ||
| return strings.Join(msgs, "; ") | ||
| } | ||
|
|
||
| // AllErrors returns a list of validation violation errors. | ||
| func (m SecretTraitMultiError) AllErrors() []error { return m } | ||
|
|
||
| // SecretTraitValidationError is the validation error returned by | ||
| // SecretTrait.Validate if the designated constraints aren't met. | ||
| type SecretTraitValidationError struct { | ||
| field string | ||
| reason string | ||
| cause error | ||
| key bool | ||
| } | ||
|
|
||
| // Field function returns field value. | ||
| func (e SecretTraitValidationError) Field() string { return e.field } | ||
|
|
||
| // Reason function returns reason value. | ||
| func (e SecretTraitValidationError) Reason() string { return e.reason } | ||
|
|
||
| // Cause function returns cause value. | ||
| func (e SecretTraitValidationError) Cause() error { return e.cause } | ||
|
|
||
| // Key function returns key value. | ||
| func (e SecretTraitValidationError) Key() bool { return e.key } | ||
|
|
||
| // ErrorName returns error name. | ||
| func (e SecretTraitValidationError) ErrorName() string { return "SecretTraitValidationError" } | ||
|
|
||
| // Error satisfies the builtin error interface | ||
| func (e SecretTraitValidationError) 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 %sSecretTrait.%s: %s%s", | ||
| key, | ||
| e.field, | ||
| e.reason, | ||
| cause) | ||
| } | ||
|
|
||
| var _ error = SecretTraitValidationError{} | ||
|
|
||
| var _ interface { | ||
| Field() string | ||
| Reason() string | ||
| Key() bool | ||
| Cause() error | ||
| ErrorName() string | ||
| } = SecretTraitValidationError{} | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
The proto files have been modified after the last code generation. Please run:
make protogento regenerate the validation code.
🔗 Analysis chain
Verify protobuf generation is up-to-date.
The validation code appears to be properly generated, but ensure it's up-to-date with the latest proto definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the generated code is up-to-date with proto definitions
# Check if proto files have been modified more recently than generated files
find . -name "*.proto" -newer pb/c1/connector/v2/annotation_trait.pb.validate.go -exec echo "Warning: {} is newer than generated files" \;
Length of output: 384
Script:
#!/bin/bash
# Look for Makefile and generation scripts
echo "=== Searching for Makefile ==="
fd Makefile
echo -e "\n=== Searching for proto generation scripts ==="
fd -e sh -e bash | grep -i "proto\|gen"
echo -e "\n=== Checking Makefile content for proto targets ==="
fd Makefile -x grep -l "proto\|buf\|protoc" {} \; -x cat {}
Length of output: 2666
| message SecretTrait { | ||
| google.protobuf.Struct profile = 1; | ||
| google.protobuf.Timestamp created_at = 2; | ||
| google.protobuf.Timestamp expires_at = 3; | ||
| google.protobuf.Timestamp last_used_at = 4; | ||
| string created_by_id = 5; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation rules and documentation for SecretTrait fields.
The SecretTrait message is missing:
- Field validation rules (e.g., for created_by_id)
- Documentation comments explaining the purpose and constraints of each field
Apply this diff to add validation and documentation:
message SecretTrait {
+ // Profile contains arbitrary metadata about the secret
google.protobuf.Struct profile = 1;
+ // Time when the secret was created
google.protobuf.Timestamp created_at = 2;
+ // Time when the secret expires (optional)
google.protobuf.Timestamp expires_at = 3;
+ // Last time the secret was accessed
google.protobuf.Timestamp last_used_at = 4;
+ // ID of the user or service that created this secret
- string created_by_id = 5;
+ string created_by_id = 5 [(validate.rules).string = {
+ min_bytes: 1
+ max_bytes: 1024
+ }];
}📝 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 SecretTrait { | |
| google.protobuf.Struct profile = 1; | |
| google.protobuf.Timestamp created_at = 2; | |
| google.protobuf.Timestamp expires_at = 3; | |
| google.protobuf.Timestamp last_used_at = 4; | |
| string created_by_id = 5; | |
| } | |
| message SecretTrait { | |
| // Profile contains arbitrary metadata about the secret | |
| google.protobuf.Struct profile = 1; | |
| // Time when the secret was created | |
| google.protobuf.Timestamp created_at = 2; | |
| // Time when the secret expires (optional) | |
| google.protobuf.Timestamp expires_at = 3; | |
| // Last time the secret was accessed | |
| google.protobuf.Timestamp last_used_at = 4; | |
| // ID of the user or service that created this secret | |
| string created_by_id = 5 [(validate.rules).string = { | |
| min_bytes: 1 | |
| max_bytes: 1024 | |
| }]; | |
| } |
santhosh-c1
force-pushed
the
mt-secret
branch
from
cd3b2f9 to
7a21e9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/types/resource/secret_trait.go (1)
41-53: Consider adding validation for the SecretTrait fields.While the constructor properly applies the options, it should validate that required fields are set and that timestamps are logically consistent (e.g., expiration time should be after creation time).
func NewSecretTrait(opts ...SecretTraitOption) (*v2.SecretTrait, error) { SecretTrait := &v2.SecretTrait{} for _, opt := range opts { err := opt(SecretTrait) if err != nil { return nil, err } } + // Validate required fields + if SecretTrait.CreatedAt == nil { + return nil, fmt.Errorf("created_at is required") + } + + // Validate timestamp logic + if SecretTrait.ExpiresAt != nil && SecretTrait.ExpiresAt.AsTime().Before(SecretTrait.CreatedAt.AsTime()) { + return nil, fmt.Errorf("expires_at must be after created_at") + } + return SecretTrait, nil }pkg/types/resource/resource.go (1)
157-179: Consider handling existing trait updates more explicitly.The
WithSecretTraitfunction could be more explicit about updating existing traits vs. creating new ones.func WithSecretTrait(opts ...SecretTraitOption) ResourceOption { return func(r *v2.Resource) error { rt := &v2.SecretTrait{} annos := annotations.Annotations(r.Annotations) - _, err := annos.Pick(rt) + picked, err := annos.Pick(rt) if err != nil { return err } - for _, o := range opts { - err := o(rt) + if picked { + // Update existing trait + for _, o := range opts { + err := o(rt) + if err != nil { + return err + } + } + } else { + // Create new trait + rt, err = NewSecretTrait(opts...) if err != nil { return err } } annos.Update(rt) r.Annotations = annos return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.gopb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
pb/c1/connector/v2/annotation_trait.pb.validate.go(1 hunks)pkg/sync/syncer.go(1 hunks)pkg/types/resource/resource.go(2 hunks)pkg/types/resource/secret_trait.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(1 hunks)proto/c1/connector/v2/connector.proto(1 hunks)proto/c1/connector/v2/resource.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- proto/c1/connector/v2/connector.proto
- proto/c1/connector/v2/resource.proto
- pkg/sync/syncer.go
🔇 Additional comments (5)
pkg/types/resource/secret_trait.go (2)
11-11: LGTM! Well-defined option type.The
SecretTraitOptiontype follows the functional options pattern, providing a clean and extensible way to configureSecretTraitinstances.
13-18: LGTM! Well-implemented option functions.The option functions are well-structured and follow a consistent pattern:
WithSecretCreatedAtWithSecretLastUsedAtWithSecretExpiresAtWithSecretCreatedByIDEach function properly handles its respective field using the protobuf timestamp conversion where needed.
Also applies to: 20-25, 27-32, 34-39
proto/c1/connector/v2/annotation_trait.proto (1)
95-101: Add validation rules and documentation for SecretTrait fields.The SecretTrait message needs:
- Field validation rules (e.g., for created_by_id)
- Documentation comments explaining the purpose and constraints of each field
Apply this diff to add validation and documentation:
message SecretTrait { + // Profile contains arbitrary metadata about the secret google.protobuf.Struct profile = 1; + // Time when the secret was created google.protobuf.Timestamp created_at = 2; + // Time when the secret expires (optional) google.protobuf.Timestamp expires_at = 3; + // Last time the secret was accessed google.protobuf.Timestamp last_used_at = 4; + // ID of the user or service that created this secret - string created_by_id = 5; + string created_by_id = 5 [(validate.rules).string = { + min_bytes: 1 + max_bytes: 1024 + }]; }pkg/types/resource/resource.go (1)
324-339: LGTM! Well-implemented resource constructor.The
NewSecretResourcefunction follows the established pattern of other resource constructors in the codebase.pb/c1/connector/v2/annotation_trait.pb.validate.go (1)
916-1132:⚠️ Proto files are out of sync - regeneration needed.The proto files have been modified after the last code generation. Please run:
make protogento regenerate the validation code.
| google.protobuf.Timestamp created_at = 2; | ||
| google.protobuf.Timestamp expires_at = 3; | ||
| google.protobuf.Timestamp last_used_at = 4; | ||
| string created_by_id = 5; |
There was a problem hiding this comment.
Do we want to make this a resource id?
There was a problem hiding this comment.
This is technically a resource ID with user trait. How do we make this specifically be a resource ID?
Ah, just saw this: message ResourceId. That kinda makes sense. Will take a look. Thanks!
There was a problem hiding this comment.
What does this bool mean: bool baton_resource = 3; ?
There was a problem hiding this comment.
It's so that we can use resources or resource IDs in grants:
baton-sdk/pkg/types/grant/grant.go
Line 17 in d99149d
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 (1)
pb/c1/connector/v2/annotation_trait.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
pb/c1/connector/v2/annotation_trait.pb.validate.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(2 hunks)
🔇 Additional comments (3)
proto/c1/connector/v2/annotation_trait.proto (3)
6-6: LGTM!The import of
resource.protois correctly added and necessary for using theResourceIdtype inSecretTrait.
97-101: LGTM!The field structure is well-organized with appropriate protobuf types:
google.protobuf.Structfor flexible metadatagoogle.protobuf.Timestampfor all time-related fields- Logical ordering of fields
96-102: 🛠️ Refactor suggestionAdd field validation rules and documentation.
The SecretTrait message would benefit from:
- Field validation rules (e.g., required fields)
- Documentation comments explaining the purpose and constraints of each field
Apply this diff to add validation and documentation:
message SecretTrait { + // Profile contains arbitrary metadata about the secret google.protobuf.Struct profile = 1; + // Time when the secret was created + // @required google.protobuf.Timestamp created_at = 2 [(validate.rules).message = {required: true}]; + // Time when the secret expires (optional) google.protobuf.Timestamp expires_at = 3; + // Last time the secret was accessed google.protobuf.Timestamp last_used_at = 4; + // ID of the user or service that created this secret + // @required connector.v2.ResourceId created_by_id = 5 [(validate.rules).message = {required: true}]; }Likely invalid or redundant comment.
| // Validate checks the field values on SecretTrait 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 *SecretTrait) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on SecretTrait 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 SecretTraitMultiError, or | ||
| // nil if none found. | ||
| func (m *SecretTrait) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *SecretTrait) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetProfile()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetProfile()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "Profile", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetCreatedAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetCreatedAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "CreatedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetExpiresAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetExpiresAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "ExpiresAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetLastUsedAt()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetLastUsedAt()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "LastUsedAt", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if all { | ||
| switch v := interface{}(m.GetCreatedById()).(type) { | ||
| case interface{ ValidateAll() error }: | ||
| if err := v.ValidateAll(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedById", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| case interface{ Validate() error }: | ||
| if err := v.Validate(); err != nil { | ||
| errors = append(errors, SecretTraitValidationError{ | ||
| field: "CreatedById", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| }) | ||
| } | ||
| } | ||
| } else if v, ok := interface{}(m.GetCreatedById()).(interface{ Validate() error }); ok { | ||
| if err := v.Validate(); err != nil { | ||
| return SecretTraitValidationError{ | ||
| field: "CreatedById", | ||
| reason: "embedded message failed validation", | ||
| cause: err, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(errors) > 0 { | ||
| return SecretTraitMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // SecretTraitMultiError is an error wrapping multiple validation errors | ||
| // returned by SecretTrait.ValidateAll() if the designated constraints aren't met. | ||
| type SecretTraitMultiError []error | ||
|
|
||
| // Error returns a concatenation of all the error messages it wraps. | ||
| func (m SecretTraitMultiError) Error() string { | ||
| var msgs []string | ||
| for _, err := range m { | ||
| msgs = append(msgs, err.Error()) | ||
| } | ||
| return strings.Join(msgs, "; ") | ||
| } | ||
|
|
||
| // AllErrors returns a list of validation violation errors. | ||
| func (m SecretTraitMultiError) AllErrors() []error { return m } | ||
|
|
||
| // SecretTraitValidationError is the validation error returned by | ||
| // SecretTrait.Validate if the designated constraints aren't met. | ||
| type SecretTraitValidationError struct { | ||
| field string | ||
| reason string | ||
| cause error | ||
| key bool | ||
| } | ||
|
|
||
| // Field function returns field value. | ||
| func (e SecretTraitValidationError) Field() string { return e.field } | ||
|
|
||
| // Reason function returns reason value. | ||
| func (e SecretTraitValidationError) Reason() string { return e.reason } | ||
|
|
||
| // Cause function returns cause value. | ||
| func (e SecretTraitValidationError) Cause() error { return e.cause } | ||
|
|
||
| // Key function returns key value. | ||
| func (e SecretTraitValidationError) Key() bool { return e.key } | ||
|
|
||
| // ErrorName returns error name. | ||
| func (e SecretTraitValidationError) ErrorName() string { return "SecretTraitValidationError" } | ||
|
|
||
| // Error satisfies the builtin error interface | ||
| func (e SecretTraitValidationError) 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 %sSecretTrait.%s: %s%s", | ||
| key, | ||
| e.field, | ||
| e.reason, | ||
| cause) | ||
| } | ||
|
|
||
| var _ error = SecretTraitValidationError{} | ||
|
|
||
| var _ interface { | ||
| Field() string | ||
| Reason() string | ||
| Key() bool | ||
| Cause() error | ||
| ErrorName() string | ||
| } = SecretTraitValidationError{} | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Proto files are newer than generated validation code - regeneration needed
The following proto files have been modified more recently than the generated validation code:
./proto/c1/connector/v2/annotation_trait.proto./proto/c1/connector/v2/connector.proto./proto/c1/connector/v2/resource.proto
🔗 Analysis chain
Regenerate validation code after adding validation rules.
The validation code needs to be regenerated after adding the suggested validation rules to the proto file.
Run the following command to regenerate the validation code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if proto files are in sync with generated code
# Check if proto files have been modified more recently than generated files
find . -name "*.proto" -newer pb/c1/connector/v2/annotation_trait.pb.validate.go -exec echo "Warning: {} is newer than generated files" \;
Length of output: 384
santhosh-c1
force-pushed
the
mt-secret
branch
from
1f13f2f to
95f9cda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pb/c1/connector/v2/annotation_trait.pb.validate.go (1)
931-1088: Consider adding specific validation rules for SecretTrait fields.While the embedded message validation is implemented correctly, consider adding specific validation rules for SecretTrait fields. For example:
- Validate that ExpiresAt is after CreatedAt
- Validate that LastUsedAt is not before CreatedAt
- Add length/format validations for any string fields in the trait
These validations would need to be added to the proto file first, then regenerate this validation 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 (3)
pb/c1/connector/v2/annotation_trait.pb.validate.go(1 hunks)pkg/types/resource/secret_trait.go(1 hunks)proto/c1/connector/v2/annotation_trait.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- proto/c1/connector/v2/annotation_trait.proto
- pkg/types/resource/secret_trait.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
🔇 Additional comments (3)
pb/c1/connector/v2/annotation_trait.pb.validate.go (3)
916-929: LGTM - Validation methods follow the established pattern.The implementation of
Validate()andValidateAll()methods forSecretTraitis consistent with other trait types in the file.
1090-1159: LGTM - Error types and interfaces are properly implemented.The implementation of error types and interfaces for SecretTrait validation follows the established pattern and correctly implements all required interfaces.
916-1159: Verify proto files are in sync with generated code.Since there were previous comments about proto files being out of sync, let's verify the current state:
✅ Verification successful
Proto files are correctly synced with generated code
The validation code matches the proto definition exactly, and the timestamps (Jan 13 04:29) show both files were updated together. The earlier timestamp warning appears to be a false positive.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that proto files and generated code are in sync # Check if proto files have been modified more recently than generated files find . -name "*.proto" -newer pb/c1/connector/v2/annotation_trait.pb.validate.go -exec echo "Warning: {} is newer than generated files" \; # Look for the proto definition to verify validation rules echo -e "\nChecking SecretTrait definition in proto files:" rg -A 5 "message SecretTrait" "*.proto"Length of output: 585
Script:
#!/bin/bash # Search for SecretTrait definition in the proto directory echo "Checking SecretTrait definition in proto files:" rg -A 5 "message SecretTrait" ./proto # Show timestamp details for relevant files echo -e "\nFile timestamps:" ls -l ./proto/c1/connector/v2/annotation_trait.proto ./pb/c1/connector/v2/annotation_trait.pb.validate.goLength of output: 1022
3 participants
Summary by CodeRabbit
New Features
Improvements
Protocol Updates
SecretTraitmessage type.CAPABILITY_SYNC_SECRETSandTRAIT_SECRETto existing enums.