Skip to content

Fix a bug with accessing hooks in EKS trigger#35989

Merged
hussein-awala merged 1 commit into
apache:mainfrom
hussein-awala:fix_triggers_hooks
Nov 30, 2023
Merged

Fix a bug with accessing hooks in EKS trigger#35989
hussein-awala merged 1 commit into
apache:mainfrom
hussein-awala:fix_triggers_hooks

Conversation

@hussein-awala
Copy link
Copy Markdown
Member

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,

@vincbeck
Copy link
Copy Markdown
Contributor

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

@hussein-awala
Copy link
Copy Markdown
Member Author

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.

@vincbeck
Copy link
Copy Markdown
Contributor

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?

@hussein-awala
Copy link
Copy Markdown
Member Author

Currently, hook is a standard method, so if someone extends the trigger, he should use:

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:

>>> 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

@vincbeck
Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants