Skip to content

Add remaining paramiko connect params to SFTP filesystem #2823

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
Merged
sh-rp merged 2 commits into from
Aug 11, 2025
Merged

Add remaining paramiko connect params to SFTP filesystem #2823

sh-rp merged 2 commits into from
Aug 11, 2025

Conversation

@AyushPatel101
Copy link
Contributor

@AyushPatel101 AyushPatel101 commented Jun 30, 2025

Description

Docs - https://docs.paramiko.org/en/3.3/api/client.html#paramiko.client.SSHClient.connect

paramiko.client.SSHClient.connect has the option to provide a pkey, disabled_algorithms, transport_factory, auth_strategy params and I would like to expose those parameters for use.

Related Issues

@netlify
Copy link

netlify bot commented Jun 30, 2025 •
edited
Loading

✅ Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit 30252a0
🔍 Latest deploy log https://app.netlify.com/projects/dlt-hub-docs/deploys/68967f10f0bbb700082fe667

@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from a707278 to 57f327c Compare July 1, 2025 04:03
@AyushPatel101
Copy link
Contributor Author

AyushPatel101 commented Jul 7, 2025

Maybe @sh-rp if you have a chance to review

@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from 57f327c to 6ec0535 Compare July 8, 2025 01:22
@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from 6ec0535 to 3601209 Compare July 16, 2025 01:45
@AyushPatel101
Copy link
Contributor Author

AyushPatel101 commented Jul 16, 2025

Sorry to keep bothering, but would really appreciate a review on this as it is blocking my current workflow. Maybe @rudolfix ?

sh-rp
sh-rp requested changes Jul 17, 2025
View reviewed changes
Copy link
Collaborator

@sh-rp sh-rp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @AyushPatel101, sorry for the long wait, we were very busy! Thanks for your PR, I just have small change requests wrt typing, and please update the filesystem docs page with the new config parameters. And also rebase to the newest devel.

dlt/common/typing.py Outdated
DictStrAny: TypeAlias = Dict[str, Any]
DictStrStr: TypeAlias = Dict[str, str]
DictStrOptionalStr: TypeAlias = Dict[str, Optional[str]]
DictStrListStr: TypeAlias = Dict[str, List[str]]
Copy link
Collaborator

@sh-rp sh-rp Jul 17, 2025

Choose a reason for hiding this comment

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

We want to start getting rid of these kind of types, I think you can remove this and use Dict[str, List[str]] directly in the other places.

dlt/common/typing.py Outdated
else:
Transport = Any

SFTPTransportFactory: TypeAlias = Callable[[socket.socket], Transport]
Copy link
Collaborator

@sh-rp sh-rp Jul 17, 2025

Choose a reason for hiding this comment

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

There are types specific to the sftp_credentials, you should define them in the sftp_credentials class and not import anything from paramiko here.

@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from 96bb5d8 to 5faad74 Compare July 17, 2025 17:06
@AyushPatel101 AyushPatel101 requested a review from sh-rp July 17, 2025 18:07
@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from 5faad74 to d40b4a2 Compare July 31, 2025 18:28
@AyushPatel101
Copy link
Contributor Author

AyushPatel101 commented Aug 1, 2025

@sh-rp requesting re-review whenever you get a chance

@sh-rp sh-rp added ci full Use to trigger CI on a PR for full load tests ci from fork Use to trigger CI on a PR (even from a fork) and removed ci full Use to trigger CI on a PR for full load tests labels Aug 8, 2025
sh-rp
sh-rp requested changes Aug 8, 2025
View reviewed changes
Copy link
Collaborator

@sh-rp sh-rp left a comment

Choose a reason for hiding this comment

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

One small change left :)

Add pkey, disabled_algorithms, transport_factory and auth_strategy pa…
f20d5b7 
…rameters to paramiko.client.connect. Also update filesystem docs for SFTP creds
@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from 6ea65f5 to f20d5b7 Compare August 8, 2025 22:44
@AyushPatel101 AyushPatel101 force-pushed the feat/2822-add-sftp-auth-strategy branch from eb1bdc7 to 30252a0 Compare August 8, 2025 22:49
@sh-rp
Copy link
Collaborator

sh-rp commented Aug 11, 2025

Thanks :)

@sh-rp sh-rp merged commit f17e981 into dlt-hub:devel Aug 11, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sh-rp sh-rp sh-rp approved these changes

Assignees

No one assigned

Labels

ci from fork Use to trigger CI on a PR (even from a fork)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add auth_strategy support for SFTP client

2 participants

@AyushPatel101 @sh-rp