Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow pytest --migrations to succeed #14663

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ jobs:
tests:
- name: api-test
command: /start_tests.sh
- name: api-migrations
command: /start_tests.sh test_migrations
- name: api-lint
command: /var/lib/awx/venv/awx/bin/tox -e linters
- name: api-swagger
Expand Down
6 changes: 6 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,12 @@ test:
cd awxkit && $(VENV_BASE)/awx/bin/tox -re py3
awx-manage check_migrations --dry-run --check -n 'missing_migration_file'

test_migrations:
if [ "$(VENV_BASE)" ]; then \
. $(VENV_BASE)/awx/bin/activate; \
fi; \
PYTHONDONTWRITEBYTECODE=1 py.test -p no:cacheprovider --migrations -m migration_test $(PYTEST_ARGS) $(TEST_DIRS)

## Runs AWX_DOCKER_CMD inside a new docker container.
docker-runner:
docker run -u $(shell id -u) --rm -v $(shell pwd):/awx_devel/:Z --workdir=/awx_devel $(DEVEL_IMAGE_NAME) $(AWX_DOCKER_CMD)
Expand Down
5 changes: 4 additions & 1 deletion awx/main/migrations/0006_v320_release.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
# AWX
import awx.main.fields
from awx.main.models import Host
from ._sqlite_helper import dbawaremigrations


def replaces():
Expand Down Expand Up @@ -131,9 +132,11 @@ class Migration(migrations.Migration):
help_text='If enabled, Tower will act as an Ansible Fact Cache Plugin; persisting facts at the end of a playbook run to the database and caching facts for use by Ansible.',
),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
sql="CREATE INDEX host_ansible_facts_default_gin ON {} USING gin(ansible_facts jsonb_path_ops);".format(Host._meta.db_table),
reverse_sql='DROP INDEX host_ansible_facts_default_gin;',
sqlite_sql=dbawaremigrations.RunSQL.noop,
sqlite_reverse_sql=dbawaremigrations.RunSQL.noop,
),
# SCM file-based inventories
migrations.AddField(
Expand Down
33 changes: 18 additions & 15 deletions awx/main/migrations/0050_v340_drop_celery_tables.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,27 @@

from django.db import migrations

from ._sqlite_helper import dbawaremigrations

tables_to_drop = [
'celery_taskmeta',
'celery_tasksetmeta',
'djcelery_crontabschedule',
'djcelery_intervalschedule',
'djcelery_periodictask',
'djcelery_periodictasks',
'djcelery_taskstate',
'djcelery_workerstate',
'djkombu_message',
'djkombu_queue',
]
postgres_sql = ([("DROP TABLE IF EXISTS {} CASCADE;".format(table))] for table in tables_to_drop)
sqlite_sql = ([("DROP TABLE IF EXISTS {};".format(table))] for table in tables_to_drop)


class Migration(migrations.Migration):
dependencies = [
('main', '0049_v330_validate_instance_capacity_adjustment'),
]

operations = [
migrations.RunSQL([("DROP TABLE IF EXISTS {} CASCADE;".format(table))])
for table in (
'celery_taskmeta',
'celery_tasksetmeta',
'djcelery_crontabschedule',
'djcelery_intervalschedule',
'djcelery_periodictask',
'djcelery_periodictasks',
'djcelery_taskstate',
'djcelery_workerstate',
'djkombu_message',
'djkombu_queue',
)
]
operations = [dbawaremigrations.RunSQL(p, sqlite_sql=s) for p, s in zip(postgres_sql, sqlite_sql)]
9 changes: 8 additions & 1 deletion awx/main/migrations/0113_v370_event_bigint.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from django.db import migrations, models, connection

from ._sqlite_helper import dbawaremigrations


def migrate_event_data(apps, schema_editor):
# see: https://github.com/ansible/awx/issues/6010
Expand All @@ -24,6 +26,11 @@ def migrate_event_data(apps, schema_editor):
cursor.execute(f'ALTER TABLE {tblname} ALTER COLUMN id TYPE bigint USING id::bigint;')


def migrate_event_data_sqlite(apps, schema_editor):
# TODO: cmeyers fill this in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still got a TODO here. Why? Are you really going to migrate event data? I see no way you will get the hourly event tables to work in sqlite3. Of course it doesn't support range partitioning from the get-go, but even if you created superficial "partitions" as separate tables, I'm skeptical that it could be made to work, or that it would be worth it if you could do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is for the BigInt migration. I'm sure we won't ever want to do this in sqlite3. The next file does migration to partitions, which again, may be feasible superficially, but questionable value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is something needed here?

return


class FakeAlterField(migrations.AlterField):
def database_forwards(self, *args):
# this is intentionally left blank, because we're
Expand All @@ -37,7 +44,7 @@ class Migration(migrations.Migration):
]

