warehouse/migrations: add missing migrations for organizations

[woodruffw]

Was crufting up another migration.

[di]

Putting the 'blocked' label on this until we can review, probably don't want to merge this as-is.

[woodruffw]

Just for my own edification: these probably originate from #11184, right?

[di]

Looks like there is some of #8324 in there as well. We probably need a CI check that ensures that no migrations would be autogenerated...

[di]

This is now just changes that resulted from the organization-related work.

@divbzero @sterbo could you take a look and make sure these are as intended? See also https://warehouse.pypa.io/development/database-migrations.html regarding autogenerating migrations when changing models.

[di]

This is now just changes that resulted from the organization-related work.

@divbzero @sterbo could you take a look and make sure these are as intended? See also https://warehouse.pypa.io/development/database-migrations.html regarding autogenerating migrations when changing models.

[divbzero]

Ugh I was the last one to merge and test migrations for #11184.

Are these the right steps for testing migrations?

1. git checkout upstream/main
2. make initdb
3. git rebase -i @ foo-feature-branch or something similar to get feature branch up-to-date
4. docker-compose run web python -m warehouse db upgrade head

I thought I had done that but must have missed something.

[divbzero]

Or was the error in this case caused by us using sa.sql.false()?
https://github.com/pypa/warehouse/blob/354f24d890c88a070973810905523050e8550cb3/warehouse/migrations/versions/614a7fcb40ed_create_organization_models.py#L54-L56

[divbzero]

I think I see what happened. We manually added server_default=sa.sql.false() to the migration script to reflect the SQLAlchemy model:
https://github.com/pypa/warehouse/blob/354f24d890c88a070973810905523050e8550cb3/warehouse/organizations/models.py#L155

But Alembic doesn’t translate default=False to server_default=... when autogenerating so it’s flagging the is_active column as not-in-sync.

@sterbo I recall you mentioned something about this in a discussion. What’s the right move here? Could we give up on having BOOLEAN DEFAULT FALSE being enforced at the database level?

[sterbo]

I think I see what happened. We manually added server_default=sa.sql.false() to the migration script to reflect the SQLAlchemy model:

https://github.com/pypa/warehouse/blob/354f24d890c88a070973810905523050e8550cb3/warehouse/organizations/models.py#L155

But Alembic doesn’t translate default=False to server_default=... when autogenerating so it’s flagging the is_active column as not-in-sync.

@sterbo I recall you mentioned something about this in a discussion. What’s the right move here? Could we give up on having BOOLEAN DEFAULT FALSE being enforced at the database level?

I'm fine with not enforcing this at the database level. Banners and sponsors are configured in a similar fashion.

[di]

Are these the right steps for testing migrations?

You probably also want to try to autogenerate a migration whenever models have been changed with docker-compose run web python -m warehouse db revision --autogenerate --message "text"

But Alembic doesn’t translate default=False to server_default=... when autogenerating so it’s flagging the is_active column as not-in-sync.

I think you probably want to set server_default=sql.false() instead of default=False on the is_active column for these models, which would make this migration a no-op.

[sterbo]

I think you probably want to set server_default=sql.false() instead of default=False on the is_active column for these models, which would make this migration a no-op.

it looks like that'll do the trick. we'll give that a whirl. thanks @di