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

bug: Add options to SFTP #2843

Merged
merged 7 commits into from
Apr 4, 2024
Merged

bug: Add options to SFTP #2843

merged 7 commits into from
Apr 4, 2024

Conversation

potter-potter
Copy link
Contributor

@potter-potter potter-potter commented Apr 3, 2024

Noticed authentication errors when connected to a non localhost SFTP. It errored out when looking for ssh keys.

This gives us the option to not look for those. Which is correct if we are giving it user/password.

@potter-potter potter-potter changed the title Potter/improve sftp bug: Add options to SFTP Apr 3, 2024
@potter-potter potter-potter marked this pull request as ready for review April 3, 2024 19:44
CHANGELOG.md Outdated
@@ -7,6 +7,7 @@
### Fixes

* **Fix `partition_html()` swallowing some paragraphs**. The `partition_html()` only considers elements with limited depth to avoid becoming the text representation of a giant div. This fix increases the limit value.
* **Fix SFTP** Adds more options to prevent looking for ssh files if only using username password.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* **Fix SFTP** Adds more options to prevent looking for ssh files if only using username password.
* **Fix SFTP** Adds flag options to SFTP connector on whether to use ssh keys / agent, with flag values defaulting to False. This is to prevent looking for ssh files when using username and password. Currently, username and password are required, making that always the case.

@ahmetmeleq
Copy link
Contributor

  • Ingest test on sftp source runs on localhost, which should be why we weren't able to catch this
  • Spinning / keeping a remote server alive just to catch similar issues in CI tests is possible, but not urgent
  • Defaults for --allow-agent and --look-for-keys being false makes sense, given that --username and --password are required

LGTM

@potter-potter potter-potter added this pull request to the merge queue Apr 4, 2024
Merged via the queue into main with commit ae31586 Apr 4, 2024
46 checks passed
@potter-potter potter-potter deleted the potter/improve-sftp branch April 4, 2024 15:37
kaaloo pushed a commit to inclusif/unstructured that referenced this pull request Apr 8, 2024
Noticed authentication errors when connected to a non localhost SFTP. It
errored out when looking for ssh keys.

This gives us the option to not look for those. Which is correct if we
are giving it user/password.
kaaloo pushed a commit to inclusif/unstructured that referenced this pull request Apr 8, 2024
Noticed authentication errors when connected to a non localhost SFTP. It
errored out when looking for ssh keys.

This gives us the option to not look for those. Which is correct if we
are giving it user/password.
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.

2 participants