operations = [
migrations.RunPython(migrate_event_data),
dbawaremigrations.RunPython(migrate_event_data, sqlite_code=migrate_event_data_sqlite),
FakeAlterField(
model_name='adhoccommandevent',
name='id',
Expand Down
8 changes: 7 additions & 1 deletion awx/main/migrations/0144_event_partitions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from django.db import migrations, models, connection

from ._sqlite_helper import dbawaremigrations


def migrate_event_data(apps, schema_editor):
# see: https://github.com/ansible/awx/issues/9039
Expand Down Expand Up @@ -59,6 +61,10 @@ def migrate_event_data(apps, schema_editor):
cursor.execute('DROP INDEX IF EXISTS main_jobevent_job_id_idx')


def migrate_event_data_sqlite(apps, schema_editor):
return None


class FakeAddField(migrations.AddField):
def database_forwards(self, *args):
# this is intentionally left blank, because we're
Expand All @@ -72,7 +78,7 @@ class Migration(migrations.Migration):
]

operations = [
migrations.RunPython(migrate_event_data),
dbawaremigrations.RunPython(migrate_event_data, sqlite_code=migrate_event_data_sqlite),
FakeAddField(
model_name='jobevent',
name='job_created',
Expand Down
32 changes: 22 additions & 10 deletions awx/main/migrations/0185_move_JSONBlob_to_JSONField.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import awx.main.models.notifications
from django.db import migrations, models

from ._sqlite_helper import dbawaremigrations


class Migration(migrations.Migration):
dependencies = [
Expand Down Expand Up @@ -104,11 +106,12 @@ class Migration(migrations.Migration):
name='deleted_actor',
field=models.JSONField(null=True),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_activitystream RENAME setting TO setting_old;
ALTER TABLE main_activitystream ALTER COLUMN setting_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_activitystream RENAME setting TO setting_old",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I guess this is kind of nice.

state_operations=[
migrations.RemoveField(
model_name='activitystream',
Expand All @@ -121,11 +124,12 @@ class Migration(migrations.Migration):
name='setting',
field=models.JSONField(blank=True, default=dict),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_job RENAME survey_passwords TO survey_passwords_old;
ALTER TABLE main_job ALTER COLUMN survey_passwords_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_job RENAME survey_passwords TO survey_passwords_old",
state_operations=[
migrations.RemoveField(
model_name='job',
Expand All @@ -138,11 +142,12 @@ class Migration(migrations.Migration):
name='survey_passwords',
field=models.JSONField(blank=True, default=dict, editable=False),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_joblaunchconfig RENAME char_prompts TO char_prompts_old;
ALTER TABLE main_joblaunchconfig ALTER COLUMN char_prompts_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_joblaunchconfig RENAME char_prompts TO char_prompts_old",
state_operations=[
migrations.RemoveField(
model_name='joblaunchconfig',
Expand All @@ -155,11 +160,12 @@ class Migration(migrations.Migration):
name='char_prompts',
field=models.JSONField(blank=True, default=dict),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_joblaunchconfig RENAME survey_passwords TO survey_passwords_old;
ALTER TABLE main_joblaunchconfig ALTER COLUMN survey_passwords_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_joblaunchconfig RENAME survey_passwords TO survey_passwords_old;",
state_operations=[
migrations.RemoveField(
model_name='joblaunchconfig',
Expand All @@ -172,11 +178,12 @@ class Migration(migrations.Migration):
name='survey_passwords',
field=models.JSONField(blank=True, default=dict, editable=False),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_notification RENAME body TO body_old;
ALTER TABLE main_notification ALTER COLUMN body_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_notification RENAME body TO body_old",
state_operations=[
migrations.RemoveField(
model_name='notification',
Expand All @@ -189,11 +196,12 @@ class Migration(migrations.Migration):
name='body',
field=models.JSONField(blank=True, default=dict),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_unifiedjob RENAME job_env TO job_env_old;
ALTER TABLE main_unifiedjob ALTER COLUMN job_env_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_unifiedjob RENAME job_env TO job_env_old",
state_operations=[
migrations.RemoveField(
model_name='unifiedjob',
Expand All @@ -206,11 +214,12 @@ class Migration(migrations.Migration):
name='job_env',
field=models.JSONField(blank=True, default=dict, editable=False),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_workflowjob RENAME char_prompts TO char_prompts_old;
ALTER TABLE main_workflowjob ALTER COLUMN char_prompts_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_workflowjob RENAME char_prompts TO char_prompts_old",
state_operations=[
migrations.RemoveField(
model_name='workflowjob',
Expand All @@ -223,11 +232,12 @@ class Migration(migrations.Migration):
name='char_prompts',
field=models.JSONField(blank=True, default=dict),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_workflowjob RENAME survey_passwords TO survey_passwords_old;
ALTER TABLE main_workflowjob ALTER COLUMN survey_passwords_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_workflowjob RENAME survey_passwords TO survey_passwords_old",
state_operations=[
migrations.RemoveField(
model_name='workflowjob',
Expand All @@ -240,11 +250,12 @@ class Migration(migrations.Migration):
name='survey_passwords',
field=models.JSONField(blank=True, default=dict, editable=False),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_workflowjobnode RENAME char_prompts TO char_prompts_old;
ALTER TABLE main_workflowjobnode ALTER COLUMN char_prompts_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_workflowjobnode RENAME char_prompts TO char_prompts_old",
state_operations=[
migrations.RemoveField(
model_name='workflowjobnode',
Expand All @@ -257,11 +268,12 @@ class Migration(migrations.Migration):
name='char_prompts',
field=models.JSONField(blank=True, default=dict),
),
migrations.RunSQL(
dbawaremigrations.RunSQL(
"""
ALTER TABLE main_workflowjobnode RENAME survey_passwords TO survey_passwords_old;
ALTER TABLE main_workflowjobnode ALTER COLUMN survey_passwords_old DROP NOT NULL;
""",
sqlite_sql="ALTER TABLE main_workflowjobnode RENAME survey_passwords TO survey_passwords_old",
state_operations=[
migrations.RemoveField(
model_name='workflowjobnode',
Expand Down
6 changes: 4 additions & 2 deletions awx/main/migrations/0186_drop_django_taggit.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

from django.db import migrations

from ._sqlite_helper import dbawaremigrations


def delete_taggit_contenttypes(apps, schema_editor):
ContentType = apps.get_model('contenttypes', 'ContentType')
Expand All @@ -20,8 +22,8 @@ class Migration(migrations.Migration):
]

operations = [
migrations.RunSQL("DROP TABLE IF EXISTS taggit_tag CASCADE;"),
migrations.RunSQL("DROP TABLE IF EXISTS taggit_taggeditem CASCADE;"),
dbawaremigrations.RunSQL("DROP TABLE IF EXISTS taggit_tag CASCADE;", sqlite_sql="DROP TABLE IF EXISTS taggit_tag;"),
dbawaremigrations.RunSQL("DROP TABLE IF EXISTS taggit_taggeditem CASCADE;", sqlite_sql="DROP TABLE IF EXISTS taggit_taggeditem;"),
migrations.RunPython(delete_taggit_contenttypes),
migrations.RunPython(delete_taggit_migration_records),
]
61 changes: 61 additions & 0 deletions awx/main/migrations/_sqlite_helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
from django.db import migrations


class RunSQL(migrations.operations.special.RunSQL):
"""
Bit of a hack here. Django actually wants this decision made in the router
and we can pass **hints.
"""

def __init__(self, *args, **kwargs):
if 'sqlite_sql' not in kwargs:
raise ValueError("sqlite_sql parameter required")
sqlite_sql = kwargs.pop('sqlite_sql')

self.sqlite_sql = sqlite_sql
self.sqlite_reverse_sql = kwargs.pop('sqlite_reverse_sql', None)
super().__init__(*args, **kwargs)

def database_forwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.sql = self.sqlite_sql or migrations.RunSQL.noop
super().database_forwards(app_label, schema_editor, from_state, to_state)

def database_backwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.reverse_sql = self.sqlite_reverse_sql or migrations.RunSQL.noop
super().database_backwards(app_label, schema_editor, from_state, to_state)


class RunPython(migrations.operations.special.RunPython):
"""
Bit of a hack here. Django actually wants this decision made in the router
and we can pass **hints.
"""

def __init__(self, *args, **kwargs):
if 'sqlite_code' not in kwargs:
raise ValueError("sqlite_code parameter required")
sqlite_code = kwargs.pop('sqlite_code')

self.sqlite_code = sqlite_code
self.sqlite_reverse_code = kwargs.pop('sqlite_reverse_code', None)
super().__init__(*args, **kwargs)

def database_forwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.code = self.sqlite_code or migrations.RunPython.noop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this behavior. One point, months ago, I did the same thing you did here - made the migrations compatible with sqlite3. But for the life of me I can not find the branch where I worked on that.

I just added a condition in the migration logic depending on the vendor, and that didn't take as many changes as you could think. Sure there are lots of logic steps we just don't want to run, but doing this I expect we're going to miss some stuff we should have, like creating default system jobs, or creating default credential types.

Also the naming of sqlitemigrations this is kind of concerning when taken out of this context.

Copy link
Member Author

@chrismeyersfsu chrismeyersfsu Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow.

Do you not like that the user can supply None for sqlite_code and are advocating for requiring sqlite_code to always be provided?
OR
Do you not like the overall idea of postgres sql vs. sqlite sql?
OR
Do you not like the variable choice that we are pivoting on?
OR
Do you not like "burying" this pivot logic and would rather the pivot be directly in the developers face, in the migration?

I'm fine with renaming sqlitemigrations

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @AlanCoding offline.

TL;DR self.code = self.sqlite_code or self.code or migrations.RunPython.noop instead of self.code = self.sqlite_code or migrations.RunPython.noop 👍

super().database_forwards(app_label, schema_editor, from_state, to_state)

def database_backwards(self, app_label, schema_editor, from_state, to_state):
if not schema_editor.connection.vendor.startswith('postgres'):
self.reverse_code = self.sqlite_reverse_code or migrations.RunPython.noop
super().database_backwards(app_label, schema_editor, from_state, to_state)


class _sqlitemigrations:
RunPython = RunPython
RunSQL = RunSQL


dbawaremigrations = _sqlitemigrations()
Loading
Loading