chore: init db changeset #675
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughFlushChangeset now locks the ChangeSet early and dispatches each Change by EntityType to per-entity apply handlers that convert entities to OAPI types and call transaction-scoped write/delete helpers for resources, deployments, environments, policies, relationship rules, job agents, systems, deployment variables, and deployment versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CS as ChangeSet
participant Flush as FlushChangeset
participant Conv as OAPI Converters
participant Tx as pgx.Tx
participant DB as DB helpers
CS->>Flush: FlushChangeset(ctx)
Flush->>Tx: begin transaction
loop for each Change (mutex held)
Flush->>Flush: switch on Change.EntityType
alt Change.Type == Delete
Flush->>DB: deleteX(ctx, Change.ID, tx)
DB-->>Flush: ok / error
else Insert/Upsert
Flush->>Conv: convert Change.Entity -> *oapi.X
Conv-->>Flush: *oapi.X / error
Flush->>DB: writeX(ctx, *oapi.X, tx)
DB-->>Flush: ok / error
end
end
Flush->>Tx: commit or rollback on error
Flush-->>CS: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
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 (4)
apps/workspace-engine/pkg/db/changeset.go(3 hunks)apps/workspace-engine/pkg/db/resources.go(2 hunks)apps/workspace-engine/pkg/oapi/type_conversion.go(1 hunks)apps/workspace-engine/pkg/workspace/changeset/changeset.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/changeset.goapps/workspace-engine/pkg/oapi/type_conversion.goapps/workspace-engine/pkg/workspace/changeset/changeset.goapps/workspace-engine/pkg/db/resources.go
🧬 Code graph analysis (4)
apps/workspace-engine/pkg/db/changeset.go (3)
apps/workspace-engine/pkg/db/client.go (1)
GetDB(60-70)apps/workspace-engine/pkg/workspace/changeset/changeset.go (3)
Change(33-39)EntityType(16-16)EntityTypeResource(19-19)apps/workspace-engine/pkg/oapi/type_conversion.go (1)
ConvertToOapiResource(5-11)
apps/workspace-engine/pkg/oapi/type_conversion.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Release(246-252)RelationshipRule(231-243)PolicyTargetSelector(207-212)UserApprovalRecord(316-322)
apps/workspace-engine/pkg/workspace/changeset/changeset.go (1)
apps/workspace-engine/pkg/workspace/relationships/match.go (1)
Entity(7-11)
apps/workspace-engine/pkg/db/resources.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Resource(262-276)
⏰ 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: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/db/changeset.go (1)
18-23: Race condition: Protect ChangeSet slice access with the mutex.
len(cs.Changes)is read without holdingcs.Mutex, allowing concurrentRecord()calls to race and potentially corrupt the slice.Apply this fix:
- if len(cs.Changes) == 0 { - return nil - } - cs.Mutex.Lock() defer cs.Mutex.Unlock() + + if len(cs.Changes) == 0 { + return nil + }
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/db/changeset.go (2)
96-112: Consider using a switch statement for cleaner entity type routing.The multiple if statements can be replaced with a switch for improved readability.
- if change.EntityType == changeset.EntityTypeResource { - return applyResourceChange(ctx, conn, change) - } - - if change.EntityType == changeset.EntityTypeDeployment { - return applyDeploymentChange(ctx, conn, change) - } - - if change.EntityType == changeset.EntityTypeEnvironment { - return applyEnvironmentChange(ctx, conn, change) - } - - if change.EntityType == changeset.EntityTypePolicy { - return applyPolicyChange(ctx, conn, change) - } - - return fmt.Errorf("unknown entity type: %s", change.EntityType) + switch change.EntityType { + case changeset.EntityTypeResource: + return applyResourceChange(ctx, conn, change) + case changeset.EntityTypeDeployment: + return applyDeploymentChange(ctx, conn, change) + case changeset.EntityTypeEnvironment: + return applyEnvironmentChange(ctx, conn, change) + case changeset.EntityTypePolicy: + return applyPolicyChange(ctx, conn, change) + default: + return fmt.Errorf("unknown entity type: %s", change.EntityType) + }
12-12: Add godoc comment for exported function.Per coding guidelines, exported functions should be documented with godoc comments explaining their purpose and behavior.
+// FlushChangeset persists all recorded changes in the context's ChangeSet to the database +// within a single transaction, then clears the change buffer. Returns an error if the +// database transaction fails. func FlushChangeset(ctx context.Context) error {Based on coding guidelines
apps/workspace-engine/pkg/workspace/changeset/changeset.go (2)
35-53: Add godoc comments for exported types.Per coding guidelines, exported types should be documented with godoc comments explaining their purpose and usage.
+// ChangeType represents the kind of modification made to an entity. type ChangeType string const ( ChangeTypeInsert ChangeType = "insert" ChangeTypeUpdate ChangeType = "update" ChangeTypeDelete ChangeType = "delete" ) +// EntityType represents the category of entity being tracked in a change. type EntityType string const ( EntityTypeResource EntityType = "resource" ... ) +// Change represents a single modification to an entity, including its type, +// the operation performed, and a timestamp. type Change struct { EntityType EntityType Type ChangeType ID string Entity any Timestamp time.Time } +// ChangeSet tracks a collection of changes with thread-safe recording via a mutex. +// IsInitialLoad indicates whether this changeset represents an initial data load. type ChangeSet struct { IsInitialLoad bool Changes []Change Mutex sync.Mutex } +// NewChangeSet creates a new empty ChangeSet ready to record changes. func NewChangeSet() *ChangeSet {Based on coding guidelines
55-66: LGTM! Consider adding godoc comment.The Record method correctly locks the mutex and safely appends changes.
Consider adding godoc per coding guidelines:
+// Record safely appends a new change to the ChangeSet, locking the mutex to prevent +// concurrent modification races. func (cs *ChangeSet) Record(entityType EntityType, changeType ChangeType, id string, entity any) {Based on coding guidelines
📜 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 (7)
apps/workspace-engine/pkg/db/changeset.go(3 hunks)apps/workspace-engine/pkg/db/deployments.go(2 hunks)apps/workspace-engine/pkg/db/environments.go(2 hunks)apps/workspace-engine/pkg/db/policies.go(2 hunks)apps/workspace-engine/pkg/db/resources.go(2 hunks)apps/workspace-engine/pkg/events/handler/handler.go(1 hunks)apps/workspace-engine/pkg/workspace/changeset/changeset.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/environments.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/db/policies.goapps/workspace-engine/pkg/db/resources.goapps/workspace-engine/pkg/db/changeset.goapps/workspace-engine/pkg/workspace/changeset/changeset.goapps/workspace-engine/pkg/db/deployments.go
🧬 Code graph analysis (7)
apps/workspace-engine/pkg/db/environments.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Environment(120-127)
apps/workspace-engine/pkg/events/handler/handler.go (2)
apps/workspace-engine/pkg/workspace/changeset/changeset.go (2)
EntityTypeReleaseTarget(22-22)ChangeTypeInsert(11-11)apps/workspace-engine/pkg/workspace/releasemanager/manager.go (1)
Changes(48-52)
apps/workspace-engine/pkg/db/policies.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Policy(188-196)PolicyTargetSelector(207-212)PolicyRule(199-204)AnyApprovalRule(62-64)
apps/workspace-engine/pkg/db/resources.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Resource(262-276)
apps/workspace-engine/pkg/db/changeset.go (3)
apps/workspace-engine/pkg/db/client.go (1)
GetDB(60-70)apps/workspace-engine/pkg/workspace/changeset/changeset.go (7)
Change(35-41)ChangeTypeDelete(13-13)EntityType(16-16)EntityTypeResource(19-19)EntityTypeDeployment(20-20)EntityTypeEnvironment(21-21)EntityTypePolicy(32-32)apps/workspace-engine/pkg/oapi/type_conversion.go (4)
ConvertToOapiResource(5-11)ConvertToOapiDeployment(13-19)ConvertToOapiEnvironment(21-27)ConvertToOapiPolicy(61-67)
apps/workspace-engine/pkg/workspace/changeset/changeset.go (2)
apps/workspace-engine/pkg/workspace/relationships/match.go (1)
Entity(7-11)apps/workspace-engine/pkg/workspace/releasemanager/manager.go (1)
Changes(48-52)
apps/workspace-engine/pkg/db/deployments.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Deployment(73-82)
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/workspace-engine/pkg/db/changeset.go (2)
51-60: LGTM!The conversion and routing logic correctly handles both delete and write operations.
62-93: LGTM!All entity-specific apply functions follow a consistent pattern with appropriate type conversion and delete/write routing.
apps/workspace-engine/pkg/workspace/changeset/changeset.go (1)
16-33: LGTM!The EntityType definition and constants provide good type safety and are consistently named.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/db/relationships.go (1)
247-259: Populate RelationshipRule.workspaceId from DBThe API now requires
workspaceId, but we never read it from the database nor set it on the returned struct, so callers always get an empty string. Includer.workspace_idin the SELECT/scan and assign it here.Consider this fix:
-const RELATIONSHIP_SELECT_QUERY = ` +const RELATIONSHIP_SELECT_QUERY = ` SELECT r.id, r.name, r.reference, r.dependency_type, r.description, + r.workspace_id, r.source_kind,type dbRelationshipRule struct { ID string `db:"id"` Name *string `db:"name"` Reference string `db:"reference"` DependencyType string `db:"dependency_type"` Description *string `db:"description"` + WorkspaceID string `db:"workspace_id"`FromSelector: fromSelector, ToSelector: toSelector, PropertyMatchers: propertyMatchers, FromType: "resource", ToType: "resource", RelationshipType: dbRelationship.DependencyType, Metadata: map[string]string{}, Reference: dbRelationship.Reference, + WorkspaceId: dbRelationship.WorkspaceID,&dbRelationship.DependencyType, &dbRelationship.Description, + &dbRelationship.WorkspaceID,
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/db/changeset.go (1)
18-24: Guard ChangeSet slice with the mutex before inspection
cs.Changesis read while unlocked, so concurrentRecordcalls can race and corrupt the slice. Lock before touching it, then release after clearing.Apply this diff:
- if len(cs.Changes) == 0 { - return nil - } - - cs.Mutex.Lock() - defer cs.Mutex.Unlock() + cs.Mutex.Lock() + defer cs.Mutex.Unlock() + + if len(cs.Changes) == 0 { + return nil + }
📜 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 (4)
apps/workspace-engine/oapi/spec.yaml(2 hunks)apps/workspace-engine/pkg/db/changeset.go(3 hunks)apps/workspace-engine/pkg/db/relationships.go(2 hunks)apps/workspace-engine/pkg/oapi/oapi.gen.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/workspace-engine/oapi/spec.yaml
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/relationships.goapps/workspace-engine/pkg/db/changeset.goapps/workspace-engine/pkg/oapi/oapi.gen.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/db/relationships.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
RelationshipRule(231-244)Selector(296-298)Value(326-328)PropertyMatcher(215-219)
apps/workspace-engine/pkg/db/changeset.go (3)
apps/workspace-engine/pkg/db/client.go (1)
GetDB(60-70)apps/workspace-engine/pkg/workspace/changeset/changeset.go (8)
Change(35-41)ChangeTypeDelete(13-13)EntityType(16-16)EntityTypeResource(19-19)EntityTypeDeployment(20-20)EntityTypeEnvironment(21-21)EntityTypePolicy(32-32)EntityTypeResourceRelationshipRule(31-31)apps/workspace-engine/pkg/oapi/type_conversion.go (5)
ConvertToOapiResource(5-11)ConvertToOapiDeployment(13-19)ConvertToOapiEnvironment(21-27)ConvertToOapiPolicy(61-67)ConvertToOapiRelationshipRule(53-59)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/db/changeset.go (1)
18-24: Lock before touchingcs.Changes.We’re still reading
len(cs.Changes)without holding the mutex. ConcurrentRecordcalls can resize the slice while Flush runs, leading to races and corruption. Take the lock first, then inspect/clear the slice.- if len(cs.Changes) == 0 { - return nil - } - - cs.Mutex.Lock() - defer cs.Mutex.Unlock() + cs.Mutex.Lock() + defer cs.Mutex.Unlock() + + if len(cs.Changes) == 0 { + return nil + }
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/oapi/type_conversion.go (1)
85-92: Remove extra blank line.func ConvertToOapiUserApprovalRecord(entity any) (*UserApprovalRecord, error) { userApprovalRecord, ok := entity.(*UserApprovalRecord) if !ok { return nil, fmt.Errorf("entity is not a user approval record") } - return userApprovalRecord, nil }
📜 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 (7)
apps/workspace-engine/pkg/db/changeset.go(3 hunks)apps/workspace-engine/pkg/db/deployment_variables.go(1 hunks)apps/workspace-engine/pkg/db/deployment_versions.go(1 hunks)apps/workspace-engine/pkg/db/job_agents.go(1 hunks)apps/workspace-engine/pkg/db/systems.go(1 hunks)apps/workspace-engine/pkg/oapi/type_conversion.go(1 hunks)apps/workspace-engine/pkg/workspace/changeset/changeset.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/job_agents.goapps/workspace-engine/pkg/workspace/changeset/changeset.goapps/workspace-engine/pkg/db/systems.goapps/workspace-engine/pkg/db/deployment_variables.goapps/workspace-engine/pkg/db/deployment_versions.goapps/workspace-engine/pkg/db/changeset.goapps/workspace-engine/pkg/oapi/type_conversion.go
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:49:05.086Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#655
File: apps/workspace-engine/pkg/engine/workspace/fluent.go:166-171
Timestamp: 2025-08-12T20:49:05.086Z
Learning: The UpdateDeploymentVersions() method with OperationCreate case in apps/workspace-engine/pkg/engine/workspace/fluent.go is specifically designed only for creating new deployment versions, not for handling potential duplicates or existing versions.
Applied to files:
apps/workspace-engine/pkg/db/deployment_versions.go
🧬 Code graph analysis (6)
apps/workspace-engine/pkg/db/job_agents.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
JobAgent(150-156)
apps/workspace-engine/pkg/db/systems.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
System(309-314)
apps/workspace-engine/pkg/db/deployment_variables.go (2)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
DeploymentVariable(85-92)packages/db/src/common.ts (1)
Tx(22-22)
apps/workspace-engine/pkg/db/deployment_versions.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
DeploymentVersion(104-114)
apps/workspace-engine/pkg/db/changeset.go (3)
apps/workspace-engine/pkg/db/client.go (1)
GetDB(60-70)apps/workspace-engine/pkg/workspace/changeset/changeset.go (12)
Change(36-42)ChangeTypeDelete(13-13)EntityType(16-16)EntityTypeResource(19-19)EntityTypeDeployment(20-20)EntityTypeEnvironment(21-21)EntityTypePolicy(33-33)EntityTypeResourceRelationshipRule(32-32)EntityTypeJobAgent(24-24)EntityTypeSystem(29-29)EntityTypeDeploymentVariable(26-26)EntityTypeDeploymentVersion(27-27)apps/workspace-engine/pkg/oapi/type_conversion.go (9)
ConvertToOapiResource(5-11)ConvertToOapiDeployment(13-19)ConvertToOapiEnvironment(21-27)ConvertToOapiPolicy(61-67)ConvertToOapiRelationshipRule(53-59)ConvertToOapiJobAgent(37-43)ConvertToOapiSystem(94-100)ConvertToOapiDeploymentVariable(102-108)ConvertToOapiDeploymentVersion(110-116)
apps/workspace-engine/pkg/oapi/type_conversion.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Release(247-253)RelationshipRule(231-244)PolicyTargetSelector(207-212)UserApprovalRecord(317-323)
🔇 Additional comments (1)
apps/workspace-engine/pkg/oapi/type_conversion.go (1)
5-116: LGTM! Consistent type-safe conversion pattern.The implementation is correct and follows a uniform pattern across all 14 conversion functions. Type assertions to pointer types are appropriate, and error handling is consistent.
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(4 hunks)apps/workspace-engine/pkg/db/policies.go(2 hunks)apps/workspace-engine/pkg/db/relationships.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/policies.goapps/workspace-engine/pkg/db/relationships.goapps/workspace-engine/pkg/db/changeset.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/db/policies.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Policy(198-206)PolicyTargetSelector(217-222)PolicyRule(209-214)AnyApprovalRule(62-64)
apps/workspace-engine/pkg/db/relationships.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
RelationshipRule(246-260)Selector(317-319)PropertyMatcher(230-234)
apps/workspace-engine/pkg/db/changeset.go (2)
apps/workspace-engine/pkg/workspace/changeset/changeset.go (12)
Change(36-42)ChangeTypeDelete(13-13)EntityType(16-16)EntityTypeResource(19-19)EntityTypeDeployment(20-20)EntityTypeEnvironment(21-21)EntityTypePolicy(33-33)EntityTypeResourceRelationshipRule(32-32)EntityTypeJobAgent(24-24)EntityTypeSystem(29-29)EntityTypeDeploymentVariable(26-26)EntityTypeDeploymentVersion(27-27)apps/workspace-engine/pkg/oapi/type_conversion.go (9)
ConvertToOapiResource(5-11)ConvertToOapiDeployment(13-19)ConvertToOapiEnvironment(21-27)ConvertToOapiPolicy(61-67)ConvertToOapiRelationshipRule(53-59)ConvertToOapiJobAgent(37-43)ConvertToOapiSystem(94-100)ConvertToOapiDeploymentVariable(102-108)ConvertToOapiDeploymentVersion(110-116)
⏰ 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). (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/workspace-engine/pkg/db/relationships.go (1)
431-481: Add error return to prevent silent data loss.Same issue as
extractFromSelector: errors and type assertion failures are silently ignored, causing incomplete rule persistence.Apply the same fix as for
extractFromSelector, changing the signature to return an error and propagating errors instead of silently returning default values.
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/db/relationships.go (2)
483-512: Change ON CONFLICT DO NOTHING to DO UPDATE for metadata matches.The query uses
ON CONFLICT DO NOTHING(line 508), which means if a metadata match already exists with the same rule ID and keys, updates are silently ignored. While the current schema only stores source/target keys (no operator field to update), this pattern is inconsistent withwriteManySourceMetadataEqualsandwriteManyTargetMetadataEqualswhich useDO UPDATE. For consistency and future-proofing (if additional match fields are added), consider usingDO UPDATEor explicitly documenting whyDO NOTHINGis correct here.If the schema truly has no updateable fields beyond the composite key:
query := "INSERT INTO resource_relationship_rule_metadata_match (resource_relationship_rule_id, source_key, target_key) VALUES " + strings.Join(valueStrings, ", ") + - " ON CONFLICT (resource_relationship_rule_id, source_key, target_key) DO NOTHING" + " ON CONFLICT (resource_relationship_rule_id, source_key, target_key) DO NOTHING" // No updateable fields beyond composite keyOr if the operator should be stored/updated in the future, use
DO UPDATEnow:query := "INSERT INTO resource_relationship_rule_metadata_match (resource_relationship_rule_id, source_key, target_key) VALUES " + strings.Join(valueStrings, ", ") + - " ON CONFLICT (resource_relationship_rule_id, source_key, target_key) DO NOTHING" + " ON CONFLICT (resource_relationship_rule_id, source_key, target_key) DO UPDATE SET source_key = EXCLUDED.source_key, target_key = EXCLUDED.target_key"
379-481: Consider consistent return types for selector extractors.
extractFromSelectorreturns(kind string, version string, ...)whileextractToSelectorreturns(kind *string, version *string, ...). This inconsistency stems from the DB schema (source fields are NOT NULL, target fields are nullable), but it creates confusion at the call site where one returns concrete strings and the other returns pointers.For clarity, consider either:
- Making both return pointers and converting in the caller
- Adding doc comments explaining why they differ
- Using the same return types and converting nil to empty string internally
Example doc comments:
// extractFromSelector returns source selector fields as concrete strings // because source_kind and source_version are NOT NULL in the DB schema. func extractFromSelector(selector *oapi.Selector) (kind string, version string, ...) {// extractToSelector returns target selector fields as pointers // because target_kind and target_version are nullable in the DB schema. func extractToSelector(selector *oapi.Selector) (kind *string, version *string, ...) {apps/workspace-engine/pkg/db/resources.go (1)
206-231: Consider simplifying parameter placeholder generation.The current approach using
fmt.Sprintffor each parameter index is correct and safe (parameterized queries prevent SQL injection). However, you could simplify the placeholder string construction slightly.Consider this alternative for generating placeholders:
func writeManyMetadata(ctx context.Context, resourceId string, metadata map[string]string, tx pgx.Tx) error { if len(metadata) == 0 { return nil } valueStrings := make([]string, 0, len(metadata)) valueArgs := make([]interface{}, 0, len(metadata)*3) i := 1 for k, v := range metadata { - valueStrings = append(valueStrings, - "($"+fmt.Sprintf("%d", i)+", $"+fmt.Sprintf("%d", i+1)+", $"+fmt.Sprintf("%d", i+2)+")", - ) + valueStrings = append(valueStrings, fmt.Sprintf("($%d, $%d, $%d)", i, i+1, i+2)) valueArgs = append(valueArgs, resourceId, k, v) i += 3 } query := "INSERT INTO resource_metadata (resource_id, key, value) VALUES " + strings.Join(valueStrings, ", ") + " ON CONFLICT (resource_id, key) DO UPDATE SET value = EXCLUDED.value" _, err := tx.Exec(ctx, query, valueArgs...) if err != nil { return err } return nil }
📜 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 (10)
apps/workspace-engine/pkg/db/deployment_variables.go(1 hunks)apps/workspace-engine/pkg/db/deployment_versions.go(1 hunks)apps/workspace-engine/pkg/db/deployments.go(2 hunks)apps/workspace-engine/pkg/db/environments.go(2 hunks)apps/workspace-engine/pkg/db/job_agents.go(1 hunks)apps/workspace-engine/pkg/db/policies.go(2 hunks)apps/workspace-engine/pkg/db/relationships.go(2 hunks)apps/workspace-engine/pkg/db/resources.go(2 hunks)apps/workspace-engine/pkg/db/systems.go(1 hunks)apps/workspace-engine/pkg/oapi/type_conversion.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/workspace-engine/pkg/db/job_agents.go
- apps/workspace-engine/pkg/db/systems.go
- apps/workspace-engine/pkg/db/deployments.go
- apps/workspace-engine/pkg/oapi/type_conversion.go
🧰 Additional context used
📓 Path-based instructions (1)
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/deployment_versions.goapps/workspace-engine/pkg/db/deployment_variables.goapps/workspace-engine/pkg/db/resources.goapps/workspace-engine/pkg/db/relationships.goapps/workspace-engine/pkg/db/policies.goapps/workspace-engine/pkg/db/environments.go
🧠 Learnings (1)
📚 Learning: 2025-08-12T20:49:05.086Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#655
File: apps/workspace-engine/pkg/engine/workspace/fluent.go:166-171
Timestamp: 2025-08-12T20:49:05.086Z
Learning: The UpdateDeploymentVersions() method with OperationCreate case in apps/workspace-engine/pkg/engine/workspace/fluent.go is specifically designed only for creating new deployment versions, not for handling potential duplicates or existing versions.
Applied to files:
apps/workspace-engine/pkg/db/deployment_versions.go
🧬 Code graph analysis (6)
apps/workspace-engine/pkg/db/deployment_versions.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
DeploymentVersion(114-124)
apps/workspace-engine/pkg/db/deployment_variables.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
DeploymentVariable(95-102)
apps/workspace-engine/pkg/db/resources.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Resource(284-298)
apps/workspace-engine/pkg/db/relationships.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
RelationshipRule(246-260)Selector(317-319)PropertyMatcher(230-234)
apps/workspace-engine/pkg/db/policies.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Policy(198-206)PolicyTargetSelector(217-222)PolicyRule(209-214)AnyApprovalRule(62-64)
apps/workspace-engine/pkg/db/environments.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Environment(130-137)
⏰ 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: Typecheck
- GitHub Check: Lint
🔇 Additional comments (12)
apps/workspace-engine/pkg/db/deployment_versions.go (2)
109-120: UPSERT now in place, but verify deployment_id mutability.The UPSERT correctly addresses the previous concern about status updates causing duplicate-key errors. However, line 117 allows
deployment_idto be updated on conflict. Once a deployment version is created for a specific deployment, allowing it to be reassigned to a different deployment could violate referential integrity assumptions and cause data consistency issues.If reassigning deployment versions between deployments is intentional (e.g., for restructuring operations in the changeset system), please confirm this is the expected behavior. Otherwise, consider removing
deployment_idfrom the UPDATE SET clause:ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name, tag = EXCLUDED.tag, config = EXCLUDED.config, job_agent_config = EXCLUDED.job_agent_config, - deployment_id = EXCLUDED.deployment_id, status = EXCLUDED.status, message = EXCLUDED.message
140-149: LGTM!The delete functionality follows the established transactional pattern and correctly delegates the operation to the transaction scope.
apps/workspace-engine/pkg/db/deployment_variables.go (3)
66-73: Past review comment addressed.The upsert query now correctly handles conflicts by updating mutable fields, making the operation idempotent as requested in the previous review.
89-98: LGTM!The delete operation is correctly implemented as a transaction-scoped function with appropriate error handling.
75-87: default_value and variable_id are not part of the deployment_variable table schema
Omitting these fields inwriteDeploymentVariableis correct since the table only definesid,key,description,deployment_id, andschema.Likely an incorrect or invalid review comment.
apps/workspace-engine/pkg/db/environments.go (4)
9-9: LGTM!The pgx v5 import is correct and necessary for transaction support via
pgx.Tx.
72-85: LGTM!The function correctly executes the UPSERT within the transaction scope and handles pointer fields (
Description,ResourceSelector) appropriately—nil pointers will translate to SQL NULL for optional columns.
87-96: LGTM!Both the DELETE query and
deleteEnvironmentfunction are correctly implemented. The function properly executes the deletion within the transaction scope.
62-70: LGTM – verify schema alignment: confirm the environment table has a primary key on id, a foreign key from system_id to system(id), and columns id, name, system_id, description, resource_selector, created_at.apps/workspace-engine/pkg/db/resources.go (3)
7-8: LGTM!The new imports are appropriately used for dynamic query building in
writeManyMetadata.
233-242: LGTM!The delete operation is correctly implemented with proper transaction usage and parameterized queries.
177-204: Verify timestamp column typesEnsure the
resourcetable’screated_at,locked_at,updated_at, anddeleted_atcolumns are defined astimestamp/timestamptzand accept the RFC3339 strings passed by pgx.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/db/relationships.go (1)
503-532: Inconsistent ON CONFLICT clauses in batch inserts.
writeManyMetadataMatchesusesON CONFLICT ... DO NOTHING(line 528)writeManySourceMetadataEqualsusesON CONFLICT ... DO UPDATE(line 551)writeManyTargetMetadataEqualsusesON CONFLICT ... DO UPDATE(line 574)Since old metadata is deleted first (lines 354-365), all three should use
DO NOTHINGfor consistency and clarity, as conflicts shouldn't occur after a fresh start.Apply this diff to align the conflict handling:
query := "INSERT INTO resource_relationship_rule_source_metadata_equals (resource_relationship_rule_id, key, value) VALUES " + strings.Join(valueStrings, ", ") + - " ON CONFLICT (resource_relationship_rule_id, key) DO UPDATE SET value = EXCLUDED.value" + " ON CONFLICT (resource_relationship_rule_id, key) DO NOTHING" _, err := tx.Exec(ctx, query, valueArgs...)query := "INSERT INTO resource_relationship_rule_target_metadata_equals (resource_relationship_rule_id, key, value) VALUES " + strings.Join(valueStrings, ", ") + - " ON CONFLICT (resource_relationship_rule_id, key) DO UPDATE SET value = EXCLUDED.value" + " ON CONFLICT (resource_relationship_rule_id, key) DO NOTHING" _, err := tx.Exec(ctx, query, valueArgs...)Also applies to: 534-555, 557-578
📜 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/policies.go(2 hunks)apps/workspace-engine/pkg/db/relationships.go(2 hunks)apps/workspace-engine/pkg/db/resources.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/policies.goapps/workspace-engine/pkg/db/relationships.goapps/workspace-engine/pkg/db/resources.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/db/policies.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Policy(198-206)PolicyTargetSelector(217-222)PolicyRule(209-214)AnyApprovalRule(62-64)
apps/workspace-engine/pkg/db/relationships.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
RelationshipRule(246-260)Selector(317-319)Value(347-349)PropertyMatcher(230-234)
apps/workspace-engine/pkg/db/resources.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Resource(284-298)
⏰ 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: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
apps/workspace-engine/pkg/db/resources.go (2)
160-174: Past timestamp issue has been addressed.The UPSERT correctly omits
created_atfrom theDO UPDATE SETclause and usesNOW()forupdated_aton updates. This ensures audit timestamps are handled properly.
261-270: Verify ON DELETE CASCADE on resource_metadata.resource_id
deleteResourcerelies on DB cascades to clean up metadata. Confirm the foreign key onresource_metadata.resource_idincludesON DELETE CASCADEor add it to avoid orphaned rows.apps/workspace-engine/pkg/db/relationships.go (4)
354-365: Past issue addressed: stale metadata cleanup.The explicit DELETE statements before inserting updated metadata ensure no orphaned rows accumulate when relationship rules are updated.
367-374: Past issue addressed: property matcher extraction.The code now correctly extracts property matchers from
relationshipRule.MatcherwhenPropertyMatchersis empty, ensuring metadata matches are preserved during persistence.
399-448: Past issue addressed: error handling in selector extraction.Both
extractFromSelectorandextractToSelectornow return errors instead of silently swallowing failures, preventing incomplete or incorrect rule persistence.Also applies to: 451-501
580-589: All relationship rule metadata tables have ON DELETE CASCADE Confirmed that foreign keys in metadata_match, source_metadata_equals, and target_metadata_equals reference resource_relationship_rule with ON DELETE CASCADE.apps/workspace-engine/pkg/db/policies.go (4)
136-151: Past timestamp parsing issue has been addressed.The code now correctly parses
policy.CreatedAtfrom an RFC3339 string totime.Timebefore passing it to the database, preventing type conversion errors.
170-193: Past placeholder and argument issues have been addressed.The function now correctly uses 5 placeholders per row and passes
selector.Idfirst, thenpolicyId, aligning with the table's column order.
204-214: Past issues have been addressed.The function now correctly:
- Passes
rule.Idfirst, thenpolicyId(line 210), matching the query's column order- Parses
rule.CreatedAtfrom string totime.Timebefore passing to the database
216-225: ON DELETE CASCADE configured on policy_target and policy_rule_any_approval Both tables reference policy(id) with ON DELETE CASCADE; no orphaned rows risk.
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 (2)
apps/workspace-engine/pkg/db/policies.go(2 hunks)apps/workspace-engine/pkg/db/resources.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/policies.goapps/workspace-engine/pkg/db/resources.go
🧬 Code graph analysis (2)
apps/workspace-engine/pkg/db/policies.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Policy(198-206)PolicyTargetSelector(217-222)PolicyRule(209-214)AnyApprovalRule(62-64)
apps/workspace-engine/pkg/db/resources.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Resource(284-298)
⏰ 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: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
Summary by CodeRabbit