-
Notifications
You must be signed in to change notification settings - Fork 21
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
Location ids table and locatable events QA check #6164
Conversation
Passing run #20323 ↗︎Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
Maybe better to add it under the etl schema rather than events through this route? I'm also thinking if anywhere it belongs in the infrastructure schema. No objection in principle to the create occurring here, but shall we add the ddl for it to the flowdb schema as well? |
My reasoning for putting the table in Could put it in I think you're right, we should add the DDL to the FlowDB schema. I'm not keen on having it in the FlowDB schema and in the ETL task, because then there's a risk of the two getting out-of-sync. Perhaps it's best to have it in the FlowDB scripts only, and then if the task tries to run on a schema that's not updated it will fail. In lieu of a proper migration process, it's probably not actually very helpful to start scattering partial migration steps throughout the ETL. |
A follow on question would then be if we actually want a separate |
Codecov Report
@@ Coverage Diff @@
## master #6164 +/- ##
=======================================
Coverage 93.24% 93.25%
=======================================
Files 277 278 +1
Lines 10828 10832 +4
Branches 895 895
=======================================
+ Hits 10097 10101 +4
Misses 602 602
Partials 129 129
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
Closes #5376
Closes #5289
I have:
Description
Should be two PRs really, but the changes are both small, and vaguely related...
events.location_ids
, which records the first and last date each location ID appeared in the CDR data (per event type).The
events.location_ids
table is created (if not exists) by the FlowETL task, rather than being defined in the FlowDB provisioning scripts. This is advantageous if we're updating FlowETL in an existing FlowKit deployment, since we don't have a formal migration process so would otherwise need to remember to manually create the table, but does mean the full specification of the FlowDB schema begins to spill out beyond theflowdb
sub-repo, which is perhaps not the cleanest approach. I'm not sure whether this is the right decision.