You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/11/30 21:07:37 UTC
[PR] Fix a bug with accessing hooks in EKS trigger [airflow]
hussein-awala opened a new pull request, #35989:
URL: https://github.com/apache/airflow/pull/35989
The hook is defined as a standard method; converting it to a cached property is a breaking change. The best solution is to call this method to get the hook,
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #35989:
URL: https://github.com/apache/airflow/pull/35989#issuecomment-1834622225
Currently, hook is a standard method, so if someone extends the trigger, he should use:
```python
self.hook()
```
If we convert it to a cached_property, `self.hook` will return the hook, and the `()` will be applied on its value. A simple example:
```python
>>> from functools import cached_property
>>> class X:
... @cached_property
... def x(self):
... return 1
...
>>> X().x()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'int' object is not callable
```
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #35989:
URL: https://github.com/apache/airflow/pull/35989#issuecomment-1834595034
Another option could be to create the hook as a cached property and use this property in the code. To keep it backward compatible, you can just return the property in the method
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #35989:
URL: https://github.com/apache/airflow/pull/35989#issuecomment-1834632581
Oh yeah, I did not realize the 2 methods would have the same name. You can name the cached property method `eks_hook` but I guess it goes against the convention
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala merged PR #35989:
URL: https://github.com/apache/airflow/pull/35989
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #35989:
URL: https://github.com/apache/airflow/pull/35989#issuecomment-1834615327
> > Another option could be to create the hook as a cached property and use this property in the code. To keep it backward compatible, you can just return the property in the method
>
> The compatibility issue is not for us, but for the users who extend the trigger and create a subclass of it. Triggers are part of the Airflow public interface.
I understand but why that would not be backward compatible for these users?
--
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] Fix a bug with accessing hooks in EKS trigger [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #35989:
URL: https://github.com/apache/airflow/pull/35989#issuecomment-1834600112
> Another option could be to create the hook as a cached property and use this property in the code. To keep it backward compatible, you can just return the property in the method
The compatibility issue is not for us, but for the users who extend the trigger and create a subclass of it. Triggers are part of the Airflow public interface.
--
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