Open
Conversation
Proposes a simple, explicit migration system for Campus storage: - Migration files with upgrade()/downgrade() functions - State tracking via _migrations table - CLI: `campus migrate` and `campus rollback` - Yapper integration for audit logging - Supports PostgreSQL tables and MongoDB documents Addresses requirements from issue #27: 1. Initialize app via auto-run on deploy 2. Data migration support with batching 3. Cross-database migration handlers 4. Rollback capability with downgrade() 5. Audit logging via Yapper events Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
56 tasks
nycomp
commented
Mar 18, 2026
Contributor
Author
nycomp
left a comment
nycomp
left a comment
There was a problem hiding this comment.
What are the pros and cons of the following strategies:
- autocommit while upgrade in progress, with downgrade option
- atomic transaction, with all upgrade operations pending until explicitly committed
Assume database downtime while migration is in progress
Addresses review question comparing autocommit vs atomic transaction approaches. Key changes: - Add "Transaction Strategy" section with trade-off analysis - Explain hybrid approach: one migration = one transaction boundary - Document pros/cons of each strategy - Add guidance for large-scale migrations requiring batching Rationale: - Autocommit per migration enables handling large datasets - Atomic within migration ensures all-or-nothing for that change - Cross-storage compatible (PostgreSQL + MongoDB independently) - Matches Campus scale (~2000 users) and existing patterns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses review comment about fragility to typos in collection/table names. - Show both import-based (type-safe) and string-based access patterns - Recommend resource imports for production migrations - Keep string access as simpler alternative for quick migrations Reference: PR #382 review comment
Addresses review comment confirming migrations transform one revision at a time. - upgrade(): N-1 to N - downgrade(): N to N-1 - Never skip versions - run sequentially Ensures predictable transformation chain and easy rollback. Reference: PR #382 review comment
Addresses review comment about migration status tracking and archival. - Add "status" field: pending/applied/rolled_back/failed - Add "error_message" field for failed migrations - Document archival strategy: PostgreSQL table is primary audit log - Explain why PostgreSQL > JSON/bucket for this use case Reference: PR #382 review comment
Update MigrationState class to reflect new schema: - Add status field to init_state_table() - Update record_migration() to set status="applied" - Add record_failure() for failed migrations with error_message - Update get_applied_migrations() to filter by status="applied" - Update mark_rollback() to set status="rolled_back" - Update is_applied() to check status value Reference: PR #382 review comment
Addresses review question about whether CLI is necessary given automated deployment. - Add "When is CLI Needed?" section - Automated deployment: primary path (staging, PR validation, CI/CD) - CLI: secondary for manual operations (local dev, rollback, status, recovery) - Add `campus status` command to query migration state CLI remains useful for manual operations even with automated deployment. Reference: PR #382 review comment
Addresses review comment about integrating pre-deployment checklist into runner. - Add pre_flight_checks() method to MigrationRunner - Warns when running in production - Validates downgrade() exists for applied migrations - Shows pending migrations count - Requires confirmation in production - Add deployment checklist table showing built-in vs manual checks Moves validation from external checklist to automated pre-flight checks. Reference: PR #382 review comment
nycomp
commented
Mar 26, 2026
Reviewer choice: Use resource imports for type safety. - Remove Option 2 (string-based access) from example - Update example migration to use SubmissionsResource._storage - Emphasize benefits: no typos, IDE autocomplete, single source of truth Reference: PR #382 review comment
Addresses question about where to store _migrations table. Current architecture: campus.auth and campus.api have separate DBs. Option A (future): Dedicated migrations/audit DB - Centralized audit trail, clear separation - Additional infrastructure to manage Option B (current): Store in each service DB - Simpler, existing connections - Fragmented audit trail Recommendation: Start with Option B, move to A when needed. Reference: PR #382 review comment
Reviewer clarification: campus CLI is for user access, not admin control. - Create separate campus-admin package for operational commands - campus CLI: user features (auth login, api list) - campus-admin CLI: migrations, health checks, configuration Rationale: - User CLI should not expose dangerous admin operations - Admin commands require different auth/authorization - Clear boundary prevents accidental misuse Reference: PR #382 review comment
Addresses question: Should migrations use a different storage interface? Option A (current): Use existing storage layer via get_table() - Reuses infrastructure, consistent with app code - Risk: circular dependency if storage layer needs migration Option B: Direct database connection for migrations - Independent bootstrapping, can migrate storage layer - Con: duplicate connection management, more code Recommendation: Option A for Campus simplicity - Migrations are part of app, not external tools - Storage layer changes are infrequent - Document "bootstrap migrations" as special case if needed Reference: PR #382 review comment
nycomp
commented
Mar 26, 2026
Reviewer decision: Use 'admin' as program name, not 'campus-admin'. Changes: - Rename campus-admin → admin throughout - Remove duplicate CLI Implementation section - Update all usage examples to use 'admin' command - Clarify separate package/project status Reference: PR #382 review comment
…view) Reviewer question: Should audit/migration logging use raw queries for performance? Context: Audit logs trace every API call (high volume), unlike regular API traffic. Analysis: - Raw queries: faster but create mixed patterns, slippery slope - Storage layer: consistent but has abstraction overhead Recommendation: Storage layer with optimization hooks - Add bulk_insert() for high-volume audit - Keep single pattern throughout codebase - Avoid slippery slope of developers bypassing storage layer Decision: Storage layer + OptimizedTable for bulk operations, not raw queries. Reference: PR #382 review comment
Architecture decision: Auditing & migrations live in campus-admin as separate apps with their own schema and storage layer. Key changes: - Add architecture diagram showing Campus vs campus-admin separation - Update storage structure: campus-admin/storage/migrations/ - Document separate campus_admin_db for _migrations and _audit_log - Update all imports: campus_admin.storage instead of campus.storage - Explain rationale: domain data vs operational infrastructure - Document raw SQL for audit (high-volume write optimization) Rationale: - Concern separation: Domain models vs operational infrastructure - Performance isolation: Admin write-heavy won't impact Campus queries - Different backup policies: Admin longer retention than business data - Independent deployment: Campus updates don't depend on admin schema - Clear ownership: campus-admin owns its storage, not campus.storage Reference: PR #382 final decision
Implementation updates: - campus-admin creates its own storage module (campus-admin/storage/) - NOT under campus.storage, separate module - Does NOT use campus.common.devops.deploy infrastructure - Migrations and deployments are manual (no auto-run) Storage module clarification: - campus/storage/ → Campus domain storage - campus-admin/storage/ → Admin operational storage (separate repo) - Import: from campus_admin.storage import get_table Deployment changes: - Removed automatic migration trigger from Campus apps - Document manual workflow: ssh → admin migrate → admin status - Future auto-run would require campus-admin as dependency (not recommended initially) Reference: PR #382 implementation discussion
56 tasks
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.
Summary
Adds documentation for the storage migration protocol to address issue #27.
Key Decisions (Final)
Architecture: Separate admin storage layer
campus-adminpackagecampus_admin_dbfor operational dataProtocol includes:
upgrade()/downgrade()functions_migrationstable (in campus_admin_db)admin migrateandadmin rollback(separate from campus CLI)Storage separation:
campus.storage→ Domain models (users, assignments, submissions)campus_admin.storage→ Operational infrastructure (_migrations, _audit_log)campus.api.resourcesfor type-safe Campus data accessDocumentation Contents
adminCLI vs usercampusCLITest Plan
Implementation
See issue #383 for full implementation plan.
Key implementation tasks:
campus-adminpackage with storage/migrations/adminCLI (migrate, rollback, status)Closes #27
🤖 Generated with Claude Code