[GitHub] [airflow] dstandish commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators

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

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



##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
         :rtype: Dict
         """
         eks_client = self.conn
+
         # The below tag is mandatory and must have a value of either 'owned' or 'shared'
         # A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
         # The 'shared' value allows more than one resource to use the subnet.
-        tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
-        if "tags" in kwargs:
-            tags = {**tags, **kwargs["tags"]}
-            kwargs.pop("tags")
+        tags = kwargs.pop("tags", {})
+        tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'

Review comment:
       this is breaking because now user cannot set it to 'shared'

##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
          If this is None or empty then the default boto3 behaviour is used.
     :type region: str
 
-    If compute is assigned the value of 'nodegroup', the following are required:
+    If compute is assigned the value of 'nodegroup':

Review comment:
       why this change?  seems better before, more explicit about the meaning.
   
   if x, then _what_? rather than just "if x:"
   
   perhaps it was not correct before?




-- 
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