-
Notifications
You must be signed in to change notification settings - Fork 16.4k
DynamoDBHook - waiter_path() to consider resource_type or client_type
#30595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
client_typeresource_type or client_type
resource_type or client_typeresource_type or client_type
|
@o-nikolas @eladkal can you please confirm, if it's okay to use |
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. |
|
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. |
|
@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 |
ferruzzi
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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
|
Random flake (we need to fix it soon). MErging |
…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
Currently, when adding a custom
waiteron DynamoDBHook, it fails to register the customwaiterbecause of the following logic:airflow/airflow/providers/amazon/aws/hooks/base_aws.py
Lines 795 to 798 in 2ce1130
It requires the
self.client_typeto be set, ifself.client_typeis not set we should pickself.resource_typeinstead.closes: #30613