-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update db tables #68
Update db tables #68
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.
this is good to after you update the app.config["TESTING"]
line
Compare is telling me maybe we should just merge this one as it's got the superset of changes |
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.
looks good
same as other PR, we should get @Jin-Sun-tts or @FuhuXia to review so we get fresh eyes. |
The merge-base changed after approval.
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.
This looks like it's contained in #72, but looks good!
Pull Request
Related to #4731
About
update database tables to reflect what's in the definition document. remove use of sqllite in testing environment because it doesn't support arrays as a dtype which we need for notification_emails in the harvest source table
PR TASKS