Cleaned up branch to no longer be a full merge of #7 and #18 #20
Conversation
jcadam14
requested review from
hkeeler,
guffee23,
lchen-2101 and
nargis-sultani
| "filing", | ||
| sa.Column("id", sa.INTEGER, primary_key=True, autoincrement=True), | ||
| sa.Column("lei", sa.String, nullable=False), | ||
| sa.Column("state", sa.Enum(FilingState, name="filingstate")), |
There was a problem hiding this comment.
Would it be possible to create the enum types as its own script instead of relying on the model? If I'm not mistaken, this will auto generate the type creation at this point in time correct? Future updates to the enum in code doesn't automatically get synced, then we run into sync issues, where future code may have additional values, and they get automatically generated, but there are revisions that add the new enums.
There was a problem hiding this comment.
I think I'd like to keep this and the code models decoupled, and be explicit on what we're doing with the database.
There was a problem hiding this comment.
Yup this makes sense. At the time I did it to ensure there were no copy/paste errors for the enum type, but yeah if we rollback this would continue to use whatever is in FilingState and not what should be in there for the rollback. So I'll create a new script to create the enums in the db that these are dependent on.
|
|
||
|
|
||
| def upgrade() -> None: | ||
| if not table_exists("filing"): |
There was a problem hiding this comment.
let's move away from this table_exists check now, since this is a brand new repo where all the tables are being created by alembic.
- Removed the if_table_exists util.py - Updated table creations to not use the model enum py file, instead builds based on default values. Can be updated later with enum_migration.update_value
| sa.Column("start_period", sa.DateTime, server_default=sa.func.now(), nullable=False), | ||
| sa.Column("end_period", sa.DateTime, server_default=sa.func.now(), nullable=False), | ||
| sa.Column("due", sa.DateTime, server_default=sa.func.now(), nullable=False), |
There was a problem hiding this comment.
these shouldn't have defaults, we want to explicitly specify them as we create them.
they would look something like:
start_period: 1/1/2024
end_period: 12/31/2024
due: 3/1/2025
There was a problem hiding this comment.
otherwise looks good, ready to merge after this change.
Closes #13
Created a new branch that focuses purely on the alembic script needs instead of a full merge of #7 and #18 into the #13 branch. Makes it so only the code related to the migrations is needed for review. Originally I merged them all together to test starting the service in poetry to make sure the tables are created correctly in Postgres but that made the PR have a lot of files that were already covered in other PRs. Still the case with the entities/models directory but couldn't get around that.
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Browsers
Accessibility
Other