-
Notifications
You must be signed in to change notification settings - Fork 15
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
chore: remove create and read fhir bundle from postgres #3137
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3137 +/- ##
==========================================
+ Coverage 87.03% 89.39% +2.35%
==========================================
Files 221 84 -137
Lines 13664 4204 -9460
Branches 708 716 +8
==========================================
- Hits 11892 3758 -8134
+ Misses 1763 426 -1337
- Partials 9 20 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -3,7 +3,6 @@ CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; | |||
CREATE TABLE ecr_data ( | |||
eICR_ID VARCHAR(200) PRIMARY KEY, | |||
set_id VARCHAR(255), | |||
data_source VARCHAR(2), -- S3 or DB |
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.
not really a question on this PR, but more generally. When we make a schema change like this, what's the migration plan for users?
values: [ | ||
ecrId, | ||
metadata.last_name, | ||
metadata.first_name, | ||
metadata.birth_date, | ||
"DB", |
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.
I'm trying to follow along on what this was doing in the first place. Was this correct? Couldn't we have saved the data to an S3 bucket, but saved the metadata to postgres?
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.
also, why don't I see a similar change in the sql server code?
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.
I believe this was supposed to define where to find the FHIR_DATA for the ecr-viewer (s3 or azure blob), but we only have ever supported fetching the data from a single source.
For your second question, this is only affects postgres because the CORE schema only supports postgres.
@austin-hall-skylight, do you have any thoughts about keeping the data_source
variable?
9f074e5
to
3dacdb5
Compare
3dacdb5
to
62bdd0d
Compare
PULL REQUEST
Summary
/save-fhir-data
Related Issue
Fixes #2406