refactor: convert release id to deterministic uuid#828
refactor: convert release id to deterministic uuid#828adityachoudhari26 merged 6 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR replaces usage of release.ContentHash() with release.Id.String() across the workspace engine, updates persistence/indexing to use UUID-based release IDs, and adds restore-time migrations to backfill deterministic UUIDs for legacy content-hash identifiers. Tests and telemetry are updated accordingly. Changes
Sequence Diagram(s)(Skipped — changes are primarily identifier migration, telemetry, persistence and tests rather than introduction of a new multi-component control flow.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Signed-off-by: Aditya Choudhari <48932219+adityachoudhari26@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go (1)
234-237:⚠️ Potential issue | 🟡 MinorReplace the remaining legacy
ContentHash()release ID in this test.Line 236 still uses
release.ContentHash()while the rest of the file has moved torelease.Id.String(). This inconsistency can hide regressions in the UUID migration path.Suggested fix
job := &oapi.Job{ Id: uuid.New().String(), - ReleaseId: release.ContentHash(), + ReleaseId: release.Id.String(), Status: oapi.JobStatusSuccessful, CreatedAt: time.Now(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go` around lines 234 - 237, The test constructs an oapi.Job using release.ContentHash(), which is legacy; update the job creation to use release.Id.String() for the ReleaseId field so it matches the rest of the file's UUID migration (replace release.ContentHash() with release.Id.String() in the oapi.Job literal where ReleaseId is set, e.g., in the job variable initialization) and ensure the ReleaseId value remains a string compatible with oapi.Job.ReleaseId.apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go (1)
92-95:⚠️ Potential issue | 🟠 Major
release.idtrace attribute still reports content hash instead of UUID.Line [94] sets
"release.id"toreleaseToDeploy.ContentHash(). After this migration, that key should carryreleaseToDeploy.Id.String(); otherwise tracing correlation diverges from persisted/job release IDs.Suggested fix
ctx, span := tracer.Start(ctx, "ExecuteRelease", oteltrace.WithAttributes( - attribute.String("release.id", releaseToDeploy.ContentHash()), + attribute.String("release.id", releaseToDeploy.Id.String()), + attribute.String("release.content_hash", releaseToDeploy.ContentHash()), attribute.String("deployment.id", releaseToDeploy.ReleaseTarget.DeploymentId), attribute.String("environment.id", releaseToDeploy.ReleaseTarget.EnvironmentId), attribute.String("resource.id", releaseToDeploy.ReleaseTarget.ResourceId),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go` around lines 92 - 95, The trace attribute "release.id" in ExecuteRelease is incorrectly using releaseToDeploy.ContentHash(); update the attribute to use the release's UUID string by setting attribute.String("release.id", releaseToDeploy.Id.String()) (locate the tracer.Start call and the attribute list around ExecuteRelease to change ContentHash() to Id.String()).
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go (1)
112-129: Consider extracting repeated system/link setup into a shared test helper.The same setup sequence is repeated in multiple tests; a helper would reduce drift and keep test intent tighter.
♻️ Suggested refactor
+func setupSystemScopedDeploymentAndEnvironment( + t *testing.T, + ctx context.Context, + testStore *store.Store, + systemID string, + deployment *oapi.Deployment, + environment *oapi.Environment, +) { + t.Helper() + testStore.Systems.Upsert(ctx, &oapi.System{ + Id: systemID, + Name: "test-system", + }) + _ = testStore.Deployments.Upsert(ctx, deployment) + testStore.SystemDeployments.Link(systemID, deployment.Id) + _ = testStore.Environments.Upsert(ctx, environment) + testStore.SystemEnvironments.Link(systemID, environment.Id) +}Also applies to: 184-217, 347-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go` around lines 112 - 129, Extract the repeated setup sequence (calls to testStore.Systems.Upsert, createTestJobAgent, testStore.JobAgents.Upsert, createTestDeploymentForExecutor + testStore.Deployments.Upsert, testStore.SystemDeployments.Link, createTestEnvironmentForExecutor + testStore.Environments.Upsert, testStore.SystemEnvironments.Link) into a single test helper (e.g., setupTestSystemWithDeploymentAndEnv) that accepts IDs/names and returns the created entities; replace the duplicated blocks in executor_test.go (and at the other indicated ranges) with calls to this helper to reduce duplication and make tests clearer.apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go (1)
47-50: Extract release ID normalization helper to avoid drift
GetandRemovenow repeat the same UUID-parse/fallback block. Please centralize it into one helper so future migration behavior can’t diverge between read and delete paths.♻️ Suggested refactor
+func normalizeReleaseID(id string) uuid.UUID { + uid, err := uuid.Parse(id) + if err == nil { + return uid + } + return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)) +} + func (r *Repo) Get(id string) (*oapi.Release, bool) { - uid, err := uuid.Parse(id) - if err != nil { - uid = uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)) - } + uid := normalizeReleaseID(id) row, err := db.GetQueries(r.ctx).GetReleaseByID(r.ctx, uid) @@ func (r *Repo) Remove(id string) error { - uid, err := uuid.Parse(id) - if err != nil { - uid = uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)) - } + uid := normalizeReleaseID(id) if err := db.GetQueries(r.ctx).DeleteReleaseVariablesByReleaseID(r.ctx, uid); err != nil {Also applies to: 140-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go` around lines 47 - 50, Create a single helper (e.g., normalizeReleaseID or parseOrSHA1UUID) that encapsulates the uid parsing logic: try uuid.Parse(id) and on error return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)); have it return a uuid.UUID and error only if you want to propagate parse failures. Replace the duplicated blocks inside the Get and Remove methods (and the other occurrence around lines 140-143) to call this helper so both read and delete paths share identical normalization behavior; update callers to use the helper's returned uuid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/workspace/releasemanager/manager_test.go`:
- Line 106: The test uses release.Id.String() (e.g., when calling createTestJob)
but discards errors from the prior release upsert, so add require.NoError(...)
immediately after each release upsert call that populates release (the same
upsert invocations whose result is used for release.Id.String()) to fail fast on
setup errors; update the test lines around release upsert, including the setup
before createTestJob and the other upsert locations referenced (lines where
release is assigned), to call require.NoError(t, err) (or the project’s test
require helper) before using release.Id.String().
In
`@apps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.go`:
- Line 272: The test fixtures call createTestVerification with
release.Id.String() for the jobId parameter, which yields invalid
JobVerification.JobId links; update each affected call (e.g., the call at
createTestVerification(s, ctx, release.Id.String(), 1, 3600)) to pass the real
job id (job.Id.String()) instead of release.Id.String(), ensuring the local job
variable (the Job under test) is used or created before the call so
JobVerification.JobId correctly references the Job; apply this change to all
listed sites that call createTestVerification and verify the job variable name
matches the surrounding test scope.
In `@apps/workspace-engine/pkg/workspace/store/store.go`:
- Around line 474-485: The migration updates job.ReleaseId in-memory and calls
s.repo.Jobs.Set but does not persist the change to the active jobs store when
running in DB-backed mode (WithDBJobs), leaving persisted jobs with legacy
ReleaseId; update the migration so that after computing the new UUID you persist
it through the active jobs repository used for runtime persistence as well
(i.e., in addition to s.repo.Jobs.Set(job) also call the active/DB-backed jobs
persistence method the service uses when WithDBJobs is enabled — the same
persistent setter used elsewhere for active jobs), ensuring you reference
job.ReleaseId and use the same error handling/logging strategy as the existing
s.repo.Jobs.Set call.
---
Outside diff comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.go`:
- Around line 234-237: The test constructs an oapi.Job using
release.ContentHash(), which is legacy; update the job creation to use
release.Id.String() for the ReleaseId field so it matches the rest of the file's
UUID migration (replace release.ContentHash() with release.Id.String() in the
oapi.Job literal where ReleaseId is set, e.g., in the job variable
initialization) and ensure the ReleaseId value remains a string compatible with
oapi.Job.ReleaseId.
In `@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.go`:
- Around line 92-95: The trace attribute "release.id" in ExecuteRelease is
incorrectly using releaseToDeploy.ContentHash(); update the attribute to use the
release's UUID string by setting attribute.String("release.id",
releaseToDeploy.Id.String()) (locate the tracer.Start call and the attribute
list around ExecuteRelease to change ContentHash() to Id.String()).
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.go`:
- Around line 112-129: Extract the repeated setup sequence (calls to
testStore.Systems.Upsert, createTestJobAgent, testStore.JobAgents.Upsert,
createTestDeploymentForExecutor + testStore.Deployments.Upsert,
testStore.SystemDeployments.Link, createTestEnvironmentForExecutor +
testStore.Environments.Upsert, testStore.SystemEnvironments.Link) into a single
test helper (e.g., setupTestSystemWithDeploymentAndEnv) that accepts IDs/names
and returns the created entities; replace the duplicated blocks in
executor_test.go (and at the other indicated ranges) with calls to this helper
to reduce duplication and make tests clearer.
In `@apps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.go`:
- Around line 47-50: Create a single helper (e.g., normalizeReleaseID or
parseOrSHA1UUID) that encapsulates the uid parsing logic: try uuid.Parse(id) and
on error return uuid.NewSHA1(uuid.NameSpaceOID, []byte(id)); have it return a
uuid.UUID and error only if you want to propagate parse failures. Replace the
duplicated blocks inside the Get and Remove methods (and the other occurrence
around lines 140-143) to call this helper so both read and delete paths share
identical normalization behavior; update callers to use the helper's returned
uuid.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (53)
apps/workspace-engine/pkg/oapi/persistence.goapps/workspace-engine/pkg/persistence/integration_test.goapps/workspace-engine/pkg/workspace/jobs/factory.goapps/workspace-engine/pkg/workspace/jobs/factory_test.goapps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator.goapps/workspace-engine/pkg/workspace/releasemanager/action/orchestrator_test.goapps/workspace-engine/pkg/workspace/releasemanager/action/rollback/hooks.goapps/workspace-engine/pkg/workspace/releasemanager/action/rollback/rollback.goapps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification.goapps/workspace-engine/pkg/workspace/releasemanager/action/verification/verification_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/executor_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_retry_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/job_eligibility_test.goapps/workspace-engine/pkg/workspace/releasemanager/deployment/planner.goapps/workspace-engine/pkg/workspace/releasemanager/manager.goapps/workspace-engine/pkg/workspace/releasemanager/manager_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_policy_action.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentdependency/deployment_dependency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentwindow/deploymentwindow_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environment_progression_action.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/environmentprogression_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/passrate_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/soaktime_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/gradualrollout/gradualrollout_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/smart_defaults_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/rollback/rollback_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/version_summary_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/versioncooldown_test.goapps/workspace-engine/pkg/workspace/releasemanager/state_index_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/executor_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/manager_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/recorder_test.goapps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.goapps/workspace-engine/pkg/workspace/store/jobs.goapps/workspace-engine/pkg/workspace/store/release_targets_test.goapps/workspace-engine/pkg/workspace/store/releases.goapps/workspace-engine/pkg/workspace/store/repository/db/releases/repo.goapps/workspace-engine/pkg/workspace/store/repository/memory/indexstore/schema_release.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile.goapps/workspace-engine/test/controllers/pipeline_test.goapps/workspace-engine/test/e2e/engine_job_verification_query_test.goapps/workspace-engine/test/e2e/engine_nil_release_handling_test.goapps/workspace-engine/test/e2e/engine_releasemanager_test.goapps/workspace-engine/test/e2e/engine_releasetarget_state_test.goapps/workspace-engine/test/e2e/engine_verification_hooks_test.go
|
|
||
| // Create a pending job for this release target | ||
| job := createTestJob(release.ContentHash(), oapi.JobStatusPending) | ||
| job := createTestJob(release.Id.String(), oapi.JobStatusPending) |
There was a problem hiding this comment.
Assert release upsert success before using release.Id.String()
Line 106, Lines 269-272, and Line 326 now depend on release IDs populated by the prior release upsert, but those upsert errors are discarded. If setup fails, these tests can proceed with invalid IDs and produce misleading outcomes. Please fail fast with require.NoError(...) on each release upsert.
Suggested test setup fix
- _ = testStore.Releases.Upsert(ctx, release)
+ require.NoError(t, testStore.Releases.Upsert(ctx, release))- _ = testStore.Releases.Upsert(ctx, release)
+ require.NoError(t, testStore.Releases.Upsert(ctx, release))- _ = testStore.Releases.Upsert(ctx, release2)
+ require.NoError(t, testStore.Releases.Upsert(ctx, release2))Also applies to: 269-272, 326-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/releasemanager/manager_test.go` at line
106, The test uses release.Id.String() (e.g., when calling createTestJob) but
discards errors from the prior release upsert, so add require.NoError(...)
immediately after each release upsert call that populates release (the same
upsert invocations whose result is used for release.Id.String()) to fail fast on
setup errors; update the test lines around release upsert, including the setup
before createTestJob and the other upsert locations referenced (lines where
release is assigned), to call require.NoError(t, err) (or the project’s test
require helper) before using release.Id.String().
|
|
||
| release := createTestRelease(s, ctx) | ||
| verification := createTestVerification(s, ctx, release.ContentHash(), 1, 3600) | ||
| verification := createTestVerification(s, ctx, release.Id.String(), 1, 3600) |
There was a problem hiding this comment.
Use real job.Id when creating JobVerification fixtures
At Line 272 (and Lines 303, 333, 353, 388, 408, 455, 475, 711, 735, 755), createTestVerification is being passed release.Id.String(), but that parameter is jobId. This produces invalid JobVerification.JobId linkage in test fixtures.
Suggested fix pattern
- release := createTestRelease(s, ctx)
- verification := createTestVerification(s, ctx, release.Id.String(), 1, 3600)
+ release := createTestRelease(s, ctx)
+ job := createTestJob(s, ctx, release.Id.String())
+ verification := createTestVerification(s, ctx, job.Id, 1, 3600)Apply the same pattern to each affected call site above.
Also applies to: 303-303, 333-333, 353-353, 388-388, 408-408, 455-455, 475-475, 711-711, 735-735, 755-755
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/releasemanager/verification/scheduler_test.go`
at line 272, The test fixtures call createTestVerification with
release.Id.String() for the jobId parameter, which yields invalid
JobVerification.JobId links; update each affected call (e.g., the call at
createTestVerification(s, ctx, release.Id.String(), 1, 3600)) to pass the real
job id (job.Id.String()) instead of release.Id.String(), ensuring the local job
variable (the Job under test) is used or created before the call so
JobVerification.JobId correctly references the Job; apply this change to all
listed sites that call createTestVerification and verify the job variable name
matches the surrounding test scope.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/store/restore_test.go (1)
1086-1107: Strengthen expected UUID assertion to avoid post-restore coupling.Line 1105 derives
expectedUUIDfrom the restored object; precomputing from the pre-restorereleasemakes this test stricter against future regressions in migration inputs.Suggested diff
func TestStore_Restore_ReleaseIdBackfill(t *testing.T) { ctx := context.Background() namespace := "workspace-" + uuid.New().String() persistenceStore := memory.NewStore() @@ release := &oapi.Release{ @@ } + expectedUUID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) changes := persistence.NewChangesBuilder(namespace). Set(release). Build() @@ releases := testStore.Releases.Items() require.Len(t, releases, 1) - for _, r := range releases { - assert.NotEqual(t, uuid.Nil, r.Id, "Release should have a non-nil Id after restore") - expectedUUID := uuid.NewSHA1(uuid.NameSpaceOID, []byte(r.ContentHash())) - assert.Equal(t, expectedUUID, r.Id, "Release Id should be deterministic UUID from content hash") - } + restored := releases[0] + assert.NotEqual(t, uuid.Nil, restored.Id, "Release should have a non-nil Id after restore") + assert.Equal(t, expectedUUID, restored.Id, "Release Id should be deterministic UUID from content hash") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/restore_test.go` around lines 1086 - 1107, The test currently computes expectedUUID from the restored release object which couples the assertion to post-restore behavior; before calling persistenceStore.Save/Load/Restore, compute the deterministic UUID from the original release's content hash (use uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) or equivalent) and store it in a variable, then after testStore.Restore assert that each restored release's Id equals that precomputed expected UUID; update references around persistence.NewChangesBuilder, persistenceStore.Save/Load, store.New and testStore.Restore to use this precomputed expected value for the equality assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/restore_test.go`:
- Around line 1086-1107: The test currently computes expectedUUID from the
restored release object which couples the assertion to post-restore behavior;
before calling persistenceStore.Save/Load/Restore, compute the deterministic
UUID from the original release's content hash (use
uuid.NewSHA1(uuid.NameSpaceOID, []byte(release.ContentHash())) or equivalent)
and store it in a variable, then after testStore.Restore assert that each
restored release's Id equals that precomputed expected UUID; update references
around persistence.NewChangesBuilder, persistenceStore.Save/Load, store.New and
testStore.Restore to use this precomputed expected value for the equality
assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/workspace-engine/pkg/workspace/releasemanager/deployment/executor.goapps/workspace-engine/pkg/workspace/store/restore_test.goapps/workspace-engine/pkg/workspace/store/store.go
Summary by CodeRabbit
Improvements
Tests
Chores