-
Notifications
You must be signed in to change notification settings - Fork 599
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
Added SFTP file transfer resume support on both PUT and GET. #775
Conversation
…ly SFTPFileTransfer has a few sanity checks to fall back to full replacement even if the resume flag is set. SCP file transfers have not been changed to support this at this time.
Please also write some tests |
Done! |
1 comment :) |
You need to add the .txt file to the ignore list of the license plugin in the build. |
I'm not as familiar with groovy scripting or license checks. I added the license header to the new JUnit test class which was missing and changed the license.excludes list to be the following. Will that work to exclude both of the test resources?
|
Apparently that last commit created a security/snyk issue but I can't see the details. |
…ch scenario are the actual bytes transferred.
Just curious what the thoughts on this PR are now and what else I can do to help move it forward? |
I've been thinking about the API that you've added. I'm not entirely sure I'm comfortable with a plain boolean and the "magic" underneath. I'm thinking more along the lines of having the read/write interfaces expose an offset at where to start writing/reading. That would make the API a bit more versatile and less magic. |
Yeah, I can do that. Probably in my excitement to get it working I put too much of my use case into that but we should leave it up to the users of this API to know what they're asking for. They already have the tooling to discover what's on the destination already - the existence, current length, and last modified timestamp - with which to make informed offset decisions. I'll work on that but it may be a few days. |
I fully understand, and I do get the usecase, but as you also pointed out, all the bits to make it work are there, except for the "starting at a particular offset". Making the API more reusable will actually allow more usecases to be implemented. |
… internal decisions to be a user-specified long byte offset. This is cleaner but puts the onus on the caller to know exactly what they're asking for in their circumstance, which is ultimately better for a library like sshj.
API changes made, tested, validated, and pushed for approval! |
Anything else I can do to help finalize this PR? We've been running it in production for a while now with no issues. |
Codecov Report
@@ Coverage Diff @@
## master #775 +/- ##
============================================
+ Coverage 64.33% 64.84% +0.51%
- Complexity 1460 1481 +21
============================================
Files 211 211
Lines 8622 8660 +38
Branches 809 812 +3
============================================
+ Hits 5547 5616 +69
+ Misses 2650 2616 -34
- Partials 425 428 +3
Continue to review full report at Codecov.
|
Thanks for the nudge, I've approved and merged it! 👍 |
Thanks! |
Internally SFTPFileTransfer has a few sanity checks to fall back to full replacement even if the resume flag is set.
SCP file transfers have not been changed to support this at this time.