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

SshTransport.listdir pattern handling is wrong #5244

Closed
chrisjsewell opened this issue Dec 4, 2021 · 2 comments · Fixed by #5252
Closed

SshTransport.listdir pattern handling is wrong #5244

chrisjsewell opened this issue Dec 4, 2021 · 2 comments · Fixed by #5252
Assignees
@chrisjsewell
Labels
topic/transports type/bug

Comments

@chrisjsewell
Copy link
Member

Pretty sure this is erroneous:

aiida-core/aiida/transports/plugins/ssh.py

Lines 1277 to 1280 in 08ac107

filtered_list = glob.glob(os.path.join(base_dir, pattern))
if not base_dir.endswith('/'):
base_dir += '/'
return [re.sub(base_dir, '', i) for i in filtered_list]

since glob acts on local paths. It looks like it was just copied from _local_listdir.
I imagine the tests pass, on GH actions because the SSH is just set up to connect to the localhost

unless I'm missing something @giovannipizzi @sphuber?
@chrisjsewell chrisjsewell self-assigned this Dec 4, 2021
@sphuber
Copy link
Contributor

sphuber commented Dec 6, 2021

Never touched that part of the code, but I would agree that that seems incorrect.

giovannipizzi added a commit to giovannipizzi/aiida-core that referenced this issue Dec 7, 2021
@giovannipizzi
Fix listdir method with pattern for SSH
9b9260a 
The `listdir` method of the SSH transport plugin was using
`glob.glob` instead of `self.glob` when a pattern was specified.
This is wrong because it's listing files locally and not on the
remote.
The tests passed because they are running on localhost.
Also, this method is probably never called by AiiDA so it went unnoticed.

This commit fixes the issue. I'm not sure if there is a simple
way to fix the tests - probably we should run them against a
docker container or at least a chroot'ed folder? This might be
another issue to be opened though.

Fixes aiidateam#5244
@giovannipizzi giovannipizzi mentioned this issue Dec 7, 2021
Merged
@giovannipizzi
Copy link
Member

Thanks, you're right! I opened a PR to fix the bug.
As I mentioned in the commit, maybe in the future we should run the tests for the transport against a docker container - this however requires (a bit of) work to rewrite the way SSH tests are run probably...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisjsewell chrisjsewell

Labels
topic/transports type/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🐛 FIX: listdir method with pattern for SSH giovannipizzi/aiida-core
3 participants
@chrisjsewell @sphuber @giovannipizzi