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 ssh_light transport plugin #6083

Closed
wants to merge 5 commits into from

Conversation

yakutovicha
Copy link
Contributor

@yakutovicha yakutovicha commented Jul 13, 2023

fixes #6082

@sphuber
Copy link
Contributor

sphuber commented Sep 17, 2023

Thanks @yakutovicha . Reading through the changes, I was wondering if, instead of creating a new class and trying to implement all methods of the Transport interface, maybe you can subclass from SshTransport and just override some of the methods that you have to in order to add fabric. I have the feeling that this might be possible, especially since fabric builds on top of paramiko and so essentially both transports anyway use the same lower-level library. This way you can keep the full implementation of SshTransport and keep the same behavior, while simplifying the interface and its usage. Did you try this approach by any chance?

@sphuber
Copy link
Contributor

sphuber commented Oct 12, 2023

I had a look at what I suggested above and it seems doable quite easily. See this branch: https://github.com/sphuber/aiida-core/tree/fix/transport-tests
I simply subclass SshTransport and replace the constructor, open and close methods, that's it. I also added a commit that refactors the transport tests such that it runs it for all plugins, and all those tests now pass. I think this approach is probably preferable because the SshTransport code is battle-tested and by simply reusing it, we get the same functionality/coverage while only simplifying the connection configuration.

Think the only thing remaining is to ensure that the verdi computer configure core.ssh_fabric now has no required options (maybe just optional tweaks where useful).

@sphuber
Copy link
Contributor

sphuber commented Oct 23, 2023

@yakutovicha could you please move this branch to your own fork? We are trying to keep the main repo clean w.r.t. branches and only have feature/fix branches if this is necessary for certain tests to run.

I have tested that verdi computer configure works as well for the SshFabricTransport plugin of this PR #6154 It now makes the configure as simple as that for the LocalTransport. People at PSI are now testing it for various SSH setups to check if it also works for more complex setups with proxies etc.

I am closing this PR for now. Let me know if you think there is a problem with the other implementation and this one is preferred.

@sphuber sphuber closed this Oct 23, 2023
@sphuber sphuber deleted the feature/ssh-light-transport-plugin branch December 21, 2023 16:44
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.

ssh_light - a simple ssh transport plugin
2 participants