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.

Sign up for GitHub

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

Jump to bottom

Provide replacement for SSHRemoteIO based on shell #655

Merged
merged 16 commits into from
Apr 25, 2024
Merged

Provide replacement for SSHRemoteIO based on shell #655

merged 16 commits into from
Apr 25, 2024

Conversation

mih
Copy link
Member

@mih mih commented Apr 17, 2024 •
edited
Loading

TODO

  • integrate patch ora_remote
  • implement tests for 7z functions
  • bring progress reports for downloads (waiting for Add progress-callback to datalad_next.shell.operations.posix.download #660)
  • resolve question of whether trailing newlines for file content are needed, still Provide replacement for SSHRemoteIO based on shell #655 (comment)
  • Download/upload throughput test. This is important to measure and report in the changelog
    -> Same throughput as scp
  • On Windows: 
    CommandError: '"C:\Windows\System32\OpenSSH\ssh.exe" "-p" "2222" "-i" "C:\DLTMP\datalad_tests_id_rsa" "-o" "SendEnv=GIT_PROTOCOL" "sshuser@datalad-test-sshd" "git-upload-pack '/usr/local/apache2/htdocs/af4/903ff-a427-4401-9863-d61c092a325e'"' failed with exitcode 128
fatal: Could not read from remote repository.

Ping #666

@codecov Codecov
Copy link

codecov bot commented Apr 17, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.26%. Comparing base (5a12b73) to head (be72a05).

❗ Current head be72a05 differs from pull request most recent head ecb516e. Consider uploading reports for the commit ecb516e to get more accurate results

Files Patch % Lines
datalad_next/tests/utils.py 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
+ Coverage   92.47%   93.26%   +0.78%     
==========================================
  Files         183      178       -5     
  Lines       12346    12510     +164     
  Branches     1890     1890              
==========================================
+ Hits        11417    11667     +250     
+ Misses        727      651      -76     
+ Partials      202      192      -10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mih mih mentioned this pull request Apr 17, 2024
Merged
@mih mih force-pushed the ssh-shell branch 2 times, most recently from f65050c to ecb516e Compare April 18, 2024 10:55
@mih mih changed the title Fork of #596 to test addons Provide replacement for SSHRemoteIO based on shell Apr 18, 2024
@mih mih mentioned this pull request Apr 18, 2024
Closed
This was referenced Apr 23, 2024
Closed
Closed
mih and others added 15 commits April 25, 2024 08:56
@mih
Start with no-op patch
3b010ed 
Imports `SSHRemoteIO` implementation from datalad v1.0.1
@mih
Start testing and fixing things up inSSHRemoteIO
0ba8c81 
Now it cd's into a root directory it has been given, rather than
assuming all paths are always absolute.
@mih
Somewhat comprehensive tests for SSHRemoteIO (minus archive handling)
ec4f3aa
@mih
Reimplement SSHRemoteIO with datalad_next.shell
ce1ee47 
This takes out all of the old remote shell implementation, and uses the
new one. It does not touch the get/put implementations (yet). They
can also be done with the new shell feature, but that is a different
problem.
@mih
Replace the sshremoteio patch in favor of the full reimplementation
1f16745
@mih
Replace scp for SSHRemoteIO with shell operation upload
d64b622 
This revealed a missing quoting of paths in the remote command, and is
fixed.
@mih
Replace SSHRemoteIO.get implementation with shell operation `down…
dbd4649 
…load`

This is not complete, because it does not do progress reporting yet.
See datalad#659
@mih
Reimplement the remaining pieces of SSHRemoteIO with shell
1f8c3df 
In particular the archive handling.
@mih
Force-appending newlines to content is no longer needed
56de939
@mih
Resolve ora_remote patch, merge with reimplementation
6d58d21
@mih
Support progress callback for SSHRemoteIO.get()
0429ac2
@mih
Enhance SpecialRemote with close()
03b4760 
A standard handler for releasing resources, compatible with
`contextlib.closing()`.
@mih
Add patch to make ORARemote call SSHRemoteIO.close() on exit
9c6f5cd 
This is using the `close()` handler introduced with the previous commit.
@christian-monch @mih
fix parameter types for upload & download
49a4ff4 
This commit ensures that the `dst`-parameter
to `datalad_next.shell.posix.download` and
the `src`-parameter in
`datalad_next.shell.posix.upload` are of
type `Path`.
@christian-monch @mih
import unquote from urllib.parse
c8de397 
This commit fixes a mypy-error.
@mih
Copy link
Member Author

mih commented Apr 25, 2024

I have rebased this branch on main and taken out all windows-related ambitions in the test. The old state is available in the ssh-shell-windows branch.

I am taking this PR out of draft mode. It is a worthwhile update in the current form. Further (windows-related) update should come in separate PRs.

@mih mih marked this pull request as ready for review April 25, 2024 07:04
@mih mih merged commit 278081c into datalad:main Apr 25, 2024
3 of 5 checks passed
@mih mih deleted the ssh-shell branch April 25, 2024 07:23
@mih mih added this to the 1.4 milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mih mih

@christian-monch christian-monch

Labels
None yet
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
2 participants
@mih @christian-monch