ramitkataria commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4044788981
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
I am not sure how people use the string form of `*_filenames` in practice,
but this could affect a few groups without any error being raised: anyone
filtering on a suffix like `".csv"`, S3 users with nested keys under `s3_key`,
and FTP servers whose `nlst` returns path-qualified entries, since
`FTPToS3Operator` currently passes the listed name straight to `retrieve_file`.
In all of those cases the task would now succeed having transferred zero files.
What do you think about:
- Match and replace on the basename in the FTP operator so `startswith`
works regardless of `nlst` output shape
- If you think the suffix use case is common, we could discuss whether
selection should stay lenient while only the rename is fixed, since the rename
corruption is the clearer bug
Depending on the approach, I think we should add a warning in the changelog
so users are aware.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
