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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
