[GitHub] [airflow] dsaiztc commented on a change in pull request #6523: [AIRFLOW-5873] KubernetesPodOperator fixes and test

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

dsaiztc commented on a change in pull request #6523:
URL: https://github.com/apache/airflow/pull/6523#discussion_r422286551



##########
File path: airflow/contrib/operators/kubernetes_pod_operator.py
##########
@@ -158,17 +167,17 @@ def execute(self, context):
                 raise AirflowException(
                     'Pod returned a failure: {state}'.format(state=final_state)
                 )
-            if self.xcom_push:
+            if self.do_xcom_push:
                 return result
         except AirflowException as ex:
             raise AirflowException('Pod Launching failed: {error}'.format(error=ex))
 
     def _set_resources(self, resources):
-        inputResource = Resources()
-        if resources:
-            for item in resources.keys():
-                setattr(inputResource, item, resources[item])
-        return inputResource
+        return Resources(**resources) if resources else Resources()
+
+    def _set_name(self, name):
+        validate_key(name, max_length=63)

Review comment:
       Hi,
   
   I've just upgraded from v.1.10.6 and thus [this change has been included](https://airflow.apache.org/docs/stable/changelog.html#airflow-1-10-7-2019-12-24), breaking a lot of the DAGs we currently have setup due to this change.
   **I hugely appreciate the effort in this development**, although fair warning of this _breaking change_ would have been very appreciated.
   
   Just leaving this here in case someone using `KubernetesPodOperator` finds the same _issue_ when upgrading.
   ```python
   ...
     File "/usr/local/lib/python3.6/site-packages/airflow/contrib/operators/kubernetes_pod_operator.py", line 167, in __init__ 
       self.name = self._set_name(name) 
     File "/usr/local/lib/python3.6/site-packages/airflow/contrib/operators/kubernetes_pod_operator.py", line 269, in _set_name 
       validate_key(name, max_length=63) 
     File "/usr/local/lib/python3.6/site-packages/airflow/utils/helpers.py", line 64, in validate_key 
       "The key has to be less than {0} characters".format(max_length)) 
   airflow.exceptions.AirflowException: The key has to be less than 63 characters
   ```
   
   I understand [Kubernetes has a character limit of 63 for its label's names](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) but, at least in v1.10.6 (the latest version previous to this change) [Airflow adds another 8 characters to the name](https://github.com/apache/airflow/blob/1.10.6/airflow/contrib/kubernetes/pod_generator.py#L167), so I'm guessing the limit to set here would be `55`, right?
   
   Please let me know if I'm missing something.
   
   Again, thanks a lot for the work on this, I mean this as constructive feedback.
   




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

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