-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix #11663, #11664, #11877: SQL DOMAIN; import logs (rebased); FS delete log trigger #2191
Conversation
omitting x- prefix for http://tools.ietf.org/html/bcp178
to repeat in OMERO5.0__1 for those already on OMERO5.0__0
Conflicts: components/blitz/src/ome/formats/importer/ImportLibrary.java
Breaking label added. |
if (!getNullable()) { | ||
sb.append(" not null"); | ||
} | ||
if (getUnique()) { |
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.
Where are these UNIQUE
and NOT NULL
handled?
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.
I too had wondered how they still take effect. I believe it is through getUnique()
and getNullable()
being accessed at http://github.com/openmicroscopy/openmicroscopy/blob/develop/components/dsl/resources/ome/dsl/object.vm#L1006.
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.
I had removed them partly because, with them included, schema.sql
gets a number of not null not null
(sic) definitions.
There is pleasing greenness at http://ci.openmicroscopy.org/view/Breaking/. |
Import, viewing and deleting images works fine. Noticed that import logs when viewed in Insight all terminate with "Step 4" as the last line, but this is also true in the 'latest' build on trout, so it's nothing to do with this PR. |
Well, the latest run of http://ci.openmicroscopy.org/view/Breaking/job/OMERO-5.1-breaking-integration-python/lastCompletedBuild/testReport/ failed but the same test failed in http://ci.openmicroscopy.org/view/5.1/job/OMERO-5.1-merge-integration-python/lastCompletedBuild/testReport/ without this PR: the usual "No valid permissions available!" gremlin. |
Asked offline by @sbesson, I don't have any objections to this. I'm slightly worried about the drop of the |
In the conditional it was applying only for floats anyway; there is also, say, column |
In general we should have some testing (maybe a CI node) with minimum versions of prerequisites installed, not just PostgreSQL. |
Looks good! |
|
Need some investigation. Relabelled as |
Somebody had actually attached import logs to an image! The upgrade script is now fixed to anticipate such strangeness. |
--rebased-to #2203 upgrade script change in latest commit |
As discussed in devteam, this is related to some kymograph images with a zero p.physicalsizey value. Another edge-case to handle as part of this script (nullify the illegal values as suggested by @mtbc?) Additionally, we should fix our kymograph script to prevent zero-valued physical sizes to be set /cc @will-moore |
Nulling done in latest commit and http://trac.openmicroscopy.org.uk/ome/ticket/12128 filed about kymograph script. |
|
I've checked the DB upgrade; importing, viewing, deleting and image; viewing the import log; the relevant integration test and the relevant tables in the DB. Everything looks good and assuming everyone is otherwise happy with this PR having been in the breaking job up until now then I'd say this is good to merge. |
Waiting for @joshmoore and/or @jburel to sign-off on this PR. OMERO-5.1-latest-deploy will need to be ugpraded. |
👍 for this. Leaving @sbesson to run the upgrade script. |
fix #11663, #11664, #11877: SQL DOMAIN; import logs (rebased); FS delete log trigger
|
At least it ran smoothly that time! Thank you all. |
--rebased-from #2055 |
Fixes:
Check that everything still seems to work: can import, view, delete images; can view import logs.
Breaking so needs label from @sbesson? Also @qidane should glance at some of the changes.