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

SFTPOperator - add support for list of file paths #26666

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

pauldalewilliams
Copy link
Contributor

This PR adds support for passing a list of file paths to local_filepath or remote_filepath for the SFTPOperator. We've used this on my team for several years now to enable uploading or downloading several files via SFTP with a single connection in one task instead of splitting it out into multiple tasks.

It checks for and requires that the total number of paths provided is equal between the two parameters.
Maintains backward compatibility by converting a single string passed to the parameter into a list with that string as its only element.

@potiuk
Copy link
Member

potiuk commented Sep 26, 2022

some tests are failing (Seems related)

@pauldalewilliams pauldalewilliams force-pushed the sftp-add-list-support branch 2 times, most recently from c8ef195 to f3fb21d Compare September 26, 2022 13:17
@pauldalewilliams
Copy link
Contributor Author

some tests are failing (Seems related)

@potiuk - This is related to our Slack conversation about the intermittently failing tests. If you look at the current checks you'll see it passed this time (after rebase). So it's not related to these code changes. I'm hoping to work on improving those 4 tests today if I can find the time.

@potiuk
Copy link
Member

potiuk commented Sep 26, 2022

some tests are failing (Seems related)

@potiuk - This is related to our Slack conversation about the intermittently failing tests. If you look at the current checks you'll see it passed this time (after rebase). So it's not related to these code changes. I'm hoping to work on improving those 4 tests today if I can find the time.

Yeah. I recall. It was just strange they all failed there - they seemed related and I do not recall those tests to be THAT flaky :)

@pauldalewilliams
Copy link
Contributor Author

some tests are failing (Seems related)

@potiuk - This is related to our Slack conversation about the intermittently failing tests. If you look at the current checks you'll see it passed this time (after rebase). So it's not related to these code changes. I'm hoping to work on improving those 4 tests today if I can find the time.

Yeah. I recall. It was just strange they all failed there - they seemed related and I do not recall those tests to be THAT flaky :)

What's funny is that they didn't fail in that run on the other variations (MySQL, MSSQL, Sqlite). That's what I'm talking about - sometimes you get lucky and they all pass, other times they don't. I'm going to rebase this again and we'll see if it passes, but I think my code in this PR is good to go...these tests just need to be tweaked.

@potiuk
Copy link
Member

potiuk commented Sep 26, 2022

MySQL/MSSQL will not fail as they do not run provider's tests (those DBs take too much memory for Public Runners to run them quickly). But yeah - i was under the impression in our Slack conversation that they failed when they were run in isolation, but if they are flaky while running in CI, then indeed it needs to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants