Add delete method to sftp client #772
Conversation
Diogenesoftoronto
force-pushed
the
dev/sftp-delete-method
branch
from
15fed7e
to
5edc8d8
Compare
There was a problem hiding this comment.
@Diogenesoftoronto overall this looks good, but I've requested a few changes.
internal/sftp/goclient.go
Outdated
| // closed when the delete is complete or cancelled. | ||
| // | ||
| // Delete is not thread safe. | ||
| func (c *GoClient) Delete(ctx context.Context, dest string) (int64, error) { |
There was a problem hiding this comment.
I'm realizing that we can't use ctx for cancellation, but I suggest to leave it in case we find a way to use it in the future. I think that both the ssh and sftp packages that we're using predate the context package so they were not designed to accept a context. One alternative that we could consider is to pass the context to the dial method and create our own dialer with net.DialContext, but I don't see it as a blocker.
There was a problem hiding this comment.
rclone is doing it here, but it looks complicated because they support edge cases like proxy settings, etc. It also looks like the ssh package is going to have a DialContext sometime soon: https://go-review.googlesource.com/c/crypto/+/504735.
There was a problem hiding this comment.
Huh, interesting @sevein. I thought ctx would be unnecessary for delete because the actual sftp.Remove() operation should be very fast, AFAIK. But now I realize that dialing the server could take a while if there are communications issues, so I agree ctx is still worthwhile for future use.
There was a problem hiding this comment.
One thing I have considered is doing something similar to what contextio does, or wwe can do @sevein suggestion. I think including context on the actual client struct makes sense and then we can context is equal for operations that normally do not have a context like in contextio
Diogenesoftoronto
force-pushed
the
dev/sftp-delete-method
branch
2 times, most recently
from
073d050
to
8c976d2
Compare
- add second arg to want error for second method - split tests, handle non-existent or insufficient perms - improve comment, and remove results - generate mocks for delete
Diogenesoftoronto
force-pushed
the
dev/sftp-delete-method
branch
from
8c976d2
to
7460ed5
Compare
No description provided.