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/01/04 05:02:21 UTC
[GitHub] [airflow] mrkm4ntr opened a new pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
mrkm4ntr opened a new pull request #20647:
URL: https://github.com/apache/airflow/pull/20647
The main use case of GKEStartPodOperator is running Kubernetes Pod from outside of cluster using Google Service Account. GKEStartPodOperator runs `gcloud container clusters get-credentials` to construct config file for Kubernetes. We have to set `in_cluster` parameter `False` explicitly to use that config file. So the default value of `in_cluster` param of GKEStartPodOperator should be `False`.
I also added warning for backward compatibility.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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 change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r791136255
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
- super().__init__(is_delete_operator_pod=is_delete_operator_pod, **kwargs)
+ super().__init__(is_delete_operator_pod=is_delete_operator_pod, in_cluster=in_cluster, **kwargs)
Review comment:
Wouldn't it be easier to just set in_cluster=False directly?
According to your description there is no other alternative in any case.
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -306,6 +306,9 @@ class GKEStartPodOperator(KubernetesPodOperator):
pod; if False, leave the pod. Current default is False, but this will be
changed in the next major release of this provider.
:type is_delete_operator_pod: bool
+ :param in_cluster: Run kubernetes client with in_cluster configuration. This parameter is
+ for backward compatibility. No need to change this parameter from default now.
+ :type in_cluster: bool
Review comment:
If the user can't really sent the value of this parameter then why should we expose it in the constructor?
--
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 change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r791136255
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
- super().__init__(is_delete_operator_pod=is_delete_operator_pod, **kwargs)
+ super().__init__(is_delete_operator_pod=is_delete_operator_pod, in_cluster=in_cluster, **kwargs)
Review comment:
Wouldn't it be easier to just set in_cluster=False directly?
According to your description there is no other alternative in any case.
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -306,6 +306,9 @@ class GKEStartPodOperator(KubernetesPodOperator):
pod; if False, leave the pod. Current default is False, but this will be
changed in the next major release of this provider.
:type is_delete_operator_pod: bool
+ :param in_cluster: Run kubernetes client with in_cluster configuration. This parameter is
+ for backward compatibility. No need to change this parameter from default now.
+ :type in_cluster: bool
Review comment:
If the user can't really sent the value of this parameter then why should we expose it in the constructor?
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -306,6 +306,9 @@ class GKEStartPodOperator(KubernetesPodOperator):
pod; if False, leave the pod. Current default is False, but this will be
changed in the next major release of this provider.
:type is_delete_operator_pod: bool
+ :param in_cluster: Run kubernetes client with in_cluster configuration. This parameter is
+ for backward compatibility. No need to change this parameter from default now.
+ :type in_cluster: bool
Review comment:
If the user can't set the value of this parameter then why should we expose it in the constructor?
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
- super().__init__(is_delete_operator_pod=is_delete_operator_pod, **kwargs)
+ super().__init__(is_delete_operator_pod=is_delete_operator_pod, in_cluster=in_cluster, **kwargs)
Review comment:
Wouldn't it be easier to just set `in_cluster=False` directly?
According to your description there is no other alternative in any case.
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
this can be explained in the operator documentation this is not really a deprecation warning.
--
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 pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#issuecomment-1006182006
It looks like a breaking change, but I'm not sure. What do you think?
--
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 change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r791135881
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -306,6 +306,9 @@ class GKEStartPodOperator(KubernetesPodOperator):
pod; if False, leave the pod. Current default is False, but this will be
changed in the next major release of this provider.
:type is_delete_operator_pod: bool
+ :param in_cluster: Run kubernetes client with in_cluster configuration. This parameter is
+ for backward compatibility. No need to change this parameter from default now.
+ :type in_cluster: bool
Review comment:
If the user can't set the value of this parameter then why should we expose it in the constructor?
--
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] github-actions[bot] closed pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #20647:
URL: https://github.com/apache/airflow/pull/20647
--
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] mrkm4ntr commented on a change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
mrkm4ntr commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r795421083
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
- super().__init__(is_delete_operator_pod=is_delete_operator_pod, **kwargs)
+ super().__init__(is_delete_operator_pod=is_delete_operator_pod, in_cluster=in_cluster, **kwargs)
Review comment:
I cared about the users who are using GKEPodOperator with KSA accidentally. Those users should KubernetesPodOperator instead of GKEPodOperator but I'd not like to introduce a breaking change.
If we don't have to care about those users, we can just set `in_cluster=False` directly.
--
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 change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r791136255
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
- super().__init__(is_delete_operator_pod=is_delete_operator_pod, **kwargs)
+ super().__init__(is_delete_operator_pod=is_delete_operator_pod, in_cluster=in_cluster, **kwargs)
Review comment:
Wouldn't it be easier to just set `in_cluster=False` directly?
According to your description there is no other alternative in any case.
--
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] github-actions[bot] commented on pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#issuecomment-1071888832
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 change in pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#discussion_r791137078
##########
File path: airflow/providers/google/cloud/operators/kubernetes_engine.py
##########
@@ -335,8 +339,16 @@ def __init__(
stacklevel=2,
)
is_delete_operator_pod = False
+ if in_cluster:
+ warnings.warn(
+ f"You should not set parameter `in_cluster` `True` in class {self.__class__.__name__}. "
+ "This operator uses Google Service Account(GSA) not Kubernetes Service Account(KSA)."
+ "If you can use KSA with in_cluster `True`, you should use KubernetesPodOperator directly.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
Review comment:
this can be explained in the operator documentation this is not really a deprecation warning.
--
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] github-actions[bot] commented on pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#issuecomment-1071888832
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 removed a comment on pull request #20647: Set default value of in_cluster False for GKEStartPodOperator
Posted by GitBox <gi...@apache.org>.
mik-laj removed a comment on pull request #20647:
URL: https://github.com/apache/airflow/pull/20647#issuecomment-1006182006
It looks like a breaking change, but I'm not sure. What do you think?
--
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