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 #10822: add "local" options to omero.fs.repo.path_rules #1924

Merged
merged 6 commits into from Jan 21, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Dec 18, 2013

Make sure that the Blitz unit tests FilePathRestrictionsTest and MakePathComponentSafeTest still pass.

Try using local required and local optional in etc/omero.properties for omero.fs.repo.path_rules (remember ./build.py update-version before restarting the server; you know if your changes are being taken because if you put a nonsense setting then there will be a stack trace in Blitz-0.log). These should limit sanitization of import paths according to the machine upon which the server is running.

To see the extra log message from CurrentPlatform you may have to adjust levels in logback.xml.

To understand the path sanitization rules see http://www.openmicroscopy.org/site/support/omero5-staging/sysadmins/fs-upload-configuration.html#legal-file-names

Fixes https://trac.openmicroscopy.org.uk/ome/ticket/10822.
--no-rebase as FS-specific.

@ximenesuk
Copy link
Contributor

Results from FilePathRestrictionsTest on MacOS is all passes except for two skips.:

testUnsafeCharacterUnsafetyLinux    0.002s  
testUnsafeCharacterUnsafetyWindows  0.000s  

Given the OS I assume these skips are expected.

With MakePathComponentSafeTest I'm less certain since the following are skipped:

testPlatformTestExecuted    0.004s  
testUnsafeCharacterUnsafetyLinux    0.000s  
testUnsafeCharacterUnsafetyWindows  0.000s  

Should the first of these be skipped?

@mtbc
Copy link
Member Author

mtbc commented Jan 20, 2014

Yes, testPlatformTestExecuted is skipped if we have identified the platform (and one of the platform-specific tests executed instead): it contains a fail for if platform identification fails.

@ximenesuk
Copy link
Contributor

Thanks for the clarification. I am seeing (with appropriate log tweaking) a log line confirming the current platform. However, I have only tested this PR on Mac OS, does it require wider testing?

@mtbc
Copy link
Member Author

mtbc commented Jan 20, 2014

I'll have tested it on Linux in creating it, so I expect that we're probably okay. It's probably not worth the effort of testing the Windows side, unless perhaps the CI system is already giving us a merge-build running a Windows OMERO.server somewhere, or vbox image with one in it, in which case I could try to generate further testing suggestions.

@ximenesuk
Copy link
Contributor

I'm happy for this to be merged. /cc @joshmoore

@joshmoore
Copy link
Member

👍

joshmoore added a commit that referenced this pull request Jan 21, 2014
fix #10822: add "local" options to omero.fs.repo.path_rules
@joshmoore joshmoore merged commit e0e6297 into ome:develop Jan 21, 2014
@mtbc mtbc deleted the trac-10822-paths-for-system branch January 21, 2014 10:47
@sbesson sbesson modified the milestones: 5.1.0, 5.1.0-m1 Oct 14, 2014
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

4 participants