-
Notifications
You must be signed in to change notification settings - Fork 25
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
V1.3.3.010 - Adding a fact table for environmental data #108
Conversation
Adding a fact table for environmental data as described in GMOD#107.
This commit/pull request isn't compatible with the flyway build system, so the pull request as currently formatted is destined to be rejected. Since it's the addition of a new table, this is a medium change, so it will require four reviews including at least one non-tripal reviewer. On the upside, since it is "just" adding a table, writing the flyway migration should be easy (written as someone who hasn't actually done it yet :-) |
@guignonv it would be great if you could rework this as a database patch in the "migrations" directory, naming it along the same lines as other files in there (so, something like "V1.3.3.009__add_environmental_fact_table.sql"). Also, putting "V1.3.3.009" in the title of the pull request would be helpful. Thanks! |
…f1db050cedbf31c6 about adding timecapturedend column.
Following yesterday conversation on gather.town, I've merged Chado branch chado/1.4 into this PR (commit e5cfe8a) to include a new migration file V1.3.3.010__add_environmental_fact_table.sql (commit e91dc32). Then, I've taken into account the modifications made by @scottcain (commit 8d3f75d) following @laceysanderson comment to add a "timecapturedend" column and included them both in the Natural Diversity module and the migration script. So this PR should be merged to the chado/1.4 branch. Did I forget anything? |
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 PR looks good from a quick code-review perspective 👍
My only comment is that I think the chado/modules/natural_diversity/natural_diversity.sql file should be removed from this PR as it's not part of the new Flyaway migrations approach.
Yeah, the maintenance or not of the chado/modules/ directories is an annoying problem: I would like to keep them around, as they are an easy place to keep things organized by module, so if I want to look for something nd-specific, I know where to find it. The problem is that there will be guaranteed code drift. |
Not only that, but with the implementation of #79 , the modules directories have gone away! |
I was going to review this PR but it has a conflict as it has the |
I just talked with @scottcain in Gather.town and he agrees @spficklin that the right approach here is to just remove the natural_diversity.sql file from this PR and then we can carry on with reviews as normal. So I will remove the file right away :-) |
Closing this PR in favour of #137 which uses a branch from this repository for easier testing and resolves the merge conflict. All review should occur there. |
Adding a fact table for environmental data as described in #107.