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

SshTransport.gettree: allow non-existing nested target directories #4175

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 15, 2020

Fixes #4174

The gettree method would raise an OSError for the SshTransport if
the target local path contains intermediate subdirectories that are not
created beforehand. The same would work without problem for the
LocalTransport plugin. It makes sense to just create non-existing
directories along the way, so the os.mkdir command that was used
originally to create the target directory is replaced with the method
os.makedirs setting exists_ok=True.

The `gettree` method would raise an `OSError` for the `SshTransport` if
the target local path contains intermediate subdirectories that are not
created beforehand. The same would work without problem for the
`LocalTransport` plugin. It makes sense to just create non-existing
directories along the way, so the `os.mkdir` command that was used
originally to create the target directory is replaced with the method
`os.makedirs` setting `exists_ok=True`.
@sphuber
Copy link
Contributor Author

sphuber commented Jun 15, 2020

It is interesting that this went unnoticed. This came up during development of the aiida-quantumespresso-hp plugin, but I definitely would have thought that retrieving a folder to a nested local folder would not be such an exotic use case.

@codecov
Copy link

codecov bot commented Jun 15, 2020

Codecov Report

Merging #4175 into develop will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4175      +/-   ##
===========================================
- Coverage    78.89%   78.88%   -0.00%     
===========================================
  Files          467      467              
  Lines        34470    34470              
===========================================
- Hits         27191    27189       -2     
- Misses        7279     7281       +2     
Flag Coverage Δ
#django 70.81% <100.00%> (ø)
#sqlalchemy 71.68% <100.00%> (-<0.01%) ⬇️
Impacted Files Coverage Δ
aiida/transports/plugins/ssh.py 76.39% <100.00%> (ø)
aiida/engine/daemon/client.py 72.58% <0.00%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 216b4ec...9b105ca. Read the comment docs.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go for me!

@ramirezfranciscof
Copy link
Member

ramirezfranciscof commented Jun 15, 2020

The only thing is that when I try to test the transport locally I keep running into problems. This is clearly my issue because the test pass on the CI and this also happened before when I was trying to do some modifications in the transport, but do you have any idea of what may it be? I think all of the tests get the same error:

paramiko.ssh_exception.NoValidConnectionsError: [Errno None] Unable to connect to port 22 on 127.0.0.1 or ::1

(test the transport locally = pytest run the test/transports/test_all_plugins.py locally)

@sphuber
Copy link
Contributor Author

sphuber commented Jun 15, 2020

Seems like you might have a problem with the configuration of the openssh-server on your localhost. We are testing the SshTransport by connecting to localhost over ssh. You may have blocked the port 22 or maybe the server is not even running?

@sphuber
Copy link
Contributor Author

sphuber commented Jun 15, 2020

Thanks for the review. Let's wait with merging until @giovannipizzi has had a look to make sure I am not patching a problem that has its origin elsewhere

@sphuber sphuber merged commit f9d66b4 into aiidateam:develop Jun 17, 2020
@sphuber sphuber deleted the fix/4174/ssh-transport-gettree-nested branch June 17, 2020 13:36
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.

SshTransport.gettree fails when asking to create nested directory
3 participants