You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Taragolis (via GitHub)" <gi...@apache.org> on 2023/12/18 16:23:35 UTC
[PR] Add support of Pendulum 3 [airflow]
Taragolis opened a new pull request, #36281:
URL: https://github.com/apache/airflow/pull/36281
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Related: https://github.com/apache/airflow/pull/35798, the differences is bump to min pendulum 3
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1884967587
Just wondering shall we merge it or better to wait for someone else review?
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1436018174
##########
airflow/providers/cncf/kubernetes/pod_launcher_deprecated.py:
##########
@@ -148,13 +148,13 @@ def monitor_pod(self, pod: V1Pod, get_logs: bool) -> tuple[State, str | None]:
"""
if get_logs:
read_logs_since_sec = None
- last_log_time = None
+ last_log_time: pendulum.DateTime | None = None
while True:
logs = self.read_pod_logs(pod, timestamps=True, since_seconds=read_logs_since_sec)
for line in logs:
timestamp, message = self.parse_log_line(line.decode("utf-8"))
if timestamp:
- last_log_time = pendulum.parse(timestamp)
+ last_log_time = cast(pendulum.DateTime, pendulum.parse(timestamp))
Review Comment:
For formal pass type checking `¯\_(ツ)_/¯`.
`penulum.parse` might return any of `pendulum.datetime.DateTime`, `pendulum.date.Date`, `pendulum.time.Time`, `pendulum.time.Period` and in addition in pendulum 3 it could return `pendulum.interval.Interval` but it not listed in type annotation:
- https://github.com/sdispater/pendulum/blob/3.0.0/src/pendulum/parser.py
- https://github.com/sdispater/pendulum/blob/2.1.2/pendulum/parser.py
Yeah, there is might be potential bug exists because `last_log_time` could be parsed in any type rather than `DateTime`. However I keep this code without any changes, rather than forced type checkers think that this actually `DateTime`.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1430966672
##########
airflow/example_dags/plugins/workday.py:
##########
@@ -77,9 +77,10 @@ def next_dagrun_info(
tzinfo=UTC
)
else: # This is the first ever run on the regular schedule.
- next_start = restriction.earliest
- if next_start is None: # No start_date. Don't schedule.
+ if restriction.earliest is None: # No start_date. Don't schedule.
return None
+ next_start = restriction.earliest
+
Review Comment:
See #36296
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1430555265
##########
tests/cli/commands/test_dag_command.py:
##########
@@ -161,7 +161,7 @@ def test_backfill(self, mock_run):
)
output = stdout.getvalue()
- assert f"Dry run of DAG example_bash_operator on {DEFAULT_DATE.isoformat()}\n" in output
+ assert f"Dry run of DAG example_bash_operator on {DEFAULT_DATE.isoformat(sep=' ')}\n" in output
Review Comment:
Because `penudlum.DateTime` now have less overwrite on `datetime.datetime` it change the behaviour of convert to string
**Pendulum 2**
```python
>>> import pendulum
>>> str(pendulum.now())
'2023-12-18T23:14:57.788852+04:00'
```
**Pendulum 3**
```python
>>> import pendulum
>>> str(pendulum.now())
'2023-12-18 23:14:57.788852+04:00'
```
##########
tests/api_connexion/endpoints/test_dag_endpoint.py:
##########
@@ -418,7 +418,7 @@ def test_should_response_200_for_null_start_date(self, url_safe_serializer):
"tags": [],
"template_searchpath": None,
"timetable_description": None,
- "timezone": "Timezone('UTC')",
+ "timezone": "UTC",
Review Comment:
Well, change in default type for UTC change API response and instead of __repr__ it starts return Timezone
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1868351118
Seems like it close to backcompat with 2, just need to resolve some static checks
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1436021229
##########
airflow/providers/cncf/kubernetes/pod_launcher_deprecated.py:
##########
@@ -148,13 +148,13 @@ def monitor_pod(self, pod: V1Pod, get_logs: bool) -> tuple[State, str | None]:
"""
if get_logs:
read_logs_since_sec = None
- last_log_time = None
+ last_log_time: pendulum.DateTime | None = None
while True:
logs = self.read_pod_logs(pod, timestamps=True, since_seconds=read_logs_since_sec)
for line in logs:
timestamp, message = self.parse_log_line(line.decode("utf-8"))
if timestamp:
- last_log_time = pendulum.parse(timestamp)
+ last_log_time = cast(pendulum.DateTime, pendulum.parse(timestamp))
Review Comment:
In addition `airflow.providers.cncf.kubernetes.utils.pod_launcher_deprecated` is deprecated and replaced by `airflow.providers.cncf.kubernetes.utils.pod_manager`
However the implementation in this case almost the same with the same potential problem, I'm not sure is it possible to get something rather than DateTime but might be better check instance of class and replace it by None if it invalid type
https://github.com/apache/airflow/blob/3e7f50ece336f4fe09f13d95fd1115394be6f314/airflow/providers/cncf/kubernetes/utils/pod_manager.py#L607-L611
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1449482646
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -239,6 +239,19 @@ function check_download_sqlalchemy() {
pip check
}
+# Download minimum supported version of pendulum to run tests with it
+function check_download_pendulum() {
+ if [[ ${DOWNGRADE_PENDULUM=} != "true" ]]; then
+ return
+ fi
+ min_pendulum_version=$(grep "\"pendulum>=" pyproject.toml | sed "s/.*>=\([0-9\.]*\).*/\1/" | xargs)
Review Comment:
❤️ it
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1866122251
I like it :-)
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1435885636
##########
airflow/providers/cncf/kubernetes/pod_launcher_deprecated.py:
##########
@@ -148,13 +148,13 @@ def monitor_pod(self, pod: V1Pod, get_logs: bool) -> tuple[State, str | None]:
"""
if get_logs:
read_logs_since_sec = None
- last_log_time = None
+ last_log_time: pendulum.DateTime | None = None
while True:
logs = self.read_pod_logs(pod, timestamps=True, since_seconds=read_logs_since_sec)
for line in logs:
timestamp, message = self.parse_log_line(line.decode("utf-8"))
if timestamp:
- last_log_time = pendulum.parse(timestamp)
+ last_log_time = cast(pendulum.DateTime, pendulum.parse(timestamp))
Review Comment:
Why does this need a cast? I wonder if the new annotation actually exposes a potential bug.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1872503744
Looks green. Does it mean we have it ??
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1860996123
Full qualified path is the same. The main difference how UTC timezone works:
- In pendulum 2 it was pendulum.tz.timezone.FixedTimezone, so it deserialise into this type if it changed in past version
- In pendulum 3 it was pendulum.tz.timezone.Timezone
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1435801808
##########
.github/workflows/ci.yml:
##########
@@ -1123,11 +1123,61 @@ jobs:
breeze testing db-tests
--parallel-test-types "${{needs.build-info.outputs.parallel-test-types-list-as-string}}"
- name: >
- Post Tests success: ${{needs.build-info.outputs.default-python-version}}:Boto"
+ Post Tests success: ${{needs.build-info.outputs.default-python-version}}:MinSQLAlchemy$"
uses: ./.github/actions/post_tests_success
if: success()
- name: >
- Post Tests failure: ${{needs.build-info.outputs.default-python-version}}:Boto"
+ Post Tests failure: ${{needs.build-info.outputs.default-python-version}}:MinSQLAlchemy$"
+ uses: ./.github/actions/post_tests_failure
+ if: failure()
+
+ tests-postgres-pendulum-2:
Review Comment:
Nice!
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1879616465
I think we should bite the bullet and start fixing stuff that might be affected. So yes we should have it.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1436019316
##########
airflow/providers/cncf/kubernetes/pod_launcher_deprecated.py:
##########
@@ -148,13 +148,13 @@ def monitor_pod(self, pod: V1Pod, get_logs: bool) -> tuple[State, str | None]:
"""
if get_logs:
read_logs_since_sec = None
- last_log_time = None
+ last_log_time: pendulum.DateTime | None = None
while True:
logs = self.read_pod_logs(pod, timestamps=True, since_seconds=read_logs_since_sec)
for line in logs:
timestamp, message = self.parse_log_line(line.decode("utf-8"))
if timestamp:
- last_log_time = pendulum.parse(timestamp)
+ last_log_time = cast(pendulum.DateTime, pendulum.parse(timestamp))
Review Comment:
Ah OK that makes sense; I hate this kind of multi purpose functions. I’d prefer something like `assert isinstance(last_log_time, pendulum.DateTime)` (less runtime overhead) but this is fine.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1885948891
Fortunately All failures after latest rebase not relevant to changes from this PR and should be fixed by https://github.com/apache/airflow/pull/36728
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1449485688
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -239,6 +239,19 @@ function check_download_sqlalchemy() {
pip check
}
+# Download minimum supported version of pendulum to run tests with it
+function check_download_pendulum() {
+ if [[ ${DOWNGRADE_PENDULUM=} != "true" ]]; then
+ return
+ fi
+ min_pendulum_version=$(grep "\"pendulum>=" pyproject.toml | sed "s/.*>=\([0-9\.]*\).*/\1/" | xargs)
Review Comment:
Feel free to resolve and merge @Taragolis :D
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1860979390
What happens if serialization (serde) encounters a OLD version of a pendulum timezone? I havent verified it myself but I assume the class names have changed?
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1430937442
##########
airflow/example_dags/plugins/workday.py:
##########
@@ -77,9 +77,10 @@ def next_dagrun_info(
tzinfo=UTC
)
else: # This is the first ever run on the regular schedule.
- next_start = restriction.earliest
- if next_start is None: # No start_date. Don't schedule.
+ if restriction.earliest is None: # No start_date. Don't schedule.
return None
+ next_start = restriction.earliest
+
Review Comment:
This does not look relevant?
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1430963141
##########
airflow/example_dags/plugins/workday.py:
##########
@@ -77,9 +77,10 @@ def next_dagrun_info(
tzinfo=UTC
)
else: # This is the first ever run on the regular schedule.
- next_start = restriction.earliest
- if next_start is None: # No start_date. Don't schedule.
+ if restriction.earliest is None: # No start_date. Don't schedule.
return None
+ next_start = restriction.earliest
+
Review Comment:
Most likely due to Pendulum 3 has better annotation, the current code is indeed not type safe as Mypy points out, but the problem is masked by inferior annotations in Pendulum.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1435885656
##########
airflow/serialization/serialized_objects.py:
##########
@@ -167,9 +168,9 @@ def encode_timezone(var: Timezone) -> str | int:
)
-def decode_timezone(var: str | int) -> Timezone:
+def decode_timezone(var: str | int):
Review Comment:
```suggestion
def decode_timezone(var: str | int) -> Timezone | FixedTimezone:
```
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1449484328
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -239,6 +239,19 @@ function check_download_sqlalchemy() {
pip check
}
+# Download minimum supported version of pendulum to run tests with it
+function check_download_pendulum() {
+ if [[ ${DOWNGRADE_PENDULUM=} != "true" ]]; then
+ return
+ fi
+ min_pendulum_version=$(grep "\"pendulum>=" pyproject.toml | sed "s/.*>=\([0-9\.]*\).*/\1/" | xargs)
Review Comment:
BTW. This is EXACTLY what I wanted to do for ALL dependencies of ours in https://github.com/apache/airflow/issues/35549 (running tests per provider).
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis merged PR #36281:
URL: https://github.com/apache/airflow/pull/36281
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1888834137
:tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada:
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1882966528
I will rebase and add release notes today if I feel 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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1450406074
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -239,6 +239,19 @@ function check_download_sqlalchemy() {
pip check
}
+# Download minimum supported version of pendulum to run tests with it
+function check_download_pendulum() {
+ if [[ ${DOWNGRADE_PENDULUM=} != "true" ]]; then
+ return
+ fi
+ min_pendulum_version=$(grep "\"pendulum>=" pyproject.toml | sed "s/.*>=\([0-9\.]*\).*/\1/" | xargs)
Review Comment:
At least this step help to found that Airflow can't work with pendulum 2.0.0 (previous min version) and had some issues.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1449484328
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -239,6 +239,19 @@ function check_download_sqlalchemy() {
pip check
}
+# Download minimum supported version of pendulum to run tests with it
+function check_download_pendulum() {
+ if [[ ${DOWNGRADE_PENDULUM=} != "true" ]]; then
+ return
+ fi
+ min_pendulum_version=$(grep "\"pendulum>=" pyproject.toml | sed "s/.*>=\([0-9\.]*\).*/\1/" | xargs)
Review Comment:
BTW. This is EXACTLY what I wanted to do for ALL dependencies of ours in https://github.com/apache/airflow/issues/35549
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #36281:
URL: https://github.com/apache/airflow/pull/36281#discussion_r1430945154
##########
airflow/example_dags/plugins/workday.py:
##########
@@ -77,9 +77,10 @@ def next_dagrun_info(
tzinfo=UTC
)
else: # This is the first ever run on the regular schedule.
- next_start = restriction.earliest
- if next_start is None: # No start_date. Don't schedule.
+ if restriction.earliest is None: # No start_date. Don't schedule.
return None
+ next_start = restriction.earliest
+
Review Comment:
Yeah, that is looks weird but `mypy` checks starts fail [here](https://github.com/apache/airflow/actions/runs/7253257238/job/19760229574)
```console
airflow/example_dags/plugins/workday.py:80: error: Incompatible types in
assignment (expression has type "Optional[DateTime]", variable has type
"DateTime") [assignment]
next_start = restriction.earliest
```
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1861025841
Anyway It is a good point to add this kind of tests.
--
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
Re: [PR] Add support of Pendulum 3 [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #36281:
URL: https://github.com/apache/airflow/pull/36281#issuecomment-1868499023
Seems like all major stuff are resolved, so this PR could be reviewed.
The only things are required is add is newsfragment file
--
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