chore: write release targets for db changeset compatibility#688
chore: write release targets for db changeset compatibility#688adityachoudhari26 merged 2 commits intomainfrom
Conversation
WalkthroughAdds handling for ReleaseTarget entities to the changeset application flow, introduces transactional DB helpers to write/delete Changes
Sequence Diagram(s)sequenceDiagram
participant Changeset as Changeset Application
participant Apply as applyChange
participant DBH as ReleaseTarget Handler
participant DB as Database
Changeset->>Apply: apply change containing ReleaseTarget entity
Apply->>Apply: detect entity type == ReleaseTarget
alt ChangeType == Delete
Apply->>DBH: deleteReleaseTarget(ctx, entity, tx)
DBH->>DB: DELETE FROM release_target WHERE resource_id=?, environment_id=?, deployment_id=?
DB-->>DBH: result / error
else ChangeType != Delete
Apply->>DBH: writeReleaseTarget(ctx, entity, tx)
DBH->>DB: INSERT INTO release_target (...) ON CONFLICT DO NOTHING
DB-->>DBH: result / error
end
DBH-->>Apply: return error or nil
Apply-->>Changeset: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🔇 Additional comments (2)
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/workspace-engine/pkg/db/changeset.go(1 hunks)apps/workspace-engine/pkg/db/release_targets.go(1 hunks)apps/workspace-engine/pkg/db/release_targets_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/db/release_targets_test.goapps/workspace-engine/pkg/db/changeset.goapps/workspace-engine/pkg/db/release_targets.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/db/release_targets_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/db/release_targets_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
ResourceId(466-466)EnvironmentId(457-457)DeploymentId(448-448)
apps/workspace-engine/pkg/db/changeset.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
ReleaseTarget(351-355)apps/workspace-engine/pkg/changeset/changeset.go (1)
ChangeTypeDelete(13-13)
apps/workspace-engine/pkg/db/release_targets.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
ResourceId(466-466)EnvironmentId(457-457)DeploymentId(448-448)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
🔇 Additional comments (6)
apps/workspace-engine/pkg/db/changeset.go (1)
139-144: LGTM!The ReleaseTarget handling follows the exact same pattern as the other entity types and integrates cleanly into the changeset application flow.
apps/workspace-engine/pkg/db/release_targets.go (1)
10-30: LGTM!The implementation is clean and idempotent by design:
- INSERT with ON CONFLICT DO NOTHING ensures safe repeated writes
- DELETE with composite key filtering is correct
- Parameter ordering matches query placeholders
apps/workspace-engine/pkg/db/release_targets_test.go (4)
12-70: LGTM!The test correctly verifies the write (created by prerequisites) and delete operations using count queries before and after deletion.
74-169: LGTM!The test properly validates multiple write/delete cycles by first cleaning up the prerequisite-created target, then iterating through 3 cycles with count verification after each operation.
173-244: LGTM!The test correctly validates that ON CONFLICT DO NOTHING makes writes idempotent by writing the same target multiple times within a transaction and verifying only one record exists.
286-431: LGTM!The test thoroughly validates handling of multiple different release targets, including creation, coexistence verification, selective deletion, and verification that the correct target remains.
| func TestReleaseTarget_DeleteNonexistent(t *testing.T) { | ||
| workspaceID, conn := setupTestWithWorkspace(t) | ||
| ctx := t.Context() | ||
|
|
||
| // Create prerequisites | ||
| systemID, deploymentID, _, resourceID, environmentID := createReleasePrerequisites( | ||
| t, workspaceID, conn) | ||
|
|
||
| releaseTarget := &oapi.ReleaseTarget{ | ||
| ResourceId: resourceID, | ||
| EnvironmentId: environmentID, | ||
| DeploymentId: deploymentID, | ||
| } | ||
|
|
||
| // Delete a release target that was never created | ||
| tx, err := conn.Begin(ctx) | ||
| if err != nil { | ||
| t.Fatalf("failed to begin tx: %v", err) | ||
| } | ||
| defer tx.Rollback(ctx) | ||
|
|
||
| // This should not error, just delete 0 rows | ||
| err = deleteReleaseTarget(ctx, releaseTarget, tx) | ||
| if err != nil { | ||
| t.Fatalf("expected no error deleting non-existent release target, got: %v", err) | ||
| } | ||
|
|
||
| err = tx.Commit(ctx) | ||
| if err != nil { | ||
| t.Fatalf("failed to commit tx: %v", err) | ||
| } | ||
|
|
||
| // Keep systemID to avoid "declared but not used" error | ||
| _ = systemID | ||
| } |
There was a problem hiding this comment.
Test is not validating deletion of non-existent target.
The test creates prerequisites (which includes a release_target), then immediately tries to delete it using the same IDs. This deletes the existing target rather than a non-existent one. To properly test idempotency of deleting a non-existent target, you should either:
- Delete the prerequisite-created target first, then delete it again, OR
- Use different IDs that don't exist in the database
Apply this diff to fix option 1 (delete twice):
releaseTarget := &oapi.ReleaseTarget{
ResourceId: resourceID,
EnvironmentId: environmentID,
DeploymentId: deploymentID,
}
+ // First delete the release target created by createReleasePrerequisites
+ tx, err := conn.Begin(ctx)
+ if err != nil {
+ t.Fatalf("failed to begin first delete tx: %v", err)
+ }
+
+ err = deleteReleaseTarget(ctx, releaseTarget, tx)
+ if err != nil {
+ tx.Rollback(ctx)
+ t.Fatalf("failed to delete prerequisite release target: %v", err)
+ }
+
+ err = tx.Commit(ctx)
+ if err != nil {
+ t.Fatalf("failed to commit first delete tx: %v", err)
+ }
+
- // Delete a release target that was never created
+ // Now delete the same target again (non-existent)
- tx, err := conn.Begin(ctx)
+ tx, err = conn.Begin(ctx)
if err != nil {
t.Fatalf("failed to begin tx: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestReleaseTarget_DeleteNonexistent(t *testing.T) { | |
| workspaceID, conn := setupTestWithWorkspace(t) | |
| ctx := t.Context() | |
| // Create prerequisites | |
| systemID, deploymentID, _, resourceID, environmentID := createReleasePrerequisites( | |
| t, workspaceID, conn) | |
| releaseTarget := &oapi.ReleaseTarget{ | |
| ResourceId: resourceID, | |
| EnvironmentId: environmentID, | |
| DeploymentId: deploymentID, | |
| } | |
| // Delete a release target that was never created | |
| tx, err := conn.Begin(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to begin tx: %v", err) | |
| } | |
| defer tx.Rollback(ctx) | |
| // This should not error, just delete 0 rows | |
| err = deleteReleaseTarget(ctx, releaseTarget, tx) | |
| if err != nil { | |
| t.Fatalf("expected no error deleting non-existent release target, got: %v", err) | |
| } | |
| err = tx.Commit(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to commit tx: %v", err) | |
| } | |
| // Keep systemID to avoid "declared but not used" error | |
| _ = systemID | |
| } | |
| func TestReleaseTarget_DeleteNonexistent(t *testing.T) { | |
| workspaceID, conn := setupTestWithWorkspace(t) | |
| ctx := t.Context() | |
| // Create prerequisites | |
| systemID, deploymentID, _, resourceID, environmentID := createReleasePrerequisites( | |
| t, workspaceID, conn) | |
| releaseTarget := &oapi.ReleaseTarget{ | |
| ResourceId: resourceID, | |
| EnvironmentId: environmentID, | |
| DeploymentId: deploymentID, | |
| } | |
| // First delete the release target created by createReleasePrerequisites | |
| tx, err := conn.Begin(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to begin first delete tx: %v", err) | |
| } | |
| err = deleteReleaseTarget(ctx, releaseTarget, tx) | |
| if err != nil { | |
| tx.Rollback(ctx) | |
| t.Fatalf("failed to delete prerequisite release target: %v", err) | |
| } | |
| err = tx.Commit(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to commit first delete tx: %v", err) | |
| } | |
| // Now delete the same target again (non-existent) | |
| tx, err = conn.Begin(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to begin tx: %v", err) | |
| } | |
| defer tx.Rollback(ctx) | |
| // This should not error, just delete 0 rows | |
| err = deleteReleaseTarget(ctx, releaseTarget, tx) | |
| if err != nil { | |
| t.Fatalf("expected no error deleting non-existent release target, got: %v", err) | |
| } | |
| err = tx.Commit(ctx) | |
| if err != nil { | |
| t.Fatalf("failed to commit tx: %v", err) | |
| } | |
| // Keep systemID to avoid "declared but not used" error | |
| _ = systemID | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/db/release_targets_test.go around lines 248 to 282,
the test intends to verify deleting a non-existent release target but currently
deletes an actually existing target because prerequisites include a created
release_target; update the test to delete the created target once and then
attempt to delete it again to validate idempotency: after committing the first
successful delete (or before commit if using the same tx), perform a second call
to deleteReleaseTarget with the same releaseTarget and assert no error and that
nothing breaks (i.e., delete twice), ensuring the test cleans up and keeps
systemID unused to avoid warnings.
📊 Code Coverage Reportworkspace-engine coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
adityachoudhari26
deleted the
write-release-targets-for-db-compatibility
branch
1 participant
Summary by CodeRabbit
New Features
Tests
Chores