Conversation
delete unused rule.html template form
…in git - Add baseline migration (001_baseline) with complete v1.2.2 schema and seed data - Rewrite db-init.py to run flask db upgrade instead of raw db.create_all() - Remove /admin/set-org-if-zero endpoint, extract logic to scripts/migrate_v0x_to_v1.py - Remove migrations/ from .gitignore - Rewrite docs/DB_MIGRATIONS.md with new workflow
…database from v0.4+ to current Migration tracking in git — removed migrations/ from .gitignore db-init.py rewritten to use flask db upgrade instead of db.create_all() /admin/set-org-if-zero removed — replaced with standalone migrate_v0x_to_v1.py Docker dev container — added PYTHONPATH=/app for easier development Alembic env.py — fixed deprecation warning Docs updated — DB_MIGRATIONS.md and CHANGELOG.md
Migrations bundled as package data — no flask db init needed. Flask-Migrate configured to find them via package-relative path. Added community.as_path to baseline migration column checks. Updated DB_MIGRATIONS.md with upgrade paths for existing deployments.
Added missing checks for very old databases (2019 era): - log.author column (added in v0.5.0) - community.comm, larcomm, extcomm columns (added ~v0.7) - rstate id=4 "whitelisted rule" seed data (added in v1.1.0)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive migration management system for ExaFS v1.2.2 to address issue #73 regarding inconsistent database migrations. The key change is bundling migration files as package data, eliminating the need for flask db init and ensuring all deployments use identical, tested migrations.
Changes:
- Introduced idempotent baseline migration (
001_baseline.py) that brings any ExaFS database from v0.4+ to v1.2.2 schema - Bundled migrations inside
flowapp/migrations/package directory and configured as package data - Added optional data migration helper script (
migrate_v0x_to_v1.py) for backfilling org_id on v0.x upgrades - Replaced admin endpoint
/admin/set-org-if-zerowith standalone migration script - Updated templates to use
url_for()helper instead of hardcoded URLs - Updated documentation with comprehensive migration upgrade paths
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| flowapp/migrations/versions/001_baseline.py | Idempotent baseline migration creating complete v1.2.2 schema with conditional logic for existing databases |
| flowapp/migrations/env.py | Standard Alembic environment configuration for Flask-Migrate |
| flowapp/migrations/alembic.ini | Alembic logging and configuration settings |
| flowapp/migrations/script.py.mako | Template for generating new migration files |
| flowapp/migrations/README | Single-line description of migration setup |
| flowapp/init.py | Configures Migrate with package-relative migrations directory |
| pyproject.toml | Adds migrations directories to package data |
| scripts/migrate_v0x_to_v1.py | Helper script to backfill org_id=0 records for v0.x to v1.0+ upgrades |
| flowapp/views/admin.py | Removes temporary /admin/set-org-if-zero endpoint and unused imports |
| flowapp/templates/layouts/default.html | Updates URLs to use url_for() helper for proper routing |
| flowapp/templates/forms/rule.html | Removes unused template file |
| flowapp/models/*.py | Adds comments noting seed data must stay in sync with migration |
| docs/DB_MIGRATIONS.md | Complete rewrite with upgrade paths for all deployment scenarios |
| db-init.py | Refactored to use flask db upgrade instead of db.create_all() |
| CHANGELOG.md | Documents v1.2.2 changes |
| CLAUDE.md | Updates developer documentation for migration workflow |
| .gitignore | Removes migrations/ from gitignore to track migrations in git |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+83
to
+84
| "migrations/*", | ||
| "migrations/versions/*", |
There was a problem hiding this comment.
The package-data configuration only includes patterns for directories (migrations/* and migrations/versions/*), but this may not include all necessary files. For proper migration support, you should explicitly include all migration infrastructure files. Consider using more specific patterns:
- "migrations/README"
- "migrations/alembic.ini"
- "migrations/env.py"
- "migrations/script.py.mako"
- "migrations/versions/*.py"
Alternatively, use "migrations/**/*" to recursively include all files in the migrations directory tree (requires setuptools >= 62.0).
Suggested change
| "migrations/*", | |
| "migrations/versions/*", | |
| "migrations/README", | |
| "migrations/alembic.ini", | |
| "migrations/env.py", | |
| "migrations/script.py.mako", | |
| "migrations/versions/*.py", |
Collaborator
Author
There was a problem hiding this comment.
Package data reflects current migrations dir layout. Will be updated if necessary in future commits.
Comment on lines
48
to
614
| def upgrade(): | ||
| # --- Tables with no foreign key dependencies --- | ||
|
|
||
| if not _table_exists("role"): | ||
| role_table = op.create_table( | ||
| "role", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=20), unique=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| ) | ||
| _seed_roles = True | ||
| else: | ||
| role_table = sa.table( | ||
| "role", | ||
| sa.column("name", sa.String), | ||
| sa.column("description", sa.String), | ||
| ) | ||
| _seed_roles = False | ||
|
|
||
| if not _table_exists("organization"): | ||
| organization_table = op.create_table( | ||
| "organization", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=150), unique=True), | ||
| sa.Column("arange", sa.Text()), | ||
| sa.Column("limit_flowspec4", sa.Integer(), default=0), | ||
| sa.Column("limit_flowspec6", sa.Integer(), default=0), | ||
| sa.Column("limit_rtbh", sa.Integer(), default=0), | ||
| ) | ||
| _seed_orgs = True | ||
| else: | ||
| organization_table = None | ||
| _seed_orgs = False | ||
| # Add limit columns if missing (pre-v1.0 databases) | ||
| for col_name in ("limit_flowspec4", "limit_flowspec6", "limit_rtbh"): | ||
| if not _column_exists("organization", col_name): | ||
| op.add_column( | ||
| "organization", sa.Column(col_name, sa.Integer(), default=0) | ||
| ) | ||
|
|
||
| if not _table_exists("rstate"): | ||
| rstate_table = op.create_table( | ||
| "rstate", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| ) | ||
| _seed_rstates = True | ||
| else: | ||
| rstate_table = sa.table( | ||
| "rstate", | ||
| sa.column("description", sa.String), | ||
| ) | ||
| _seed_rstates = False | ||
|
|
||
| if not _table_exists("user"): | ||
| op.create_table( | ||
| "user", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("uuid", sa.String(length=180), unique=True), | ||
| sa.Column("comment", sa.String(length=500)), | ||
| sa.Column("email", sa.String(length=255)), | ||
| sa.Column("name", sa.String(length=255)), | ||
| sa.Column("phone", sa.String(length=255)), | ||
| ) | ||
|
|
||
| if not _table_exists("as_path"): | ||
| op.create_table( | ||
| "as_path", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("prefix", sa.String(length=120), unique=True), | ||
| sa.Column("as_path", sa.String(length=250)), | ||
| ) | ||
|
|
||
| if not _table_exists("log"): | ||
| op.create_table( | ||
| "log", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("time", sa.DateTime()), | ||
| sa.Column("task", sa.String(length=1000)), | ||
| sa.Column("author", sa.String(length=1000)), | ||
| sa.Column("rule_type", sa.Integer()), | ||
| sa.Column("rule_id", sa.Integer()), | ||
| sa.Column("user_id", sa.Integer()), | ||
| ) | ||
| else: | ||
| # Add author column if missing (pre-v0.5 databases) | ||
| if not _column_exists("log", "author"): | ||
| op.add_column( | ||
| "log", | ||
| sa.Column("author", sa.String(length=1000)), | ||
| ) | ||
|
|
||
| # --- Junction tables --- | ||
|
|
||
| if not _table_exists("user_role"): | ||
| op.create_table( | ||
| "user_role", | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| sa.PrimaryKeyConstraint("user_id", "role_id"), | ||
| ) | ||
|
|
||
| if not _table_exists("user_organization"): | ||
| op.create_table( | ||
| "user_organization", | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "organization_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.PrimaryKeyConstraint("user_id", "organization_id"), | ||
| ) | ||
|
|
||
| # --- Tables with foreign key to role --- | ||
|
|
||
| if not _table_exists("action"): | ||
| action_table = op.create_table( | ||
| "action", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=120), unique=True), | ||
| sa.Column("command", sa.String(length=120), unique=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| ) | ||
| _seed_actions = True | ||
| else: | ||
| action_table = sa.table( | ||
| "action", | ||
| sa.column("name", sa.String), | ||
| sa.column("command", sa.String), | ||
| sa.column("description", sa.String), | ||
| sa.column("role_id", sa.Integer), | ||
| ) | ||
| _seed_actions = False | ||
|
|
||
| if not _table_exists("community"): | ||
| community_table = op.create_table( | ||
| "community", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=120), unique=True), | ||
| sa.Column("comm", sa.String(length=2047)), | ||
| sa.Column("larcomm", sa.String(length=2047)), | ||
| sa.Column("extcomm", sa.String(length=2047)), | ||
| sa.Column("description", sa.String(length=255)), | ||
| sa.Column("as_path", sa.Boolean(), default=False), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| ) | ||
| _seed_communities = True | ||
| else: | ||
| community_table = sa.table( | ||
| "community", | ||
| sa.column("name", sa.String), | ||
| sa.column("comm", sa.String), | ||
| sa.column("larcomm", sa.String), | ||
| sa.column("extcomm", sa.String), | ||
| sa.column("description", sa.String), | ||
| sa.column("as_path", sa.Boolean), | ||
| sa.column("role_id", sa.Integer), | ||
| ) | ||
| _seed_communities = False | ||
| # Add community columns if missing (pre-v0.7 databases) | ||
| for col_name in ("comm", "larcomm", "extcomm"): | ||
| if not _column_exists("community", col_name): | ||
| op.add_column( | ||
| "community", | ||
| sa.Column(col_name, sa.String(length=2047)), | ||
| ) | ||
| # Add as_path column if missing (pre-v1.1 databases) | ||
| if not _column_exists("community", "as_path"): | ||
| op.add_column( | ||
| "community", | ||
| sa.Column("as_path", sa.Boolean(), default=False), | ||
| ) | ||
|
|
||
| # --- API key tables --- | ||
|
|
||
| if not _table_exists("api_key"): | ||
| op.create_table( | ||
| "api_key", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("machine", sa.String(length=255)), | ||
| sa.Column("key", sa.String(length=255)), | ||
| sa.Column("readonly", sa.Boolean(), default=False), | ||
| sa.Column("expires", sa.DateTime(), nullable=True), | ||
| sa.Column("comment", sa.String(length=255)), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| ) | ||
| else: | ||
| # Add columns introduced after initial api_key creation | ||
| for col_name, col_type, col_default in [ | ||
| ("comment", sa.String(length=255), None), | ||
| ("readonly", sa.Boolean(), False), | ||
| ("expires", sa.DateTime(), None), | ||
| ]: | ||
| if not _column_exists("api_key", col_name): | ||
| op.add_column( | ||
| "api_key", | ||
| sa.Column(col_name, col_type, default=col_default), | ||
| ) | ||
| if not _column_exists("api_key", "org_id"): | ||
| op.add_column( | ||
| "api_key", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("machine_api_key"): | ||
| op.create_table( | ||
| "machine_api_key", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("machine", sa.String(length=255)), | ||
| sa.Column("key", sa.String(length=255)), | ||
| sa.Column("readonly", sa.Boolean(), default=True), | ||
| sa.Column("expires", sa.DateTime(), nullable=True), | ||
| sa.Column("comment", sa.String(length=255)), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| ) | ||
|
|
||
| # --- Rule tables --- | ||
|
|
||
| if not _table_exists("flowspec4"): | ||
| op.create_table( | ||
| "flowspec4", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("source", sa.String(length=255)), | ||
| sa.Column("source_mask", sa.Integer()), | ||
| sa.Column("source_port", sa.String(length=255)), | ||
| sa.Column("dest", sa.String(length=255)), | ||
| sa.Column("dest_mask", sa.Integer()), | ||
| sa.Column("dest_port", sa.String(length=255)), | ||
| sa.Column("protocol", sa.String(length=255)), | ||
| sa.Column("flags", sa.String(length=255)), | ||
| sa.Column("packet_len", sa.String(length=255)), | ||
| sa.Column("fragment", sa.String(length=255)), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "action_id", sa.Integer(), sa.ForeignKey("action.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("flowspec4", "fragment"): | ||
| op.add_column( | ||
| "flowspec4", | ||
| sa.Column("fragment", sa.String(length=255)), | ||
| ) | ||
| if not _column_exists("flowspec4", "org_id"): | ||
| op.add_column( | ||
| "flowspec4", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("flowspec6"): | ||
| op.create_table( | ||
| "flowspec6", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("source", sa.String(length=255)), | ||
| sa.Column("source_mask", sa.Integer()), | ||
| sa.Column("source_port", sa.String(length=255)), | ||
| sa.Column("dest", sa.String(length=255)), | ||
| sa.Column("dest_mask", sa.Integer()), | ||
| sa.Column("dest_port", sa.String(length=255)), | ||
| sa.Column("next_header", sa.String(length=255)), | ||
| sa.Column("flags", sa.String(length=255)), | ||
| sa.Column("packet_len", sa.String(length=255)), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "action_id", sa.Integer(), sa.ForeignKey("action.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("flowspec6", "org_id"): | ||
| op.add_column( | ||
| "flowspec6", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("RTBH"): | ||
| op.create_table( | ||
| "RTBH", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("ipv4", sa.String(length=255)), | ||
| sa.Column("ipv4_mask", sa.Integer()), | ||
| sa.Column("ipv6", sa.String(length=255)), | ||
| sa.Column("ipv6_mask", sa.Integer()), | ||
| sa.Column( | ||
| "community_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("community.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("RTBH", "org_id"): | ||
| op.add_column( | ||
| "RTBH", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("whitelist"): | ||
| op.create_table( | ||
| "whitelist", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("ip", sa.String(length=255)), | ||
| sa.Column("mask", sa.Integer()), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("rule_whitelist_cache"): | ||
| op.create_table( | ||
| "rule_whitelist_cache", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("rid", sa.Integer()), | ||
| sa.Column("rtype", sa.Integer()), | ||
| sa.Column("rorigin", sa.Integer()), | ||
| sa.Column( | ||
| "whitelist_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("whitelist.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| # --- Seed data (only for newly created tables) --- | ||
|
|
||
| if _seed_roles and not _table_has_data("role"): | ||
| op.bulk_insert( | ||
| role_table, | ||
| [ | ||
| {"name": "view", "description": "just view, no edit"}, | ||
| {"name": "user", "description": "can edit"}, | ||
| {"name": "admin", "description": "admin"}, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_orgs and organization_table is not None: | ||
| op.bulk_insert( | ||
| organization_table, | ||
| [ | ||
| { | ||
| "name": "TU Liberec", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| { | ||
| "name": "Cesnet", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| # Ensure rstate has the "whitelisted rule" entry (id=4, added in v1.1.0) | ||
| if not _seed_rstates and _table_has_data("rstate"): | ||
| conn = op.get_bind() | ||
| result = conn.execute( | ||
| sa.text("SELECT COUNT(*) FROM rstate WHERE id = 4") | ||
| ) | ||
| if result.scalar() == 0: | ||
| conn.execute( | ||
| sa.text( | ||
| "INSERT INTO rstate (id, description) VALUES (4, 'whitelisted rule')" | ||
| ) | ||
| ) | ||
|
|
||
| if _seed_rstates and not _table_has_data("rstate"): | ||
| op.bulk_insert( | ||
| rstate_table, | ||
| [ | ||
| {"description": "active rule"}, | ||
| {"description": "withdrawed rule"}, | ||
| {"description": "deleted rule"}, | ||
| {"description": "whitelisted rule"}, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_actions and not _table_has_data("action"): | ||
| op.bulk_insert( | ||
| action_table, | ||
| [ | ||
| { | ||
| "name": "QoS 100 kbps", | ||
| "command": "rate-limit 12800", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "QoS 1Mbps", | ||
| "command": "rate-limit 13107200", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "QoS 10Mbps", | ||
| "command": "rate-limit 131072000", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "Discard", | ||
| "command": "discard", | ||
| "description": "Discard", | ||
| "role_id": 2, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_communities and not _table_has_data("community"): | ||
| op.bulk_insert( | ||
| community_table, | ||
| [ | ||
| { | ||
| "name": "65535:65283", | ||
| "comm": "65535:65283", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "local-as", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "64496:64511", | ||
| "comm": "64496:64511", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "64497:64510", | ||
| "comm": "64497:64510", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
|
|
||
| def downgrade(): | ||
| op.drop_table("rule_whitelist_cache") | ||
| op.drop_table("whitelist") | ||
| op.drop_table("RTBH") | ||
| op.drop_table("flowspec6") | ||
| op.drop_table("flowspec4") | ||
| op.drop_table("machine_api_key") | ||
| op.drop_table("api_key") | ||
| op.drop_table("community") | ||
| op.drop_table("action") | ||
| op.drop_table("user_organization") | ||
| op.drop_table("user_role") | ||
| op.drop_table("log") | ||
| op.drop_table("as_path") | ||
| op.drop_table("user") | ||
| op.drop_table("rstate") | ||
| op.drop_table("organization") | ||
| op.drop_table("role") |
There was a problem hiding this comment.
The migration script includes comprehensive database migrations but there are no corresponding tests for the migration logic itself. Given the complexity and critical importance of migrations (as evidenced by issue #73), tests should be added to verify:
- Idempotent behavior (running migration twice doesn't fail)
- Upgrading from v0.x databases works correctly
- Column additions preserve existing data
- Seed data is properly inserted only when needed
The test suite has comprehensive coverage for models, APIs, and services (as seen in tests/), but no migration tests exist.
Comment on lines
+493
to
+502
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| { | ||
| "name": "Cesnet", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, |
There was a problem hiding this comment.
The seed data in the migration explicitly sets limit_flowspec4, limit_flowspec6, and limit_rtbh to 0, but the event listener in organization.py (line 35-36) doesn't set these fields when creating organizations via db.create_all(). This creates an inconsistency between migration-based initialization and model-based initialization.
While the model defines default=0 for these columns, which should handle NULL values, it's better to keep the seed data consistent. Either update the event listener to include the limit fields, or rely on the column defaults and remove them from the migration seed data.
Suggested change
| "limit_flowspec4": 0, | |
| "limit_flowspec6": 0, | |
| "limit_rtbh": 0, | |
| }, | |
| { | |
| "name": "Cesnet", | |
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | |
| "limit_flowspec4": 0, | |
| "limit_flowspec6": 0, | |
| "limit_rtbh": 0, | |
| }, | |
| { | |
| "name": "Cesnet", | |
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", |
Collaborator
Author
There was a problem hiding this comment.
@todo emove default organizations completely.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
1 participant
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.
version 1.2.2 release candidate, closes #73
Migrations bundled as package data — no flask db init needed.
Flask-Migrate configured to find them via package-relative path.
New doscs DB_MIGRATIONS.md with upgrade paths for existing deployments.