[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence

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

dstandish commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r991467530


##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -56,15 +56,15 @@ class KubernetesHook(BaseHook):
         :doc:`/connections/kubernetes`
 
     :param conn_id: The :ref:`kubernetes connection <howto/connection:kubernetes>`
-        to Kubernetes cluster.
+        to Kubernetes cluster
     :param client_configuration: Optional dictionary of client configuration params.
-        Passed on to kubernetes client.
+        Passed on to kubernetes client
     :param cluster_context: Optionally specify a context to use (e.g. if you have multiple
-        in your kubeconfig.
-    :param config_file: Path to kubeconfig file.
-    :param in_cluster: Set to ``True`` if running from within a kubernetes cluster.
-    :param disable_verify_ssl: Set to ``True`` if SSL verification should be disabled.
-    :param disable_tcp_keepalive: Set to ``True`` if you want to disable keepalive logic.
+        in your kubeconfig
+    :param config_file: Path to kubeconfig file
+    :param in_cluster: Set to ``True`` if running from within a kubernetes cluster
+    :param disable_verify_ssl: Set to ``True`` if SSL verification should be disabled
+    :param disable_tcp_keepalive: Set to ``True`` if you want to disable keepalive logic
     """

Review Comment:
   I appretiate the punctiliousness, however would you please revert these lines?  
   Period at end of line in param docstring is not an established convention for airflow.  So it's not really worth the obfuscation of the git history.  Adding these kinds of unrelated changes to PRs is a distraction for reviewers.  I don't mind it when it's something uncontroversial / obviously positive, like fixing a typo.  But here a developer could go either way and no one would care, so we shouldn't change it for no reason.
   
   Same for the capitalization you've introduced in comments below.



##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -301,36 +301,39 @@ def create_custom_object(
     ):
         """
         Creates custom resource definition object in Kubernetes
-
         :param group: api group
         :param version: api version
         :param plural: api plural
         :param body: crd object definition
         :param namespace: kubernetes namespace
         """
         api = client.CustomObjectsApi(self.api_client)
+
         if namespace is None:
             namespace = self.get_namespace()
-        if isinstance(body, str):
-            body_dict = _load_body_to_dict(body)
-        else:
-            body_dict = body

Review Comment:
   again here is a completely unrelated change that is purely stylistic.  in this situation i think it's best to just leave it alone.  oneline vs if / else is the stylistic choice of the original developer, and it's perfectly acceptable. meanwhile, every change has risk of introducing bug and it's also a distraction for the reviewer.



##########
airflow/providers/cncf/kubernetes/hooks/kubernetes.py:
##########
@@ -126,8 +126,8 @@ def __init__(
 
         self._is_in_cluster: bool | None = None
 
-        # these params used for transition in KPO to K8s hook
-        # for a deprecation period we will continue to consider k8s settings from airflow.cfg

Review Comment:
   best to leave it alone



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