Conversation
- Create TenantScheduleEntity GORM model in services/control-plane/internal/scheduling/ - Add Atlas migration 20260406000001_create_tenant_schedule.sql with schema, unique constraint on schedule_name, and indexes for enabled/saga_name lookups - Update atlas.sum checksums The tenant_schedule table lives in per-tenant schemas (schema-per-tenant architecture). manifest_version_id is a soft cross-schema reference with no FK constraint.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/control-plane/migrations/20260406000001_create_tenant_schedule.sql (1)
12-12: Consider indexingmanifest_version_idfor audit/debug retrieval paths.If you’ll trace schedules by manifest version during investigations or rollback analysis, this will avoid full scans as table size grows.
📌 Suggested migration addition
CREATE INDEX idx_tenant_schedule_saga_name ON tenant_schedule (saga_name); +CREATE INDEX idx_tenant_schedule_manifest_version_id ON tenant_schedule (manifest_version_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/migrations/20260406000001_create_tenant_schedule.sql` at line 12, Add a secondary index on manifest_version_id to speed lookups: create an index on the tenant_schedule table for the manifest_version_id column (use a CONCURRENTLY create if the table may already be large) so queries that filter by manifest_version_id avoid full-table scans; update the migration to include this index creation and a matching drop index in the rollback path.services/control-plane/internal/scheduling/entity.go (1)
20-20: Consider usingdatatypes.JSONfor consistency across other services, though control-plane currently uses*stringfor all JSONB fields.While
datatypes.JSONis objectively cleaner and avoids manual marshal/unmarshal, the control-plane service has established a deliberate pattern of using*stringfor JSONB fields (manifest repository also follows this). If the field requires structured access or deserialization, migration todatatypes.JSONwould be an improvement; otherwise, maintaining consistency with the service's existing pattern is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/control-plane/internal/scheduling/entity.go` at line 20, The Metadata field in the struct currently uses *string for JSONB; change it to use datatypes.JSON for consistency and easier structured access: update the Metadata field's type to datatypes.JSON in the entity struct (symbol: Metadata) and add the import for "gorm.io/datatypes", then adjust any code that assumes a *string (marshal/unmarshal or nil checks) to handle datatypes.JSON accordingly (e.g., treat it as a byte slice or use datatypes.JSON.Marshal/Unmarshal where needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/control-plane/internal/scheduling/entity.go`:
- Line 20: The Metadata field in the struct currently uses *string for JSONB;
change it to use datatypes.JSON for consistency and easier structured access:
update the Metadata field's type to datatypes.JSON in the entity struct (symbol:
Metadata) and add the import for "gorm.io/datatypes", then adjust any code that
assumes a *string (marshal/unmarshal or nil checks) to handle datatypes.JSON
accordingly (e.g., treat it as a byte slice or use
datatypes.JSON.Marshal/Unmarshal where needed).
In `@services/control-plane/migrations/20260406000001_create_tenant_schedule.sql`:
- Line 12: Add a secondary index on manifest_version_id to speed lookups: create
an index on the tenant_schedule table for the manifest_version_id column (use a
CONCURRENTLY create if the table may already be large) so queries that filter by
manifest_version_id avoid full-table scans; update the migration to include this
index creation and a matching drop index in the rollback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 044c580e-2067-4284-ac1d-872c020ff8ec
⛔ Files ignored due to path filters (1)
services/control-plane/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
services/control-plane/internal/scheduling/entity.goservices/control-plane/migrations/20260406000001_create_tenant_schedule.sql
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ing--8--tenant-schedule-table
…ing--8--tenant-schedule-table
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Claude Code ReviewCommit: SummaryClean, well-scoped PR adding the Risk Assessment
Findings
Verification Checklist
|
1 participant
Summary
TenantScheduleEntityGORM model inservices/control-plane/internal/scheduling/20260406000001_create_tenant_schedule.sqlcreating thetenant_scheduletableatlas.sumchecksumsChanges Made
New entity:
services/control-plane/internal/scheduling/entity.goTenantScheduleEntitywith fields:id,schedule_name,saga_name,cron_expr,enabled,manifest_version_id,metadata,created_at,updated_atschedule_namehas a unique constraint for idempotent manifest applicationmanifest_version_idis a soft cross-schema reference (no FK) for audit/debuggingNew migration:
20260406000001_create_tenant_schedule.sqltenant_idcolumn)enabled(for ScheduleProvider load) andsaga_name(for manifest applier lookups)Technical Details
manifest_version_idis intentionally a soft reference - cross-schema FK is not supported in CockroachDB's per-tenant schema patternmetadatausesJSONBfor extensibility without schema churnTesting
go build ./services/control-plane/...passesatlas migrate hashupdated checksums successfully