Skip to content

Conversation

@utkarsharma2
Copy link
Contributor

@utkarsharma2 utkarsharma2 commented Apr 12, 2023

Currently, when adding a custom waiter on DynamoDBHook, it fails to register the custom waiter because of the following logic:

@cached_property
def waiter_path(self) -> PathLike[str] | None:
path = Path(__file__).parents[1].joinpath(f"waiters/{self.client_type}.json").resolve()
return path if path.exists() else None

It requires the self.client_type to be set, if self.client_type is not set we should pick self.resource_type instead.

closes: #30613

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Apr 12, 2023
@utkarsharma2 utkarsharma2 marked this pull request as draft April 12, 2023 08:44
@utkarsharma2 utkarsharma2 changed the title DynamoDBHook missing client_type DynamoDBHook - waiter_path path to consider resource_type or client_type Apr 12, 2023
@utkarsharma2 utkarsharma2 changed the title DynamoDBHook - waiter_path path to consider resource_type or client_type DynamoDBHook - waiter_path() to consider resource_type or client_type Apr 12, 2023
@utkarsharma2 utkarsharma2 marked this pull request as ready for review April 12, 2023 14:53
@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Apr 12, 2023

@o-nikolas @eladkal can you please confirm, if it's okay to use resource_type if client_type is missing?

@utkarsharma2 utkarsharma2 marked this pull request as draft April 12, 2023 15:06
@o-nikolas
Copy link
Contributor

@o-nikolas @eladkal can you please confirm, if it's okay to use resource_type if client_type is missing?

If you've tested it and it's working then it's likely perfectly fine.

CCing @ferruzzi as well, he's done a lot of the work on our latest waiter code.

@ferruzzi
Copy link
Contributor

ferruzzi commented Apr 13, 2023

What Niko said :P I added that bit that you are modifying. I know of no reason why it wouldn't work. If you've tested it and it works, I'll back the change. So far DynamoDB is one of the only services I've run into that seems to prefer Resources over Clients. I'm sure there are others, I just haven't really used them. If it works as a Resource, it sounds like a good change to me. Glad to see others looking at making use of the custom waiters, I really like how they simplify things for us.

@utkarsharma2 utkarsharma2 marked this pull request as ready for review April 13, 2023 04:47
@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Apr 13, 2023

@o-nikolas @ferruzzi Yes, I tested it locally and added relevant info to the ticket. I also added a test case to validate. PTAL.

@ferruzzi Yup, good work on waiters. This seems like a neat way to standardize the polling process. :)

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

def test_waiter_path_generated_from_resource_type(self, _):
hook = DynamoDBHook(aws_conn_id="aws_default")
path = hook.waiter_path
assert path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you assert the path is what you expect rather than just not falsey? That will ensure your code to get the right filename is working as you expect and won't regress due to future code changes..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-nikolas Asserted the expected path

@potiuk
Copy link
Member

potiuk commented Apr 14, 2023

Random flake (we need to fix it soon). MErging

@potiuk potiuk merged commit 7c2d361 into apache:main Apr 14, 2023
wookiist pushed a commit to wookiist/airflow that referenced this pull request Apr 19, 2023
…ype` (apache#30595)

* Add  while initilizing

* Add  while initilizing

* Add logic to pick either client_type or resource_type

* Add test case

* Assert expected path
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.

DynamoDBHook - not able to registering a custom waiter

5 participants