Skip to content
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

attributes migration #609

Merged
merged 9 commits into from Jan 16, 2019
Merged

attributes migration #609

merged 9 commits into from Jan 16, 2019

Conversation

robkooper
Copy link
Member

@robkooper robkooper commented Nov 30, 2018

Add a new table that allows for attributes in json form #505
Add dump/load bety scripts from PEcAn to BETY #597

Add a new table that allows for attributes in json form #505
This moves the load/dump scripts from PEcAn to BETY (fixes #597)

Added attributes table to dump/load script.
@dlebauer
Copy link
Member

dlebauer commented Dec 4, 2018

@gsrohde has Rob addressed your concerns and can this be merged?

@robkooper
Copy link
Member Author

Waiting on merging this issue to see if we want to rush this before AGU

@robkooper robkooper changed the title attributes migration {wip} attributes migration Dec 4, 2018
@gsrohde
Copy link
Contributor

gsrohde commented Dec 4, 2018

@dlebauer Rob addressed one minor coding nit that I flagged. Regarding the larger concern having to do with lack of constraints on the value column, I don't think I really am in a position to weigh in on this question. I'm not versed in the theory or philosophy of using relational databases in this way, and I wasn't in on any discussion of what the motive was for adding this table and how it would be used. From what I could glean from the simple fact that this is called an attributes table, I take it that this was seen as a method for adding "sparse" attributes to various database relations in an ad hoc way. If this is the case, I would think there would at least be the requirement that the value be a JSON object (as opposed to an array or a primitive) with the keys serving as the attribute names and perhaps no restriction on the values.

I'm also guessing that this new table was motivated by needs coming from PEcAn and that the BETYdb web app will simply provide the migration and will otherwise ignore these changes. Under this assumption, I'm going to go ahead and click Approve changes since I think the changes are largely out of the scope of both my expertise and what I am in charge of.

@gsrohde
Copy link
Contributor

gsrohde commented Dec 4, 2018

@robkooper @dlebauer Actually, I do have one concern not related to any previously expressed concerns—namely, that this requires at least PostgreSQL 9.4 and the official documentation only calls for version 9.3 or did until very recently. (I somehow can access the technical documentation link, so I can't check what it says now.) I think all the deployments I work with—EBI, bety-mepp, and terra-ref—now are using 9.4 or later, but I don't know if this is the case with all the other public instances.

@robkooper
Copy link
Member Author

https://github.com/PecanProject/betydb-documentation/blob/master/installing_betydb/installing-betydb-web-application.md talks about PostgreSQL 9.3 or greater. other places in the documentation talks about postgresql 9.4

robkooper added a commit to PecanProject/betydb-documentation that referenced this pull request Dec 5, 2018
@gsrohde
Copy link
Contributor

gsrohde commented Dec 5, 2018

@robkooper Yes, I noticed that, when the site eventually came up for me. I also made this change on the bookdown branch of the documentation, which will eventually replace the master branch when we deploy the bookdown version to some official place. (It can't replace it now, because GitBook will choke on some of the bookdown syntax.) If you think it make sense to, I'll rename the bookdown branch to develop.

@robkooper robkooper changed the title {wip} attributes migration attributes migration Dec 13, 2018
@robkooper robkooper merged commit 8d4d62f into develop Jan 16, 2019
@robkooper robkooper deleted the attributes_table branch January 16, 2019 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants