Fix sftp sensor with pattern #29467
Merged
Fix sftp sensor with pattern #29467
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
BenediktRamsauer
requested review from
XD-DENG,
ashb,
bbovenzi,
dstandish,
eladkal,
ephraimbuddy,
jedcunningham,
kaxil,
mik-laj,
o-nikolas,
potiuk and
ryanahamilton
as code owners
Contributor
Author
|
Sorry guys, I made a mistake trying to upgrade my branch to the latest changes in I will also solve the unsuccessful tests |
The output of SFTPHook.get_file_by_pattern is a file name and not the full path to the file.
To my understanding, the goal of `test_file_with_pattern_parameter_call` is to test whether we can call the function with the `file_pattern` parameter. Due to the previous changes, the `os.path.join` requires to set a value for `sftp_hook_mock.return_value.get_file_by_pattern.return_value` so that the `poke` function would run. The case where we set this value is dealt with `test_file_present_with_pattern` and `test_file_not_present_with_pattern`. `test_file_with_pattern_parameter_call` thus does not test anything different or more specific than the two other tests.
o-nikolas
approved these changes
Feb 13, 2023
|
Awesome work, congrats on your first merged pull request! |
17 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will solve the issue registered by @RishuGuru in #28121.
It also takes into account the request from @potiuk in #28123 to add/fix the unit tests.
In summary, the SFTP Sensor was not finding the files when the
file_patternargument was used.This was due to an incorrect assumption that the SFTP Hook function
get_file_by_patternwould return the full path, where actually it only returns the file name (corrected in e08a702b508e01c8437e4a45eaa726bb39e4f47f).The unit test that was checking that also made this assumption (corrected in 018f316f96ed5f6361eea59987996454ced73bc7).
Also the unit test
test_file_with_pattern_parameter_callseemed to become redundant, and I removed it (full explanation in faa5a73d5b353a3c2c83a90b5040966158b97979).closes: #28121