V1.3.3.010 - Adding a fact table for environmental data

[guignonv]

Adding a fact table for environmental data as described in #107.

[scottcain]

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 :-)

[scottcain]

@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!

[guignonv]

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?

[laceysanderson]

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.

[scottcain]

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.

[scottcain]

Not only that, but with the implementation of #79 , the modules directories have gone away!

[spficklin]

I was going to review this PR but it has a conflict as it has the chado/modules/natural_diversity/natural_diversity.sql file in it. From the conversation above it sounds like that file needs to be dropped. That's an easy enough fix and I'm happy to fix the merge conflict but I want to double-check that removing the file is the right thing to do.

[laceysanderson]

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 :-)

[laceysanderson]

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.