dstandish commented on a change in pull request #19665:
URL: https://github.com/apache/airflow/pull/19665#discussion_r760478968
##########
File path: airflow/providers/amazon/aws/hooks/redshift.py
##########
@@ -65,9 +81,9 @@ def cluster_status(self, cluster_identifier: str) -> str:
"""
try:
response =
self.get_conn().describe_clusters(ClusterIdentifier=cluster_identifier)['Clusters']
- return response[0]['ClusterStatus'] if response else None
+ return RedshiftClusterStates(response[0]['ClusterStatus']) if
response else None
Review comment:
> coerce anything we haven't seen yet to an UNKOWN Enum state, or just
continue to return None like other errors cases can do in this existing code
Yup it's true this is an approach that would be more tolerant of the
unexpected, it's just not what was done here.
But I would say there is little value in implementing such error handling in
an effort to still convert in this return value. If your value conforms to the
enum, then your comparisons against the enum would work even without converting
in the return. And if it doesn't conform, then you either fail (current code
state), or throw away information (replacing with None or UNKNOWN). So what's
the point of converting it to an enum at all? I would just leave the raw
values alone and use the enums for evaluation.
> Though I'm happy to disagree and commit to using strings if that's what
others think as well :)
I don't know that we need to make a universal proclaimation but in this case
I think not converting to enum makes sense :)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
