Moved the feed data for the lookup tables from csv file into src/conf… #67
Conversation
…ig file. Added custom tests for alembic migration
nargis-sultani
requested review from
lchen-2101,
guffee23,
hkeeler and
jcadam14
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
|
closes #66 |
src/config.py
Outdated
There was a problem hiding this comment.
I don't know if this was discussed already in the other PRs or with Le, but should the individual feed lists go into their corresponding migration scripts to keep the seed data with the creation of the table the data is going into? The config class seems to be specific to env configurations we pull from either env vars or .env files.
There was a problem hiding this comment.
I can move it inside its individual scripts. That's the way I initially had it, but then I needed to access the same data for testing. So, I wanted to have them just in one place.
I can go ahead and move it inside the individual scripts.
There was a problem hiding this comment.
Gotcha, thought it might be that because of the comment in there. It's great that the tests check that the expected seed data is in tables so hopefully the tests can access the lists in the migrations (which is a little annoying to import each especially since the module names are horrific) or at the worst copy the list to the test testing the table.
There was a problem hiding this comment.
We should still have them inside the individual scripts for data integrity, considering the small data set. For testing purposes, you can test just a small sample of the data; or create test resources.
There was a problem hiding this comment.
@lchen-2101 Got it, yeah makes more sense.
…sions/seed.py; renamed filenames and functions names that had 'feed' to 'seed'
db_revisions/seed.py
Outdated
| @@ -0,0 +1,119 @@ | |||
| """ | |||
There was a problem hiding this comment.
We run into the same problem having this file rather than having the csv's. These should live within the scripts themselves; the purpose of the alembic scripts with versions is that each time the upgrade is ran, the same data is populated; we have decide to update the seed data, and we run the script in a different environment, then the 2 environments' data would become out of sync. Let's have these data within each script.
There was a problem hiding this comment.
I think if it was a big dataset, we should do the csv route, but version the file appropriately. i.e. address_state_7b6ff51002b5.csv; but since these are small, keeping them within the alembic script itself should be fine.
| data = get_feed_data_from_file("federal_regulator") | ||
|
|
||
| op.bulk_insert(FederalRegulatorDao.__table__, data) | ||
| op.bulk_insert(FederalRegulatorDao.__table__, federal_regulator_seed) |
There was a problem hiding this comment.
I think we talked about keeping the table names hardcoded? same kind of data integrity concerns where as we iterate, if we decided to switch the table for the dao, the new script would succeed in the old environment at creating the new table; but it would fail in the new environment as the table would already exist after running the previous version table creation script.
| branch_labels: Union[str, Sequence[str], None] = None | ||
| depends_on: Union[str, Sequence[str], None] = None | ||
|
|
||
| address_state_table = Base.metadata.tables.get("address_state") |
There was a problem hiding this comment.
If I'm not mistaken, this is still tied to the dao, correct? so if the dao has been updated to say "address_state_new" (hypothetical, do not actually do this); this would fail, correct?
Here's a way to decouple the two:
meta = MetaData()
meta.reflect(op.get_bind())
table = Table("address_state", meta)There was a problem hiding this comment.
Looking into it.
| meta = MetaData() | ||
| meta.reflect(op.get_bind()) | ||
| table = Table("federal_regulator", meta) |
There was a problem hiding this comment.
guessing u r in the process of replacing these with the get_table_by_name in utils? looks good otherwise, so ready to merge once the last changes are implemented.
lchen-2101
deleted the
feature/66_add_more_alembic_custom_tests_and_add_seed_data_inside_script
branch
…ig file. Added custom tests for alembic migration