You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/18 08:39:38 UTC
[GitHub] [airflow] morooshka opened a new pull request, #25787: Fix KubernetesHook failed on an attribute metadata:name absence
morooshka opened a new pull request, #25787:
URL: https://github.com/apache/airflow/pull/25787
closes: #25668
related: #25668
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r976051758
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"]:
Review Comment:
Hi Daniel! Recently there was no functionality of removing already launched objects. So no deleting is ok. Then it was implemented, but the author thought the "name" attribute is mandatory. It is not true - "generateName" can be used. So we have 2 cases:
1. "name" is used. OK, we can delete the old object if found
2. "name" is not used. It is possible if "generateName" is used. If so, we can not delete the old object - we do not know its name (k8s will generate it randomly using a name mask, but we cant guess it for sure). So we have nothing to do there, the old object will be not touched if exists
Hope this sounds understandable, please ask further to clear the case
Thank you
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1307189938
@potiuk @dstandish @eladkal thank you for your help and mentoring! Hope to continue contributing
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1294874943
I followed all the recommendations above and made corrections
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1009060594
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
Sorry, I have clicked "request review" and something changed itself. I have deleted the news fragment from PR as you asked. Is it ok?
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1003417209
##########
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:
The rule of thumb here is that if possible - such changes should be separted out. This makes it easier for cherry-picking (in case we decide to cherry-pick your change to 2.4 branch it might cause unnecessary conflicts that we will have to resolve. So I suggest you split that change out into a separate PR.
This is basically the rule that everyone shoudl follow - when you make a bugfix or change, avoid doing unrelated refactoring and separate it out to a separate PR. We are continiuously doing it and while it des not always work, it is something we strive for.
This is much more important for Airlfow's and Core provider's code - because it allows cherry-picking and produces much better changelogs (we use commit messages to automatically generate changelog. So this is rather justified requirement that has very good reasons and I kindly ask you to follow that.
--
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
[GitHub] [airflow] potiuk merged pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25787:
URL: https://github.com/apache/airflow/pull/25787
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r976052846
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"].keys():
Review Comment:
Daniel, thank you, I will follow that style. I see someone made a commit - so I do not need to make an additional patch?
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1289172446
This one needs rebase/conflict resolution after applying string normalization (See slack and devlist for information). Ths might be good opportunity to remove the unrelated changes and move them to separate PR if needed.
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251010577
> Let's see. we skip some unnneded checks (based on what your change brings) but I also have to approve the rest of your workflow because you are "new' user.
The skipped checks we reported as failed by email to me:
<img width="677" alt="image" src="https://user-images.githubusercontent.com/72786791/191026560-bb527f10-216c-4d74-a012-7919d78eac0c.png">
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by 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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1264418136
Hi @potiuk! How can we go further?
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r992503951
##########
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:
well it's certainly not worth debating so, as you wish. but going forward you might consider disabling those specific warnings since... they will pop up all over the codebase
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r992505196
##########
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:
same here, as you wish
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1007693637
##########
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:
OK, got it
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251019458
All our tests for PRs from fork run in Airflow repo's GitHub Actions
--
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
[GitHub] [airflow] dstandish commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1273566612
i hesitated suggesting reverting your unrelated changes cus i didn't want to be too nitpicky but after looking again, i do think it's best to revert those changes if you wouldn't mind since you're going to be rebasing anyway.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1003419073
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
Yep. No need for that (and it a bit harmful if it stays here as it will impact airflow's changelog)..
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1007696417
##########
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:
OK, thank you for comments
I see how high communications skills required from you to rule this project =)
I do appreciate this
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1298662813
@potiuk Can you help with restarting skipped build stage please?
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1296418259
I thinkn you need to solve conflicts now :( @eladkal Are you ok ?
--
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
[GitHub] [airflow] mik-laj commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
mik-laj commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r975137002
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"].keys():
Review Comment:
```suggestion
if "name" in body_dict["metadata"]:
```
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1241921076
@potiuk The problem had gone. I was so mind fixed that it is not connected with my changes that I really missed a piece of code deleted by occasion. Thank you, you forced me to recheck
--
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
[GitHub] [airflow] dstandish commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1274914016
just need to resolve conflicts here
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r975143355
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"].keys():
Review Comment:
Hi Kamil! Thank you for joining reviewing my PR. I use exact .keys() mention because its more definitely. Is it bad style?
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251018892
> > Let's see. we skip some unnneded checks (based on what your change brings) but I also have to approve the rest of your workflow because you are "new' user.
>
> The skipped checks we reported as failed by email to me: <img alt="image" width="677" src="https://user-images.githubusercontent.com/72786791/191026560-bb527f10-216c-4d74-a012-7919d78eac0c.png">
Those are likely tests that are run in your fork "main" after synchronizing the changes. You should likely disable the workflows in your own fork.
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251007400
Let's see. we skip some unnneded checks (based on what your change brings) but I also have to approve the rest of your workflow because you are "new' user.
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r997318612
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
providers releases don't use news fragment so we should remove this rst file.
The change log will be automatically created from the commit message
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1009039477
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
It was m ntioned in the issue template but you deleted its content and replaced it with the text you wrote in the issue :)
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1219198262
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1250381632
And now. -you have some conflicts to solve and rebase, look at the Helm isssues and see if those are your problems and fix them if nedeed.
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251999949
No. It's ok. But I think @jedcunningham aand @dstandish / @ephraimbuddy shoudl take a look at it :)
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251007767
So some checks will still run
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1272804607
<img width="1015" alt="Screenshot 2022-10-10 at 00 28 09" src="https://user-images.githubusercontent.com/595491/194803528-5aa1746b-4f1e-48c1-a1d7-f16078b33947.png">
Start with resolving conflicts and ask here for merging. I am going for holidays now, so won't be able to merge it, but others might.
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1009039477
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
It was mentioned in the issue template but you deleted its content and replaced it with the text you wrote in the issue :)
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1307175160
Awesome work, congrats on your first merged pull request!
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251002481
> And now. -you have some conflicts to solve and rebase, look at the Helm isssues and see if those are your problems and fix them if nedeed.
Hi Jarek! Fixed. Some CI pipes were skipped, I have reported to Slack. I do not understand why and do not understand how to get the reason. However, the checks were successful 😁
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1003410061
##########
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:
Yep. We have the rule, that if we want to follow certain standards, we enforce it with pre-commit so that it can be kept coinsistently across the codebase- if there is no rule for that, modifying code because "someone's" warnings are showing up is not a consistent approach and suffers the "works-for-me" syndrome.
We should either turn such things in a rule (which I think we should not in this case) or refrain from changing the code to satisfy the "individual" rule. PyCharm has a way to disable such rules individually - you can store it in your project after checking out. Since we do not "enforce" common configuration for PyCharm/Intellij etc. we do not want to share IDE configuration - instead we opted for .gitignoring known tool configurations so whatever you change in your Pycharm's project config will not be committed to the repo - feel free to change it.
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r992205928
##########
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 have changed these due to pycharm weak warnings. And the pros of these just not to stop an eye on these week warnings
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r992202041
##########
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:
I would like to object here. In such ternary operations the special construction is a good style. Also we need type hint, which in its original form will be not good:
```
if isinstance(body, str):
body_dict: dict = _load_body_to_dict(body)
else:
body_dict: dict = body
```
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r992202041
##########
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:
I would like to object here. In such ternary operations the special construction is a good style. Also we need type hint, which in its original form will be not good:
`
if isinstance(body, str):
body_dict: dict = _load_body_to_dict(body)
else:
body_dict: dict = body
`
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1296921787
@potiuk @eladkal conflicts resolved, it is being builded. In the previous try I have done the same but the main branch had succeeded to go further
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1242451108
@potiuk Sure, I understand the mission =)
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r975537161
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"].keys():
Review Comment:
yeah you don't need to, it's commonly accepted to do `if key in dict`
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1003423386
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"]:
Review Comment:
I tihnk it would be great if you add some of that context to commit message. While it links to the issue, it's always good to summarse the context in the commit message so that someone looking at the message will be able to understand the context (rather than having to read the whole issue).
--
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
[GitHub] [airflow] morooshka commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1007697019
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
OK. But this was not clear from docs
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r1003419073
##########
newsfragments/21092.bugfix.rst:
##########
@@ -0,0 +1 @@
+``KubernetesHook`` failed on a manifest field ``metadata:name`` absence.
Review Comment:
Yep. No need for that.
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1241345346
There are errors to fix.
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1241785121
> There are errors to fix.
@potiuk thank you for your message. I see some tests failing, but I consider these are not connected with my changes. How can I resolve the case? I have made several rebases on apache/main and the number of falling tests changes from iteration to iteration. What is the best practice with this? Thank you
--
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
[GitHub] [airflow] potiuk commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1242023610
> @potiuk The problem had gone. I was so mind fixed that it is not connected with my changes that I really missed a piece of code deleted by occasion. Thank you, you forced me to recheck
:D -> yeah complexity of our CI leads it to some false negatives that might cloud the understanding which changes are causing which problems. Actually - we have a main failure currently that needs to be fixed that WILL likely fail in your PR so brace for it.
I wish we had the situaton that we have super-stable and solid CI where we have 100% certaintty about the roor cause of the problem but this is IMHO very nice goal to have but very unrealistic to achieve in practice for any system which has sizeable complexity. It's pretty much always a moving target and we need to accept it as a fact of life (and strive to improve it continuousy without the false hopes we will ever achieve it "for ever". There are brief periods where it might work and the most we can achieve is to make those periods longere and more "stable".
--
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
[GitHub] [airflow] dstandish commented on a diff in pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
dstandish commented on code in PR #25787:
URL: https://github.com/apache/airflow/pull/25787#discussion_r975546116
##########
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
- try:
- api.delete_namespaced_custom_object(
- group=group,
- version=version,
- namespace=namespace,
- plural=plural,
- name=body_dict["metadata"]["name"],
- )
- self.log.warning("Deleted SparkApplication with the same name.")
- except client.rest.ApiException:
- self.log.info("SparkApp %s not found.", body_dict['metadata']['name'])
+
+ body_dict: dict = _load_body_to_dict(body) if isinstance(body, str) else body
+
+ # Attribute "name" is not mandatory if "generateName" is used instead
+ if "name" in body_dict["metadata"]:
Review Comment:
can you clarify what you're doing here with this change?
you say
> Attribute "name" is not mandatory if "generateName" is used instead
which suggests, you can still delete the object without `name` as long as `generateName` is present.
but, with your change, if `name` is not present, it appears you won't attempt to delete the object at all!
is it not required to delete the object here? am i missing something?
--
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
[GitHub] [airflow] morooshka commented on pull request #25787: Fix KubernetesHook failed on an attribute metadata:name absence
Posted by GitBox <gi...@apache.org>.
morooshka commented on PR #25787:
URL: https://github.com/apache/airflow/pull/25787#issuecomment-1251866972
> So some checks will still run
Hi Jarek! Finally all checks succeeded. Do I need to rebase again? The main head had gone away again =)
--
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