-
Notifications
You must be signed in to change notification settings - Fork 66
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
Allow to specify source database instead of defaulting to bronze database in __get_silver_dataflow_spec_dataframe #34
base: main
Are you sure you want to change the base?
Conversation
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
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.
Please fix Linting errors
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
- Coverage 90.28% 89.95% -0.34%
==========================================
Files 8 8
Lines 803 806 +3
Branches 149 151 +2
==========================================
Hits 725 725
- Misses 31 32 +1
- Partials 47 49 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@rtdtwo, Needs to add unit tests for added new lines as per above code coverage report. |
} | ||
if f"bronze_database_{env}" in onboarding_row: | ||
source_details = { | ||
"database": onboarding_row["bronze_database_{}".format(env)], |
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.
Why not use f"bronze_database_{env}"
as in if
above (which IMHO is more pythonic)?
Issue
The FAQs describes that any data source can be used for silver tables. However, the code always tries to find the bronze database from the onboarding rows, and there's no explicit mention of a source database other than a bronze database.
Fix
Change the code to accomplish the following:
bronze_database_{env}
if the field is found in onboarding rows.source_details
.