-
Notifications
You must be signed in to change notification settings - Fork 0
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
Product Logging: Add sql scripts used for creating views in db #1194
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
=======================================
Coverage 68.61% 68.61%
=======================================
Files 109 109
Lines 5773 5773
Branches 634 634
=======================================
Hits 3961 3961
Misses 1685 1685
Partials 127 127 ☔ View full report in Codecov by Sentry. |
f9f010d
to
bcb8403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! This looks quite useful. There's one change I think we'll need, and then a few thoughts.
One change to request:
Everything in migrations has to be run in order - there are different filenaming conventions to achieve that, but it's pretty typical to just prepend a datetime so that the migrations sort.
Other thoughts:
Maybe there's a nicer approach than WHERE product <> 'db-template'
? We could either make a whitelisted products table, or a table of products to ignore (with one row). Then you can create a view of the events table referencing that 👆so you never have to write WHERE product <> 'db-template'
ever again.
And sorry, I know I've asked this before, but do you see this being used for other lifecycle stages other than draft/publish?
42b404a
to
1bf9b25
Compare
Thanks for the feedback! I timestamped the files. I ended up not creating a migration bash script recreating the tables in this PR because I would need a container to test this in... Which I would like to address in a separate PR if that's okay! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! One minor change: some of the filenames have an extra .sql
on the end. Feel free to merge after fixing (No need to re-request review)
@sf-dcp @damonmcc So what we've done here for filenames is a pretty standard convention for migrations (e.g. Rails uses it). It's... quite long though. This isn't exactly nice to look at: What we've got now20240101000000_create_source_data__metadata_logging.sql 👆is totally good enough for now. But I want to get your thoughts on some potential alternatives: YYYYMMDDHH_{schema}__{description}.sql (omit seconds onward)2024010100_source_data__metadata_logging.sql YYMMDDHH_{schema}__{description}.sql (omit first two of year and seconds. I think this is my preference)24010100_source_data__metadata_logging.sql -> {epoch_timestamp}_{schema}__{description}.sql1704070800_source_data__metadata_logging.sql -> |
|
@damonmcc yeah, I think I've with you on the readability. @sf-dcp thoughts? |
Yeah, I found this format most readable as well. Sounds like we have a consensus. I will revise the file names and merge :) Thanks for the feedback! |
1bf9b25
to
52e6d4d
Compare
There are some failing pytests. I merged this PR as these tests are unrelated to the PR files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sf-dcp I think we should probably put the migrations in a folder called dcpy/migrations/database/
and have the README up one level in dcpy/migrations/README.md
to avoid mixing these files.
I created a couple of views built on top of the logging table. Adding the scripts to the repo just to have them as a reference. We don't actually use these scripts anywhere.
What these views look like:
You can also check these views under
edm-qaqc
db ->product_data
schema