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

Implement the small schema additions #207

Merged
merged 12 commits into from
Feb 12, 2019
Merged

Implement the small schema additions #207

merged 12 commits into from
Feb 12, 2019

Conversation

RKrahl
Copy link
Member

@RKrahl RKrahl commented Oct 9, 2018

This implements the "smaller" schema additions and thus closes #198, closes #199, and closes #201. As these schema changes are purely additive, it should not create any compatibility issues.

It is assumed that this PR will be merged before the release of icat.server 4.10.0. This version number is mentioned in the release notes, the installation instructions, and the file names of the database upgrade scripts.

@RKrahl RKrahl added this to the 4.10.0 milestone Oct 9, 2018
@RKrahl
Copy link
Member Author

RKrahl commented Oct 9, 2018

Note that I only added integration tests involving the new attributes for the REST API, but not for SOAP. As far as I understand things, I would need an icat.client 4.10.0 in order to write such tests for the SOAP API. But I need a running icat.server 4.10.0 in order to build an icat.client 4.10.0, so there is a kind of a circular dependency. I guess the proper path would be:

  1. create an icat.server 4.10 snapshot release from this PR branch,
  2. use that to build an icat.client 4.10 snapshot, and finally
  3. use the icat.client snapshot to add SOAP tests to icat.server and push that to this PR.

This could still be done before the final release of 4.10.0.

@stuartpullinger stuartpullinger self-assigned this Jan 22, 2019
@stuartpullinger
Copy link
Contributor

DOI vs PID

Apologies in advance as this is not my area of expertise but we have DOI fields on Investigation, DataSet, DataCollection and DataFile. You have created PID fields for Instrument, ParameterType and Sample. I understand that PID is the more generic term but shouldn't we be consistent? Won't we be storing the same type of information in these fields?

@RKrahl
Copy link
Member Author

RKrahl commented Jan 29, 2019

Good question. I guess there is no best solution. I would consider the attribute name doi for Investigation, Dataset, and so on as legacy. pid would have been the better name also there, because each site will in any case decide which kind of PID they prefer to use. In the end, the value of the attribute may be a DOI, but may very well also be a different kind of PID. At HZB, although this is not finally decided yet, I will probably use Handles and not DOIs for investigations and datasets. But I will store these Handles in the doi attribute anyway. Nevertheless, doi may be considered an acceptable name, because many sites actually do use DOIs here. In the case of ParameterType it is somewhat more questionable if DOIs are the appropriate kind of PIDs to use. In the case of Instrument it will certainly be the new instrument PID system that is currently in discussion to be established and that may or may not be based on DOIs. In the case of samples it may very well be IGSN in the end (it is currently being discussed to broaden the scope of IGSN to accommodate all possible kinds of samples). I suggest it's better not to give the false hint that DOIs would be the only kind of PID to be used.

But, in the end, it's an attribute name that is most likely never visible to the end user.

@RKrahl
Copy link
Member Author

RKrahl commented Jan 29, 2019

Note: the failing travis test is due to the fact that travis first installs an old version of icat.server and undeploys it (in order to setup the JDBC connection pool as a side effect), but fails to run the provided schema upgrade script on the old database. If I run the test locally in the proper manner, they succeed.

@stuartpullinger stuartpullinger merged commit bf076ba into master Feb 12, 2019
@RKrahl RKrahl deleted the schema branch February 12, 2019 11:22
@RKrahl RKrahl added the schema this involves changes to the ICAT schema label May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement review wanted schema this involves changes to the ICAT schema
Projects
None yet
2 participants