Conversation
| # Get SFTP connection details from keyvault | ||
| self.host = self.keyvault.fetch_secret(f"{az_prefix}--sftp--host") | ||
| self.username = self.keyvault.fetch_secret(f"{az_prefix}--sftp--username") | ||
| self.password = self.keyvault.fetch_secret(f"{az_prefix}--sftp--password") |
There was a problem hiding this comment.
Probably makes more sense to pass in a private SSH key here for authentication, instead of password.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 88.35% 81.09% -7.27%
==========================================
Files 73 68 -5
Lines 3272 3210 -62
==========================================
- Hits 2891 2603 -288
- Misses 381 607 +226 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Adds a new SFTPUploader strategy to upload DICOM archives and Parquet exports to an SFTP server, backed by integration tests using a Docker‐based test server.
- Implements
SFTPUploaderwith methods to send ZIP streams and upload nested Parquet files. - Introduces
SFTPServerhelper for spinning up a local Docker SFTP container in tests. - Updates dependencies (
docker,paramiko) and pre‐commit config to include their type stubs.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pixl_core/src/core/uploader/_sftp.py | New SFTPUploader with _connect_client, send_via_sftp, and upload_parquet_files |
| pixl_core/tests/uploader/test_sftp.py | Tests for sending DICOM ZIPs and Parquet exports via SFTP |
| pixl_core/tests/uploader/helpers/sftpserver.py | SFTPServer Docker container helper for local SFTP testing |
| pixl_core/pyproject.toml | Added docker and paramiko dependencies |
| .pre-commit-config.yaml | Added types-docker and types-paramiko to linting hooks |
Comments suppressed due to low confidence (2)
pixl_core/src/core/uploader/_sftp.py:78
- The docstring mentions "FTPS" but this class implements SFTP. Update to "Upload Parquet via SFTP" for accuracy.
Upload parquet to FTPS under <project name>/<extract datetime>/parquet.
pixl_core/tests/uploader/test_sftp.py:143
- The path segment uses
"test"but the directory in the repo is likelytests. Update to/"tests"/resources/omopso the fixture can find the sample files.
parquet_export.copy_to_exports(Path(__file__).parents[3] / "test" / "resources" / "omop")
| self.username = self.keyvault.fetch_secret(f"{az_prefix}--sftp--username") | ||
| self.password = self.keyvault.fetch_secret(f"{az_prefix}--sftp--password") | ||
| self.port = int(self.keyvault.fetch_secret(f"{az_prefix}--sftp--port")) | ||
| self.known_hosts_path = self.keyvault.fetch_secret(f"{az_prefix}--sftp--known-hosts-path") |
There was a problem hiding this comment.
Might make more sense to pass in the host key itself, instead of a known_hosts file. On the other hand, the known_hosts file is what you get from the TRE.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Closing in favour of #608 |
|
Why not have both? |
Not against it, but the tests on CI were running into permission errors which I'm not getting locally. So I decided not to open that can of worms 😅 But happy to pick this back up if we ever really need it/I have some more time. |
2 participants
Description
Closes #606
Adds the
SFTPUploaderclass to handle uploads to an SFTP server, such as the ARC TRE.Type of change
Please delete options accordingly to the description.
Suggested Checklist
mainbranch.squash and merge