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

Remove command #4470

Closed
wants to merge 7 commits into from
Closed

Remove command #4470

wants to merge 7 commits into from

Conversation

jburel
Copy link
Member

@jburel jburel commented Feb 9, 2016

Use nio package to create hard link and symlink

Test the various in-place import options on several OS.

cc @joshmoore @mtbc @pwalczysko

@joshmoore
Copy link
Member

The error code handling from exec will need to be resurrected as exception handling. Specifically the error messages were requested so that users could figure out what was going on.

Also, since createProcessBuilder is protected, this would count as a breaking change since it would no longer do anything for subclasses which implement the createProcessBuilder method. Options I can think of:

  • keep createProcessBuilder but return an implementation which calls the native Java method
  • leave AbstractExecFileTransferas it is, and implement add new classes (Hardlink2FileTransfer extends AbstractFileTransfer` (that may require moving some of the methods from AEFT up to AFT)

@jburel
Copy link
Member Author

jburel commented Feb 9, 2016

@joshmoore: thanks I was not sure about the plan for error handling since the method already returns IOException (not handled)

I will see what I can do. I might add new classes

@jburel
Copy link
Member Author

jburel commented Feb 9, 2016

I have introduced new classes. I opted for a new AbstractExecFileTransfer2.
The simplest thing might be to mark some classes deprecated this round and use 5.3 for an update.
1.6 support was dropped a while back and those classes never got cleaned up.

@jburel
Copy link
Member Author

jburel commented Feb 10, 2016

Sorry I did not have a chance to run the tests. I will try to do that this pm

@jburel
Copy link
Member Author

jburel commented Feb 10, 2016

Another example of testing headache (plenty with Ice 3.6)
run locally
test/integration/clitest/test_import.py::TestImport::testSymlinkImport PASSED

@sbesson is the workspace cleaned daily? since the failure is due to a "FileAlreadyExistsException"
and I cannot reproduce it locally

@jburel
Copy link
Member Author

jburel commented Feb 10, 2016

@joshmoore @sbesson:
run the tests on breaking with this PR included
no problem see https://ci.openmicroscopy.org/view/OMERO-DEV/job/OMERO-DEV-breaking-integration-python/491/

@sbesson
Copy link
Member

sbesson commented Feb 10, 2016

@jburel: interestingly this PR was not picked in the merge job - see https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-breaking-push/623/ and snoopycrimecop@2951b97

@jburel
Copy link
Member Author

jburel commented Feb 10, 2016

It did not pick when I removed the exclude label
I am going to run it again

@jburel jburel removed the exclude label Feb 10, 2016
@jburel
Copy link
Member Author

jburel commented Feb 11, 2016

Few errors on breaking. I will investigate when I get back since the Symlink test passes locally
Same problem with the fs test currently failing.

@jburel jburel added the exclude label Feb 17, 2016
@jburel
Copy link
Member Author

jburel commented Feb 17, 2016

Tested locally symlink creation with both Java 1.7 and 1.8 no problem noticed

@jburel jburel added exclude and removed exclude labels Feb 17, 2016
@jburel
Copy link
Member Author

jburel commented Mar 23, 2016

The difference between local and server is the java version installed. I will run some tests on docker now that we have the various parameters in place.
Closing this PR for now.

@jburel jburel closed this Mar 23, 2016
@jburel jburel deleted the ln-cleanup branch September 25, 2018 13:47
@jburel jburel restored the ln-cleanup branch September 25, 2018 13:48
@jburel jburel deleted the ln-cleanup branch June 10, 2019 13:23
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