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

Implement upload file #59

Merged
merged 6 commits into from
May 5, 2024
Merged

Conversation

hwrdtm
Copy link
Contributor

@hwrdtm hwrdtm commented May 1, 2024

What

This PR:

  • Implements the ability to copy file contents from locally to remote using SFTP

Misc

Related to #14

@hwrdtm
Copy link
Contributor Author

hwrdtm commented May 1, 2024

@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
Copy link
Owner

@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.

  • Add unit tests.
  • Fix linter warnings.
  • Tidy up dependencies.
    • If possible, I think it's better to avoid using "beta" unless necessary.
    • Also, specifying just "1" for the Tokio version seems better. Alternatively, consider leaving Tokio as a dev-dependency for now.
      • If Tokio has been added to dependencies, it's better to use the asynchronous version provided by Tokio rather than std::fs::read.

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
Copy link
Contributor Author

hwrdtm commented May 1, 2024

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
Copy link
Owner

Miyoshi-Ryota commented May 1, 2024

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
Copy link
Contributor Author

hwrdtm commented May 1, 2024

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

@Miyoshi-Ryota
Copy link
Owner

@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
Copy link
Owner

Miyoshi-Ryota commented May 1, 2024

@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
Copy link
Contributor Author

hwrdtm commented May 1, 2024

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
Copy link
Owner

@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.

  • specifying tokio="1" allows tokio version over v1.0.0
  • specifying tokio="1.14.0" allows the tokio version over v1.14.0.

...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 Miyoshi-Ryota self-requested a review May 1, 2024 07:23
@Miyoshi-Ryota Miyoshi-Ryota marked this pull request as ready for review May 5, 2024 02:13
@Miyoshi-Ryota Miyoshi-Ryota merged commit 49aacfd into Miyoshi-Ryota:main May 5, 2024
2 checks passed
@Miyoshi-Ryota Miyoshi-Ryota mentioned this pull request May 5, 2024
@Miyoshi-Ryota
Copy link
Owner

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

@hwrdtm
Copy link
Contributor Author

hwrdtm commented May 5, 2024

@hwrdtm

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants