PR#1218: Add missing migration - fm-orchestrator

fm-orchestrator

#1218 Add missing migration

Closed 4 years ago by mprahl. Opened 5 years ago by cqi.

cqi/fm-orchestrator add-missing-migration into master

Add missing migration

Chenxiong Qi • 5 years ago

4ff6fe5

module_build_service/migrations/versions/7b7788869415_.py

file added

+34

		`@@ -0,0 +1,34 @@`
		`+ """empty message`
mprahl commented 5 years ago Could you set a message here explaining the migration?
		`+`
		`+ Revision ID: 7b7788869415`
		`+ Revises: 60c6093dde9e`
		`+ Create Date: 2019-04-19 06:57:37.472140`
		`+`
		`+ """`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = '7b7788869415'`
		`+ down_revision = '60c6093dde9e'`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ def upgrade():`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
mprahl commented 5 years ago Please remove any default comments.
		`+ op.alter_column('module_builds', 'context',`
		`+ existing_type=sa.VARCHAR(),`
		`+ nullable=True,`
mprahl commented 5 years ago After doing some research, it seems that we want `nullable=False` set on the `context` column in `models.py`. This will cause a migration script to not be generated, and that is the behavior we want anyways. I'm not sure how all databases treat nullable + server default, but some databases such as Microsoft SQL Server, will ignore the server default if the column is nullable. You can read about that here: https://blog.sqlauthority.com/2017/07/02/default-value-nullable-column-works-interview-question-week-129/ It does seem that setting it to `nullable=False` causes issues in the tests. Perhaps we can use `default` to resolve this. Edited 4 years ago by mprahl
		`+ existing_server_default=sa.text(u"'00000000'"))`
		`+ op.create_unique_constraint('unique_buildrequires', 'module_builds_to_module_buildrequires', ['module_id', 'module_buildrequire_id'])`
mprahl commented 5 years ago I noticed this as well on Thursday. I'm not sure how it got missed in previous PRs. Perhaps as part of our CI tests, we should make sure `mbs-manager db migrate` doesn't generate any new migration scripts. Edited 5 years ago by mprahl
		`+ # ### end Alembic commands ###`
		`+`
		`+`
		`+ def downgrade():`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
		`+ op.drop_constraint('unique_buildrequires', 'module_builds_to_module_buildrequires', type_='unique')`
		`+ op.alter_column('module_builds', 'context',`
		`+ existing_type=sa.VARCHAR(),`
		`+ nullable=False,`
		`+ existing_server_default=sa.text(u"'00000000'"))`
		`+ # ### end Alembic commands ###`

cqi commented 5 years ago

Add migrations on ModuleBuild.context and unique constraint on table
module_builds_to_module_buildrequires.

Signed-off-by: Chenxiong Qi cqi@redhat.com

cqi commented 5 years ago

Found this missing migration when trying to make migration for other changes to model ModuleBuild. Steps to reproduce

Remove old module_build_service.db
Run mbs-upgradedb
Run python module_build_service/manage.py db revision --autogenerate -d module_build_service/migrations/

Tests pass with this migration.

mprahl commented on line 22 of module_build_service/migrations/versions/7b7788869415_.py 5 years ago

After doing some research, it seems that we want nullable=False set on the context column in models.py. This will cause a migration script to not be generated, and that is the behavior we want anyways. I'm not sure how all databases treat nullable + server default, but some databases such as Microsoft SQL Server, will ignore the server default if the column is nullable. You can read about that here:
https://blog.sqlauthority.com/2017/07/02/default-value-nullable-column-works-interview-question-week-129/

It does seem that setting it to nullable=False causes issues in the tests. Perhaps we can use default to resolve this.

Edited 4 years ago by mprahl

mprahl commented on line 2 of module_build_service/migrations/versions/7b7788869415_.py 5 years ago

Could you set a message here explaining the migration?

mprahl commented on line 19 of module_build_service/migrations/versions/7b7788869415_.py 5 years ago

Please remove any default comments.

mprahl commented on line 24 of module_build_service/migrations/versions/7b7788869415_.py 5 years ago

I noticed this as well on Thursday. I'm not sure how it got missed in previous PRs. Perhaps as part of our CI tests, we should make sure mbs-manager db migrate doesn't generate any new migration scripts.

Edited 5 years ago by mprahl

cqi commented 4 years ago

I noticed this as well on Thursday. I'm not sure how it got missed in previous PRs. Perhaps as part of our CI tests, we should make sure mbs-manager db migrate doesn't generate any new migration scripts.

I found out the unique constraint has to have name unique_buildrequires, otherwise revision --autogenerate always detects the unique constraint and generate a op.create_unique_constraint call to create one and name it unqiue_buildrequires. I compared this behavior in sqlite and PostgreSQL, it happens in both. But, a slight different is that, for PostgreSQL, alembic also call op.drop_constraint to drop the original one created in 526fb7d445f7_module_buildrequires.py. Hence, the solution should be just to rename existing unique constraint to unique_buildrequires.

cqi commented 4 years ago

It does seem that setting it to nullable=False causes issues in the tests. Perhaps we can use default to resolve this.

Using default does not fix the issue here.

ModuleBuild.context was added in commit d83e689, where it describes explicitly NULL is not allowed for field context. The migration sets nullable=False, but it is not set in model ModuleBuild accordingly. I tested setting nullable=False, it works in both SQLite and PostgreSQL. With this change, it is really necessary to fix code building test data for tests.