[Testing] Test SSH file transfer with images.#208
Merged
JoeZiminski merged 68 commits intomainfrom Jul 2, 2025
Merged
Conversation
7f9781b to
7d6add8
Compare
8d0f0a3 to
9442694
Compare
88591cb to
bafb5d0
Compare
f47884c to
b26163b
Compare
niksirbi
reviewed
Jun 10, 2024
Member
There was a problem hiding this comment.
My intermediate report on this:
- The ssh tests failed when locally running on MacOS (M2), with docker running
- The contributing guide should be updated to let contributors know how to handle local ssh tests (either skip them or configure their local machines accordingly).
7 tasks
cs7-shrey
reviewed
Mar 23, 2025
Collaborator
There was a problem hiding this comment.
Hey @JoeZiminski I tried the tests in this PR on linux (kali) and windows. With a few changes, the tests work as expected.
I have added some comments.
50ae318 to
c29e250
Compare
Closed
76b5e80 to
1958363
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
cs7-shrey
reviewed
Jun 25, 2025
Collaborator
cs7-shrey
left a comment
There was a problem hiding this comment.
Hey @JoeZiminski, this is really comprehensive and the overall structure is really great. I've added some review comments, please take a look.
Also,
- We can delete
tests/ssh_test_utils.pyas we have the same file now intests/tests_transfers/ssh - Do consider adding some type hints in the utility functions in
base_transfer.py, etc. At some places it was really hard to infer the data flow especially in the parts where the pandas Dataframe is being used.
Member
Author
|
Thanks a lot @cs7-shrey for the feedback have made all suggested changes! Let me know what you think |
cs7-shrey
approved these changes
Jun 27, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements SSH tests through a docker image. They key changes in this PR are:
code_test_and_deploy.ymlto selectively run SSH tests in linux runners. This is because the Windows and macOS runners already are in a container, so it's not possible to run a container from within these containers ('nested containerisation').canonical_configs.py. At the moment this is not documented / exposed to users. In general I don't think there will be much need to SSH from other ports except for 22. But it's possible it will be needed.Dockerfileis added that spins up an ubuntu image with SSH setup so that it can be SSHed into from tests.ssh_test_utils.pynow contains all machinery to SSH into the docker image.Note this required refactoring the test structing, and introducing relative imports into the test suite.
The rest of this PR is pretty much refactoring.
base_transfer.py,test_ssh_file_transfer.py,test_ssh_setup.pyjust contains the already-existing tests. The new additions is testing the SSH through images not through a configuration where paths etc. were entered into theconftest.py. However, any feedback on these tests is of course welcome.