Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
boring-cyborg[bot] commented on PR #39056: URL: https://github.com/apache/airflow/pull/39056#issuecomment-2096414610 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis merged PR #39056: URL: https://github.com/apache/airflow/pull/39056 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1590099031
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -97,11 +97,24 @@ def test_file_not_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@pytest.mark.parametrize(
+"newer_than", (
+datetime(2020, 1, 2),
+datetime(2020, 1, 2, tzinfo=timezone.utc),
Review Comment:
Test failed due to `pendulum.timezone` is a function, I guess you want to
use here `datetime.timezone.utc` here
So just import it and use here
```python
...
from datetime import timezone as stdlib_timezone
...
datetime(2020, 1, 2, tzinfo=stdlib_timezone.utc),
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1571992193
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -97,11 +97,12 @@ def test_file_not_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@pytest.mark.parametrize("newer_than", (datetime(2020, 1, 2), "2020-01-02
00:00:00+00:00"))
Review Comment:
Rename and add test case parameters.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1571991298 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: Use [airflow.utils.timezone.parse](https://github.com/apache/airflow/blob/0a74928894fb57b0160208262ccacad12da23fc7/airflow/utils/timezone.py#L194-L204) now. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1570691384
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -97,11 +97,12 @@ def test_file_not_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@pytest.mark.parametrize("newer_than", (datetime(2020, 1, 2), "2020-01-02
00:00:00+00:00"))
Review Comment:
- datetime(2020, 1, 2) parsed as a tz-naive datetime, and required to
convert it to the aware again, which is not as trivial as it might seem
- `"2020-01-02 00:00:00+00:00"` as tz-aware datetime and already has
timezone information
`datetime(2020, 1, 2) != datetime(2020, 1, 2, tzinfo=timezone.utc)`
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1570682361 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: My point here why we need yet another way to parse ISO format, or parse datetime/dates if we already have [`airflow.utils.timezone.parse`](https://github.com/apache/airflow/blob/0a74928894fb57b0160208262ccacad12da23fc7/airflow/utils/timezone.py#L194-L204) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1570674872 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: > newer_than accept originaly a datetime type. The meaning of the PR is to accept the same type in a jinja template. ds/ts are, you are right in ISO format. But if you try this : Don't get this example, could you elaborate a bit more? > Note that the same methode is used in the SFTPHook in the deferred mode. I use this method to be coherent with this. This not a point if some part already use it, it might be add years ago, or by a mistake. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1570674872 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: > newer_than accept originaly a datetime type. The meaning of the PR is to accept the same type in a jinja template. ds/ts are, you are right in ISO format. But if you try this : Don't get this example, could you elaborate a bit more? > Note that the same methode is used in the SFTPHook in the deferred mode. I use this method to be coherent with this. This not ever concern if some part already use it, it might be add years ago, or by a mistake. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1569031404 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: Sorry, not the SFTPHook, the SFTPTrigger. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1568677913
##
airflow/providers/sftp/sensors/sftp.py:
##
@@ -21,6 +21,7 @@
import os
from datetime import datetime, timedelta
+from dateutil.parser import parse as parse_date
Review Comment:
newer_than accept originaly a datetime type. The meaning of the PR is to
accept the same type in a jinja template.
ds/ts are, you are right in ISO format.
But if you try this :
```
from datetime import datetime
from dateutil.parser import parse as parse_date
print(datetime(2020, 1, 2))
print(parse_date("2020-01-02 00:00:00"))
print(parse_date("2018-01-01T00:00:00+00:00"))
print(parse_date("2020-01-02T00:00:00Z"))
```
It will render :
```
2020-01-02 00:00:00
2020-01-02 00:00:00
2018-01-01 00:00:00+00:00
2020-01-02 00:00:00+00:00
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056: URL: https://github.com/apache/airflow/pull/39056#discussion_r1568681779 ## airflow/providers/sftp/sensors/sftp.py: ## @@ -21,6 +21,7 @@ import os from datetime import datetime, timedelta +from dateutil.parser import parse as parse_date Review Comment: Note that the same methode is used in the SFTPHook in the deferred mode. I use this method to be coherent with this. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1568669329
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -97,11 +97,12 @@ def test_file_not_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@pytest.mark.parametrize("newer_than", (datetime(2020, 1, 2), "2020-01-02
00:00:00+00:00"))
Review Comment:
Doesn't look but ... It's actually the str representation of "datetime(2020,
1, 2)" like it could be passed as a jina template in newer_than.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
Taragolis commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1568523093
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -97,11 +97,12 @@ def test_file_not_new_enough(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@pytest.mark.parametrize("newer_than", (datetime(2020, 1, 2), "2020-01-02
00:00:00+00:00"))
Review Comment:
Small nit, "2020-01-02 00:00:00+00:00" - doesn't look as naive datetime
##
airflow/providers/sftp/sensors/sftp.py:
##
@@ -21,6 +21,7 @@
import os
from datetime import datetime, timedelta
+from dateutil.parser import parse as parse_date
Review Comment:
Is it actually required to parse ds/ts which are ISO format representation?
Because if a target to parse ISO format (other formats might be not parsed
correctly) you could just use a `datetime.datetime.fromisoformat(...)`
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
grrolland commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1567514975
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -108,6 +108,17 @@ def test_naive_datetime(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@patch("airflow.providers.sftp.sensors.sftp.SFTPHook")
Review Comment:
Your are right, my bad. It's done now.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Fix SFTPSensor.newer_than not working with jinja logical ds/ts expression [airflow]
dirrao commented on code in PR #39056:
URL: https://github.com/apache/airflow/pull/39056#discussion_r1567407163
##
tests/providers/sftp/sensors/test_sftp.py:
##
@@ -108,6 +108,17 @@ def test_naive_datetime(self, sftp_hook_mock):
sftp_hook_mock.return_value.get_mod_time.assert_called_once_with("/path/to/file/1970-01-01.txt")
assert not output
+@patch("airflow.providers.sftp.sensors.sftp.SFTPHook")
Review Comment:
I would suggest to parameterize the newer_than rather than reapting the same
code.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
