-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Fix EC2Hook get_instance for client_type api #35960
Conversation
@@ -85,7 +85,7 @@ def get_instance(self, instance_id: str, filters: list | None = None): | |||
:return: Instance object | |||
""" | |||
if self._api_type == "client_type": | |||
return self.get_instances(filters=filters, instance_ids=[instance_id]) | |||
return self.get_instances(filters=filters, instance_ids=[instance_id])[0] |
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.
Is not it breaking change? Or we consider it as bug fix?
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.
For me, it's a bug fix. We can check with @eladkal.
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.
The method seems to return a single instance with the default api_type="resource_type". Similarly, this method should return a single instance for other api_types, so I would treat it as a bug fix. Also, it's named as get_instance so the expectation would be a single instance to be returned.
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.
I would classify this as bug fix
@@ -85,7 +85,7 @@ def get_instance(self, instance_id: str, filters: list | None = None): | |||
:return: Instance object | |||
""" | |||
if self._api_type == "client_type": | |||
return self.get_instances(filters=filters, instance_ids=[instance_id]) | |||
return self.get_instances(filters=filters, instance_ids=[instance_id])[0] |
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.
The method seems to return a single instance with the default api_type="resource_type". Similarly, this method should return a single instance for other api_types, so I would treat it as a bug fix. Also, it's named as get_instance so the expectation would be a single instance to be returned.
Thanks to all of you! |
The method
get_instance
should return a single instance and not a list.