Open
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end support for a SYSTEM_KEY_ROTATE job type: proto enum and timestamp field changes, new JobType constant, factory method to create SYSTEM_KEY_ROTATE jobs (without mutating system status), resolver support and validation (including key ID equality and rotation-time/version propagation), a dedicated SystemKeyRotate job handler with done/failed/canceled flows and version-mismatch detection helper, reconciler registration, manager permission/ retry adjustments, tests covering resolver/handler/factory behavior, and related keystore plugin and API extensions plus tooling (sonar) updates. Changes
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/event-processor/reconciler_test.go (1)
468-505: Consider asserting the parsedRotationTimevalue in the happy path test.The test verifies key and version fields but doesn't assert that
sa.GetRotationTime()is correctly parsed from"2024-01-15T10:30:00Z". This would ensure end-to-end correctness of the timestamp conversion.💡 Suggested assertion addition
case eventProto.TaskType_SYSTEM_KEY_ROTATE.String(): assert.Equal(t, keyTo.ID.String(), sa.GetKeyIdTo()) assert.Equal(t, keyTo.ID.String(), sa.GetKeyIdFrom()) // Same key assert.Equal(t, "version-1", sa.GetKeyVersionIdFrom()) assert.Equal(t, "version-2", sa.GetKeyVersionIdTo()) + // Verify rotation time is correctly parsed (for non-empty timestamp case) + if tt.name == "SYSTEM_KEY_ROTATE task" { + assert.NotNil(t, sa.GetRotationTime()) + assert.Equal(t, int64(1705315800), sa.GetRotationTime().GetSeconds()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/event-processor/reconciler_test.go` around lines 468 - 505, Add assertions in the "SYSTEM_KEY_ROTATE task" test case to verify RotationTime is parsed correctly: after unmarshalling into sa (from SystemActionJobData), assert that sa.GetRotationTime() is non-nil and equals time.Parse(time.RFC3339, "2024-01-15T10:30:00Z") for the happy-path case, and for the "SYSTEM_KEY_ROTATE task with empty timestamp" case assert that sa.GetRotationTime() is nil; reference the test cases by their names and the SystemActionJobData -> sa.GetRotationTime() accessor to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/event-processor/handlers_system.go`:
- Around line 383-394: The IsVersionMismatchError branch currently only logs and
then lets execution fall through to terminateFailedSystemJob (which sets
system.Status = FAILED); change this by short-circuiting when
IsVersionMismatchError(errorMessage) is true: after logging (the existing
log.Warn) immediately return from the handler without calling
terminateFailedSystemJob so the system is not downgraded by an out-of-order
SYSTEM_KEY_ROTATE event; keep the TODO about retry/recreate, but ensure no state
change occurs for this branch (references: IsVersionMismatchError,
terminateFailedSystemJob, SYSTEM_KEY_ROTATE, and the code path that sets
system.Status = FAILED).
- Around line 343-360: The handler currently returns an error when
getSystemByID(ctx, h.repo, data.SystemID) yields repo.ErrNotFound, which
prevents cleanUpEvent from running and causes retries for already-deleted
systems; update the error handling around getSystemByID so that if
errors.Is(err, repo.ErrNotFound) you treat it as a non-fatal case (proceed to
cleanUpEvent and exit normally) while still returning for other errors;
reference getSystemByID, repo.ErrNotFound, and cleanUpEvent to locate and adjust
the branch, or alternatively defer calling getSystemByID until the audit log
path (cmkAuditor.SendCmkKeyRotationAuditLog) is actually used.
---
Nitpick comments:
In `@internal/event-processor/reconciler_test.go`:
- Around line 468-505: Add assertions in the "SYSTEM_KEY_ROTATE task" test case
to verify RotationTime is parsed correctly: after unmarshalling into sa (from
SystemActionJobData), assert that sa.GetRotationTime() is non-nil and equals
time.Parse(time.RFC3339, "2024-01-15T10:30:00Z") for the happy-path case, and
for the "SYSTEM_KEY_ROTATE task with empty timestamp" case assert that
sa.GetRotationTime() is nil; reference the test cases by their names and the
SystemActionJobData -> sa.GetRotationTime() accessor to locate where to add
these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a21d5435-5947-4cf6-8b1e-0b05c4ae5da9
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/factory.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.go
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
3262c96 to
5401642
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/event-processor/handlers_system.go`:
- Around line 406-408: The HandleJobCanceledEvent currently calls
terminateCanceledSystemJob which sets system.Status = cmkapi.SystemStatusFAILED;
change HandleJobCanceledEvent in SystemKeyRotateJobHandler to avoid modifying
CMK system state on cancellation—log the cancellation and return without calling
terminateCanceledSystemJob (mirror the version-mismatch handling approach used
elsewhere), so no system.Status updates or persistence occur for
SYSTEM_KEY_ROTATE cancellation events; keep references to HandleJobCanceledEvent
and terminateCanceledSystemJob to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d5c0468-766e-48ad-b1ff-63ac5ade2a94
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/factory.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.go
✅ Files skipped from review due to trivial changes (1)
- internal/event-processor/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/event-processor/errors.go
- internal/event-processor/proto/task.proto
- internal/event-processor/resolvers.go
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
5401642 to
c89d1b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/event-processor/reconciler_test.go (1)
1088-1225: Cover the handler-specific branches too.This new suite still doesn't lock in
HandleJobConfirmalways completing orHandleJobDoneEventcleaning up when the system is already gone. Those are the two custom branches inSystemKeyRotateJobHandlerand easy to regress later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/event-processor/reconciler_test.go` around lines 1088 - 1225, Add tests to exercise the SystemKeyRotateJobHandler branches for HandleJobConfirm and the case where HandleJobDoneEvent must clean up when the target system is missing: create job data via the existing createJobData helper, then (1) call getHandler().HandleJobConfirm(ctx, job) with a job whose ExternalID either exists or is missing and assert it returns nil (completes) and that any created/updated entities are in the expected state, and (2) create and then delete the system before calling getHandler().HandleJobDoneEvent(ctx, job) and assert the event is still cleaned up (deleted) even when the System referenced in the job data no longer exists; reference the SystemKeyRotateJobHandler and the methods HandleJobConfirm and HandleJobDoneEvent to locate the implementation branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/event-processor/handlers_system.go`:
- Around line 391-401: The early return in the IsVersionMismatchError branch
leaves the event row unchanged and no failure reason recorded; before returning,
persist the errorMessage to the event (or call terminateFailedSystemJob) so the
event row reflects the outcome. Locate the IsVersionMismatchError check in
handlers_system.go and, instead of returning immediately, call the existing
terminateFailedSystemJob (or an updateEventStatus helper) with the event
identifier and errorMessage so the event is updated/cleaned up (include context
such as data.KeyIDTo and data.SystemID in the log/update), then return nil.
In `@internal/event-processor/reconciler_test.go`:
- Around line 469-506: The test cases for SYSTEM_KEY_ROTATE (the table entries
named "SYSTEM_KEY_ROTATE task" and "SYSTEM_KEY_ROTATE task with empty
timestamp") never assert the RotationTime field; update the assertions in the
test loop that currently checks key/version IDs to also assert RotationTime on
the decoded/constructed job (eventprocessor.SystemActionJobData or the struct
returned by the reconciler): for the populated case assert RotationTime equals
the parsed time "2024-01-15T10:30:00Z" (or the equivalent time.Time) and for the
empty-string case assert RotationTime is nil/zero as expected. Ensure you
reference the same variable used in the existing checks so the new assertions
run alongside the key/version ID assertions.
In `@internal/event-processor/resolvers.go`:
- Around line 130-133: The SYSTEM_KEY_ROTATE event path currently serializes
version changes using KeyVersionIdFrom/KeyVersionIdTo without verifying the key
IDs; add a guard in the rotate handler (the code that processes
SYSTEM_KEY_ROTATE and builds the version info with
KeyVersionIdFrom/KeyVersionIdTo/RotationTime) to reject payloads where KeyIdFrom
!= KeyIdTo (return an error or drop the event) so cross-key rotations cannot be
processed; ensure the check runs before any metadata lookup or serialization
that assumes a single key (use the existing KeyIdFrom and KeyIdTo fields to
perform this equality check).
- Around line 104-112: The code currently parses and rejects non-empty
data.RotationTime for every SystemActionJobData, causing failures for non-rotate
actions; change the logic so rotationTimestamp is only parsed/validated when the
job's action/type equals SYSTEM_KEY_ROTATE (check the field on the
SystemActionJobData instance that indicates the action), otherwise ignore any
non-empty RotationTime and leave rotationTimestamp nil; update the block
referenced (RotationTime, rotationTimestamp, SystemActionJobData) to first guard
on the action (SYSTEM_KEY_ROTATE) before attempting time.Parse and returning an
error.
---
Nitpick comments:
In `@internal/event-processor/reconciler_test.go`:
- Around line 1088-1225: Add tests to exercise the SystemKeyRotateJobHandler
branches for HandleJobConfirm and the case where HandleJobDoneEvent must clean
up when the target system is missing: create job data via the existing
createJobData helper, then (1) call getHandler().HandleJobConfirm(ctx, job) with
a job whose ExternalID either exists or is missing and assert it returns nil
(completes) and that any created/updated entities are in the expected state, and
(2) create and then delete the system before calling
getHandler().HandleJobDoneEvent(ctx, job) and assert the event is still cleaned
up (deleted) even when the System referenced in the job data no longer exists;
reference the SystemKeyRotateJobHandler and the methods HandleJobConfirm and
HandleJobDoneEvent to locate the implementation branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46b563c9-fe0a-4383-805d-3451d9213904
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/factory.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.gosonar-project.properties
✅ Files skipped from review due to trivial changes (1)
- sonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/event-processor/reconciler.go
- internal/event-processor/errors_test.go
- internal/event-processor/types.go
- internal/event-processor/factory.go
- internal/event-processor/proto/task.proto
minh-nghia
reviewed
Mar 24, 2026
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
c89d1b4 to
b29837c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/event-processor/reconciler.go (1)
167-177: Plan a compatibility window for queuedKEY_ROTATEwork.Line 172 only registers
SYSTEM_KEY_ROTATE. If Orbital can still hold persisted rotate jobs from the previous release,GetHandlerByJobTypewill start treating them as unsupported after deploy. A temporary alias or a one-off rewrite makes this cutover safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/event-processor/reconciler.go` around lines 167 - 177, The jobHandlerMap only registers JobTypeSystemKeyRotate and will treat older persisted KEY_ROTATE jobs as unsupported; add a temporary alias entry mapping the legacy JobTypeKeyRotate to the same handler instance (use the same constructor used for JobTypeSystemKeyRotate) so GetHandlerByJobType continues to find a handler for older queued rotate jobs, and keep this alias until the compatibility window ends so you can safely remove it later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/event-processor/handlers_system.go`:
- Around line 406-418: HandleJobCanceledEvent currently logs and calls
cleanUpEvent without recreating the tenant-scoped context, which can leave the
canceled system key rotate event row behind; update HandleJobCanceledEvent
(after unmarshalSystemJobData returns `data`) to recreate the tenant context the
same way HandleJobDoneEvent and HandleJobFailedEvent do (use the same
tenant-scoping logic before calling cleanUpEvent) so cleanUpEvent runs within
the tenant scope and removes the event row from the tenant store.
In `@internal/event-processor/resolvers.go`:
- Around line 94-95: Create a sentinel error named ErrSystemKeyRotateCrossKey in
the existing var block in reconciler.go (alongside ErrInvalidJobType, etc.),
then replace the dynamic fmt.Errorf at the check in resolvers.go (the if that
tests taskType == proto.TaskType_SYSTEM_KEY_ROTATE && data.KeyIDFrom !=
data.KeyIDTo) with an error wrap that returns ErrSystemKeyRotateCrossKey
annotated with the KeyIDFrom/KeyIDTo values so callers receive the sentinel for
comparisons while preserving the contextual key IDs in the returned error.
In `@internal/manager/system.go`:
- Around line 672-677: The case currently marks JobTypeSystemKeyRotate as
retryable which can leave the system stuck in PROCESSING because
retrySystemAction patches to PROCESSING before the rotate handler restores
status; remove JobTypeSystemKeyRotate from the retryable branch so only
JobTypeSystemLink uses canRetry = checkAuth(...), leaving canCancel = true
unchanged, and add a TODO comment referencing
SystemKeyRotateJobHandler.HandleJobDoneEvent to revisit enabling retries once
that handler reliably restores status.
---
Nitpick comments:
In `@internal/event-processor/reconciler.go`:
- Around line 167-177: The jobHandlerMap only registers JobTypeSystemKeyRotate
and will treat older persisted KEY_ROTATE jobs as unsupported; add a temporary
alias entry mapping the legacy JobTypeKeyRotate to the same handler instance
(use the same constructor used for JobTypeSystemKeyRotate) so
GetHandlerByJobType continues to find a handler for older queued rotate jobs,
and keep this alias until the compatibility window ends so you can safely remove
it later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5045a3ac-f38b-4f6d-8127-d946869d53a0
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/export_test.gointernal/event-processor/factory.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.gointernal/manager/system.gosonar-project.properties
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- internal/event-processor/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/event-processor/factory.go
- internal/event-processor/proto/task.proto
- internal/event-processor/reconciler_test.go
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
2 times, most recently
from
022c303 to
1fa09c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/event-processor/reconciler.go (1)
168-178:⚠️ Potential issue | 🟠 MajorKeep a fallback for pre-upgrade
KEY_ROTATErows.If this lands while old
KEY_ROTATEjobs/events are still persisted, they will now fall throughGetHandlerByJobTypeas unsupported because onlySYSTEM_KEY_ROTATEis registered here. Please keep a temporary handler alias or migrate outstanding records before rollout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/event-processor/reconciler.go` around lines 168 - 178, The map in reconciler.go registers a SYSTEM_KEY_ROTATE handler but drops legacy KEY_ROTATE rows, causing GetHandlerByJobType to return unsupported for pre-upgrade jobs; add an alias entry mapping JobTypeKeyRotate to the same handler as JobTypeSystemKeyRotate (i.e., use NewSystemKeyRotateJobHandler(repository, cmkAuditor, manager, systemResolver) for JobTypeKeyRotate) or otherwise ensure outstanding KEY_ROTATE records are migrated before rollout so GetHandlerByJobType continues to find a handler.
🧹 Nitpick comments (1)
internal/event-processor/reconciler_test.go (1)
469-506: Add the missing mismatched-key regression test.
SYSTEM_KEY_ROTATEnow relies onKeyIDFrom == KeyIDTo, but the new resolver coverage only validates timestamp parsing. A negative case here would lock down that invariant and prevent rotate requests from silently turning into cross-key switches.Also applies to: 671-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/event-processor/reconciler_test.go` around lines 469 - 506, The test suite is missing a negative regression case ensuring SystemActionJobData enforces KeyIDFrom == KeyIDTo for JobTypeSystemKeyRotate/TaskType_SYSTEM_KEY_ROTATE; add a test entry in reconciler_test.go (alongside the existing SYSTEM_KEY_ROTATE cases) that sets KeyIDFrom to a different key than KeyIDTo and asserts the reconciler returns/records a validation error or rejects the job, referencing SystemActionJobData, KeyIDFrom, KeyIDTo and the JobTypeSystemKeyRotate/TaskType_SYSTEM_KEY_ROTATE identifiers; replicate the same negative-case addition for the other block mentioned (around lines 671-689) to lock down the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/event-processor/errors.go`:
- Around line 66-82: The code currently uses a placeholder
KernelServiceVersionMismatchCode and IsVersionMismatchError() is relied upon in
HandleJobFailedEvent (which triggers cleanUpEvent) — gate this behavior until
the KS contract is verified by disabling or neutralizing the placeholder branch:
update KernelServiceVersionMismatchCode handling so
IsVersionMismatchError(errorMessage) does not return true for
unknown/placeholder codes (e.g., make it validate against a confirmed contract
or temporarily always return false and log a clear TODO), and add a comment
referencing HandleJobFailedEvent and cleanUpEvent so reviewers know this is
intentionally disabled until KS provides the real code.
In `@internal/event-processor/factory_test.go`:
- Around line 311-333: The test shows SystemKeyRotate events reuse the system ID
as the job ExternalID; update createSystemEventJob (the SystemKeyRotate branch)
so it does not set event.Identifier = data.SystemID but instead generates a
unique identifier per rotation (e.g., a UUID) for event.Identifier which flows
into CreateJob/job.ExternalID, while keeping data.SystemID inside the event
payload; this ensures each rotation has a distinct ExternalID to avoid
collisions during retries/audits.
---
Outside diff comments:
In `@internal/event-processor/reconciler.go`:
- Around line 168-178: The map in reconciler.go registers a SYSTEM_KEY_ROTATE
handler but drops legacy KEY_ROTATE rows, causing GetHandlerByJobType to return
unsupported for pre-upgrade jobs; add an alias entry mapping JobTypeKeyRotate to
the same handler as JobTypeSystemKeyRotate (i.e., use
NewSystemKeyRotateJobHandler(repository, cmkAuditor, manager, systemResolver)
for JobTypeKeyRotate) or otherwise ensure outstanding KEY_ROTATE records are
migrated before rollout so GetHandlerByJobType continues to find a handler.
---
Nitpick comments:
In `@internal/event-processor/reconciler_test.go`:
- Around line 469-506: The test suite is missing a negative regression case
ensuring SystemActionJobData enforces KeyIDFrom == KeyIDTo for
JobTypeSystemKeyRotate/TaskType_SYSTEM_KEY_ROTATE; add a test entry in
reconciler_test.go (alongside the existing SYSTEM_KEY_ROTATE cases) that sets
KeyIDFrom to a different key than KeyIDTo and asserts the reconciler
returns/records a validation error or rejects the job, referencing
SystemActionJobData, KeyIDFrom, KeyIDTo and the
JobTypeSystemKeyRotate/TaskType_SYSTEM_KEY_ROTATE identifiers; replicate the
same negative-case addition for the other block mentioned (around lines 671-689)
to lock down the invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51ba4b78-f71e-434d-ae88-2034bced6c69
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/export_test.gointernal/event-processor/factory.gointernal/event-processor/factory_test.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.gointernal/manager/system.gosonar-project.properties
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- internal/event-processor/errors_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/event-processor/export_test.go
- internal/event-processor/types.go
- internal/event-processor/proto/task.proto
- internal/event-processor/factory.go
- internal/manager/system.go
- internal/event-processor/resolvers.go
- internal/event-processor/handlers_system.go
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
3 times, most recently
from
4e48a58 to
3191982
Compare
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
3191982 to
022194a
Compare
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
2 times, most recently
from
7296984 to
37ce3a8
Compare
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
37ce3a8 to
88483c8
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/event-processor/resolvers.go (1)
217-227: Consider creating a copy of cryptoData before mutation.
fetchAndPopulateVersionInfodirectly mutates the map returned bykey.GetCryptoAccessData(). IfGetCryptoAccessData()returns a reference to internal state rather than a copy, this could have unintended side effects on the model.While the key model is freshly loaded and likely discarded after this operation, it's safer to create an explicit copy:
♻️ Suggested refactor
// Merge fresh version info into crypto access data - cryptoData := key.GetCryptoAccessData() + originalCryptoData := key.GetCryptoAccessData() + cryptoData := make(map[string]map[string]any, len(originalCryptoData)) + for region, data := range originalCryptoData { + cryptoData[region] = make(map[string]any, len(data)) + for k, v := range data { + cryptoData[region][k] = v + } + } if freshKeyInfo.LatestKeyVersionId != "" || freshKeyInfo.LatestRotationTime != nil {internal/event-processor/reconciler_test.go (2)
1462-1472: Userequire.NoErrorinstead ofassert.NoErrorin helper that returns a value.The
createJobDatahelper usesassert.NoError(line 1470) which doesn't stop execution on failure. If marshaling fails, the helper returns an invalid byte slice that could cause confusing downstream test failures.🔧 Suggested fix
createJobData := func() []byte { jobData := eventprocessor.SystemActionJobData{ TenantID: tenant, SystemID: system.ID.String(), KeyIDTo: key.ID.String(), KeyIDFrom: key.ID.String(), } dataBytes, err := json.Marshal(jobData) - assert.NoError(t, err) + require.NoError(t, err) return dataBytes }
1504-1509: Consider simplifying the event deletion verification pattern.The repeated pattern for verifying event deletion is verbose:
found, err := r.First(ctx, &model.Event{Identifier: eventID}, *repo.NewQuery()) if err != nil && !errors.Is(err, repo.ErrNotFound) { t.Fatalf("unexpected error checking event: %v", err) } assert.False(t, found)A helper function would reduce duplication:
♻️ Optional refactor
assertEventDeleted := func(t *testing.T, eventID string) { t.Helper() found, err := r.First(ctx, &model.Event{Identifier: eventID}, *repo.NewQuery()) if err != nil && !errors.Is(err, repo.ErrNotFound) { t.Fatalf("unexpected error checking event: %v", err) } assert.False(t, found, "event should be deleted") }Also applies to: 1540-1545, 1599-1604
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b65cf342-116c-4e3a-8c0f-df644d14ba0f
⛔ Files ignored due to path filters (1)
internal/event-processor/proto/task.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
internal/event-processor/errors.gointernal/event-processor/errors_test.gointernal/event-processor/export_test.gointernal/event-processor/factory.gointernal/event-processor/factory_test.gointernal/event-processor/handlers_system.gointernal/event-processor/proto/task.protointernal/event-processor/reconciler.gointernal/event-processor/reconciler_test.gointernal/event-processor/resolvers.gointernal/event-processor/types.gointernal/manager/system.gointernal/pluginregistry/service/api/keymanagement/key_management.gointernal/pluginregistry/service/wrapper/key_management/v1.gointernal/testutils/testplugins/keystore_operator.gosonar-project.properties
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- internal/event-processor/proto/task.proto
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/event-processor/export_test.go
- internal/event-processor/types.go
- internal/event-processor/reconciler.go
- internal/manager/system.go
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
d0cb7e5 to
3bafd60
Compare
doanhluongsap
force-pushed
the
task/refactor_KEY_ROTATE
branch
from
b42757f to
34dba2f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Special notes for your reviewer:
Release note:
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Chores