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/01/05 07:49:53 UTC
[GitHub] [airflow] uranusjr opened a new pull request #20669: Fix ECSProtocol compat shim inheritance
uranusjr opened a new pull request #20669:
URL: https://github.com/apache/airflow/pull/20669
Tests are failing:
```
TypeError: Protocols can only inherit from other protocols, got <class 'airflow.providers.amazon.aws.operators.ecs.ECSProtocol'>
```
This is due to the compatibility shim for `ECSProtocol` is incorrectly double-inheriting `Protocol`. Since the “real” `ECSProtocol` class (i.e. `NewECSProtocol`) is already a protocol, the compat `ECSProtocol` does not need to (and can’t) also inherit from `Protocol`. Similarly, the `@runtime_checkable` decorator shouldn’t be applied, since the parent `ECSProtocol` is already runtime-checkable.
--
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] eladkal commented on a change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778842284
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
Is `pytest -s tests/core/test_core_to_contrib.py` passing? (I'm not sure if the test is executed as it's a core test)
I wonder if it causes troubles as we have a deprecated class pointing to a deprecated class
https://github.com/apache/airflow/blob/61b29d7454f024efce63a2cb684eb876ec7e4856/tests/deprecated_classes.py#L1067-L1068
--
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] eladkal commented on a change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778940673
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
If passes then we are OK!
--
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] eladkal commented on a change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778664620
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
mm this classes are also deprecated.
it was renamed to `EcsOperator` and `EcsProtocol`
--
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] github-actions[bot] commented on pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#issuecomment-1005515267
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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] kaxil commented on pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#issuecomment-1005514664
oh .. there is https://github.com/apache/airflow/pull/20670 from @eladkal too that address the same issue in a different way
--
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 a change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778932532
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
Shall we merge to make the PRs green again ?
--
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 change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778922143
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
It passes. We currently only check the old import is deprecated, whether the new import is deprecated or _not_ is irrelevant.
--
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 merged pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20669:
URL: https://github.com/apache/airflow/pull/20669
--
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 change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778922143
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
It passes. We currently only check the old import is deprecated, whether the _new_ import is deprecated or not is irrelevant.
--
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] ferruzzi commented on pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#issuecomment-1006056319
Cool, I think this is the issue I was running into here as well: https://github.com/apache/airflow/pull/20374
--
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 change in pull request #20669: Fix ECSProtocol compat shim inheritance
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20669:
URL: https://github.com/apache/airflow/pull/20669#discussion_r778768017
##########
File path: airflow/contrib/operators/ecs_operator.py
##########
@@ -19,26 +19,12 @@
import warnings
-from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol as NewECSProtocol # noqa
-from airflow.typing_compat import Protocol, runtime_checkable
+from airflow.providers.amazon.aws.operators.ecs import ECSOperator, ECSProtocol
Review comment:
Yeah, and I’m intentionally importing those deprecated ones so if someone imports this module (and misses the import-time deprecation warning), instantiating either class would also spit out a deprecation warning.
--
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