Miyoshi-Ryota / async-ssh2-tokio

asynchronous and easy-to-use high level ssh client library for rust
Apache License 2.0
71 stars 27 forks source link

Implement upload file #59

Closed hwrdtm closed 6 months ago

hwrdtm commented 6 months ago

What

This PR:

Misc

Related to https://github.com/Miyoshi-Ryota/async-ssh2-tokio/issues/14

hwrdtm commented 6 months ago

@Miyoshi-Ryota how does this draft look to you?

I could write unit tests and also use buffers, but don't have much time 😅

Miyoshi-Ryota commented 6 months ago

@hwrdtm

Comment

Thanks for your work. It's been excellent. There are a few tasks we need to address before releasing. Some you might already be aware of.

If you don't have time to write unit tests, etc., I can merge this PR. And I'll take care of these tasks on this weekend.

Question

also use buffers

By the way, what does this sentence mean? Where are the areas for improvement assumed?

hwrdtm commented 6 months ago

Question

also use buffers

By the way, what does this sentence mean? Where are the areas for improvement assumed?

right now we load the entirety of the local file into memory https://github.com/Miyoshi-Ryota/async-ssh2-tokio/pull/59/files#diff-7f93c4e263c4e9ec748f804c7fd04a3b2fde86ffd741fb5516d67e1097bae4c1R275

we should use buffers to avoid overloading the memory space and for a more "streaming" based approach

Miyoshi-Ryota commented 6 months ago

I completely understood, thanks.

The buffer issue can be modified later without destroying the interface, and I think it can be merged as is.

hwrdtm commented 6 months ago

@Miyoshi-Ryota how do I replicate the linter errors? When I run cargo clippy locally everything is fine.

Miyoshi-Ryota commented 6 months ago

@Miyoshi-Ryota how do I replicate the linter errors? When I run cargo clippy locally everything is fine.

Using rust version as 1.69, rust clippy throws this warnings. is_some_and is already stable after v1.70. So we should fix this CI's lint error by modifying CI environment instead of source code.

% rustup install 1.69
<-snip->
% rustup default 1.69.0-aarch64-apple-darwin
<-snip->
% cargo clippy
<-snip->
error[E0658]: use of unstable library feature 'is_some_and'
   --> /Users/miyoshi/.cargo/registry/src/github.com-1ecc6299db9ec823/russh-sftp-2.0.0-beta.4/src/client/rawsession.rs:242:14
    |
242 |             .is_some_and(|h| self.handles >= Wrapping(h))
    |              ^^^^^^^^^^^
    |
    = note: see issue #93050 <https://github.com/rust-lang/rust/issues/93050> for more information
Miyoshi-Ryota commented 6 months ago

@hwrdtm Updating super-linter may lead new lint errors. I accept just disabling it for now.

        env:
          VALIDATE_SHELL_SHFMT: false
          VALIDATE_CHECKOV: false
hwrdtm commented 6 months ago

If possible, I think it's better to avoid using "beta" unless necessary.

The russh_sftp::client mod seems to only be introduced in the recent beta versions though :/

Also, specifying just "1" for the Tokio version seems better.

Would you mind explaining why this is better?

Miyoshi-Ryota commented 6 months ago

@hwrdtm

The russh_sftp::client mod seems to only be introduced in the recent beta versions though :/

Ok, I understood.

Would you mind explaining why this is better?

specifying "tokio=1" provides a more flexible choice of tokio version for the library user.

...However, russh specifies tokio="1.17.0". So specifying tokio=1 and tokio=1.14.0 leads completely same result that 1.17.0 or higher tokio is only acceptable to build my crates. So, I leave the decision of tokio=1 or 1.14.0 to you.

Miyoshi-Ryota commented 6 months ago

@hwrdtm I've released your work as version 0.8.9. I'm sorry to late release.

hwrdtm commented 6 months ago

@hwrdtm

I've released your work as version 0.8.9. I'm sorry to late release.

Thank you!