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

fix #11877: move import logs to upload jobs instead of annotations #2055

Merged
merged 10 commits into from Feb 7, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Feb 2, 2014

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/11877 to make import logs attached to a fileset's upload job instead of being a file annotation on it; the import log's original file has a distinct MIME type. The upgrade script from RC1 now includes PL/pgSQL for moving legacy import logs accordingly.

Ensure that the new integration tests run and pass in http://ci.openmicroscopy.org/job/OMERO-5.0-merge-integration-java/lastCompletedBuild/testngreports/integration/integration.MetadataServiceTest/.

In testing, make sure that import logs can still be viewed from Insight, and that they indeed no longer exist as annotations in the database.

@sbesson
Copy link
Member

sbesson commented Feb 2, 2014

@mtbc: what is the impact of this PR in terms of upgrade from 5.0.0-rc1 to 5.0.0? In terms of upgrade documentation?

@snoopycrimecop
Copy link
Member

Conflicting PR.Removed from build OMERO-5.0-merge-daily#564. See the console output for more details.

@mtbc
Copy link
Member Author

mtbc commented Feb 3, 2014

@sbesson: I don't think this is worth any upgrade documentation. The system will work apparently the same with or without the upgrade script, and for good measure the same upgrade will be included in the 5.0.0 → 5.0.1 script. Clients don't currently offer access to "old" import logs so if some older imports can't be found, through the script not having yet been run, nobody will notice. Also, this PR doesn't remove clients' not listing old-style import logs among the file annotations.

The conflict is with #2043 -- we both add the same import -- so I will wait for that to be merged, then rebase, fix, and force-push.

@@ -607,6 +607,56 @@ UPDATE image set archived = false where id in (
AND i.archived
AND NOT EXISTS ( SELECT 1 FROM pixelsoriginalfilemap m WHERE m.child = p.id));


-- #11877 move import logs to upload jobs so they are no longer file annotations
CREATE FUNCTION upgrade_import_logs() RETURNS void AS $$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it hurts, but I don't think this situation can occur: there would never be FS logs in a 4.4 DB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, excellent, maybe I should just omit this then.

@will-moore
Copy link
Member

With a local build and server, import logs showed up in Insight fine, and there were no new entries in the filesetannotationlink table. Looks good!

@snoopycrimecop
Copy link
Member

Conflicting PR.Removed from build OMERO-5.0-merge-daily#565. See the console output for more details.

@joshmoore
Copy link
Member

2043 is merged, @mtbc, if you want to rebase and kick off the daily build.

@bpindelski
Copy link

Functionally, this works as expected. Code seems fine, but (as with any big PR), I can only give a +1 (on the gerrit scale) - I'm not sure if there are any subtle implications that I'm missing.

Another thing for viewing log files under Ubuntu - it doesn't work. Clicking the "View" button and then selecting the log file in the list only brings up an "Open file" modal and nothing happens. I wonder if it's some Java UI weirdness on Linux or is it connected to the MIME type? I'd expect the log file to open in a text editor, as it happens on OS X.

@mtbc
Copy link
Member Author

mtbc commented Feb 4, 2014

Thanks for trying it. Which JVM? @jburel, any thoughts?

@jburel
Copy link
Member

jburel commented Feb 4, 2014

The default text editor should be used unless none is set. I have started to modify the Open With section to integrate FLIMfit and other applications. I will do further tests. Not related to that PR.

@bpindelski
Copy link

@jburel On my Ubuntu the default text editor is gedit and works as expected.
@mtbc Oracle JDK 7u51.

This definitely isn't in the scope of this PR, so I'm done.

@mtbc
Copy link
Member Author

mtbc commented Feb 5, 2014

Deletion of filesets is broken due to the new original file link on the upload job.

@mtbc
Copy link
Member Author

mtbc commented Feb 5, 2014

The delete tests should now be fixed.

joshmoore added a commit that referenced this pull request Feb 7, 2014
fix #11877: move import logs to upload jobs instead of annotations
@joshmoore joshmoore merged commit d243708 into ome:dev_5_0 Feb 7, 2014
@mtbc mtbc deleted the trac-11877-import-log branch February 7, 2014 12:59
@mtbc
Copy link
Member Author

mtbc commented Feb 7, 2014

@joshmoore: Should this be rebased, or is there a different plan for how import logs should be linked in 5.1? Or perhaps that plan is easier to reach after a rebase of this.

@joshmoore
Copy link
Member

Definitely rebase. Even if there were a different strategy, we should have this in place for the moment.

@mtbc
Copy link
Member Author

mtbc commented Feb 11, 2014

Given that #1742 also upgrades to OMERO5.1DEV__0 I will try to work out with @sbesson how best to rebase this in the new breaking builds workflow; maybe waiting a few days would help. (Also, @qidane will also have database changes, I'd guess.)

@qidane
Copy link
Contributor

qidane commented Feb 11, 2014

@mtbc yes, db changes were the next item on my list, and I was going to ask you for some pointers.

@mtbc
Copy link
Member Author

mtbc commented Feb 12, 2014

Once #1742 is merged will rebase against that.

@mtbc
Copy link
Member Author

mtbc commented Mar 14, 2014

Now awaiting #2163.

@mtbc
Copy link
Member Author

mtbc commented Mar 21, 2014

--rebased-to #2191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants