[GitHub] [airflow] dstandish commented on a change in pull request #19665: Add two operators in AWS Providers: RedshiftResumeClusterOperator and RedshiftPauseClusterOperator

[GitBox <gi...@apache.org>]

dstandish commented on a change in pull request #19665:
URL: https://github.com/apache/airflow/pull/19665#discussion_r759873574



##########
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:
       > IMO if there are breaking API changes and the API starts returning different states than you expect it's going to break your code whether you're using strings or an enum, so it's a moot point
   
   Not necessarily.  This hook method is just returning status.  If new statuses appear, it won't break your code, unless you force it into an enum that doesn't know the new status.  And it's far from certain that a new status would break your operator.  If you are waiting for your cluster to become `available` and there's a new status `kindof-almost-available` well that's still not available so your logic is unaffected -- unless of course you are forcing it into an enum and then your code breaks unnecessarily.  That's the scenario that makes me wary of this approach.




-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org