You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/09/26 02:10:33 UTC
[GitHub] [airflow] pauldalewilliams opened a new pull request, #26666: SFTPOperator - add support for list of file paths
pauldalewilliams opened a new pull request, #26666:
URL: https://github.com/apache/airflow/pull/26666
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #26666:
URL: https://github.com/apache/airflow/pull/26666#discussion_r979510418
##########
airflow/providers/sftp/operators/sftp.py:
##########
@@ -129,7 +143,7 @@ def __init__(
)
self.sftp_hook = SFTPHook(ssh_hook=self.ssh_hook)
- def execute(self, context: Any) -> str | None:
+ def execute(self, context: Any) -> list[str] | None:
Review Comment:
I wonder if it’d be a good idea to still return `str` if a str is passed (instead of `list[str]`). This would avoid breaking compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26666:
URL: https://github.com/apache/airflow/pull/26666#issuecomment-1258506378
> > 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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pauldalewilliams commented on pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
pauldalewilliams commented on PR #26666:
URL: https://github.com/apache/airflow/pull/26666#issuecomment-1258225270
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pauldalewilliams commented on pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
pauldalewilliams commented on PR #26666:
URL: https://github.com/apache/airflow/pull/26666#issuecomment-1258509217
> > > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr merged pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
uranusjr merged PR #26666:
URL: https://github.com/apache/airflow/pull/26666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pauldalewilliams commented on a diff in pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
pauldalewilliams commented on code in PR #26666:
URL: https://github.com/apache/airflow/pull/26666#discussion_r979517661
##########
airflow/providers/sftp/operators/sftp.py:
##########
@@ -129,7 +143,7 @@ def __init__(
)
self.sftp_hook = SFTPHook(ssh_hook=self.ssh_hook)
- def execute(self, context: Any) -> str | None:
+ def execute(self, context: Any) -> list[str] | None:
Review Comment:
I'm leaning toward setting a flag since that would be the only way to keep it consistent. I'll push shortly - adding a test to cover this scenario.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26666:
URL: https://github.com/apache/airflow/pull/26666#issuecomment-1258513525
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26666:
URL: https://github.com/apache/airflow/pull/26666#issuecomment-1257718331
some tests are failing (Seems related)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pauldalewilliams commented on a diff in pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
pauldalewilliams commented on code in PR #26666:
URL: https://github.com/apache/airflow/pull/26666#discussion_r979516391
##########
airflow/providers/sftp/operators/sftp.py:
##########
@@ -129,7 +143,7 @@ def __init__(
)
self.sftp_hook = SFTPHook(ssh_hook=self.ssh_hook)
- def execute(self, context: Any) -> str | None:
+ def execute(self, context: Any) -> list[str] | None:
Review Comment:
Ah, good point - I had considered that at one point and forgot to incorporate it. Hmm, what do you think - set a flag if I converted a `str` to a `list[str]` or just make `execute` return a `str` if `len(list)` == 1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pauldalewilliams commented on a diff in pull request #26666: SFTPOperator - add support for list of file paths
Posted by GitBox <gi...@apache.org>.
pauldalewilliams commented on code in PR #26666:
URL: https://github.com/apache/airflow/pull/26666#discussion_r979519902
##########
airflow/providers/sftp/operators/sftp.py:
##########
@@ -129,7 +143,7 @@ def __init__(
)
self.sftp_hook = SFTPHook(ssh_hook=self.ssh_hook)
- def execute(self, context: Any) -> str | None:
+ def execute(self, context: Any) -> list[str] | None:
Review Comment:
OK @uranusjr - see pauldalewilliams/airflow@9157bbc
Look better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org