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/11 22:42:49 UTC
[GitHub] [airflow] ferruzzi opened a new pull request #20819: Adds support for optional kwargs in the EKS Operators
ferruzzi opened a new pull request #20819:
URL: https://github.com/apache/airflow/pull/20819
closes: https://github.com/apache/airflow/issues/19641
TL;DR: BaseOperator will throw an exception if it gets an unexpected parameter so this is used as a workaround.
Adds the option to include an operator-specific kwargs parameter to the three "Create" operators in the EKS module as seen in other Operators. Includes unit testing.
<!--
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] dstandish commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782621426
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
this is breaking because now user cannot set it to 'shared'
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
Review comment:
why this change? seems better before, more explicit about the meaning.
if x, then _what_? rather than just "if x:"
perhaps it was not correct before?
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782707712
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
Yes, I couldn't remember offhand if the previous/existing implementation `{**tags, **kwargs["tags"]}` overwrote the default with the user's or not. I was going to confirm how it worked before I said anything, but @dstandish caught it first.
--
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] uranusjr commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782705234
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
I don’t generally like the `x or default` pattern, but this is OK as well.
--
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 change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782707383
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
in effect the current code has a default of `owned` and i think that should be preserved without good reason for changing
--
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] ferruzzi commented on pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#issuecomment-1011461226
[42751d7](https://github.com/apache/airflow/pull/20819/commits/6d059d4af4fcaeae0d3980b49d86c73cf42751d7) should address the tag override issue, and [a84ad15](https://github.com/apache/airflow/pull/20819/commits/a84ad15e87e82b240440991ba060956ecda434d0) adds two unit tests to make sure it doesn't happen 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
[GitHub] [airflow] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782715594
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
Review comment:
Indeed. I'll get that updated while I'm in there.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782696197
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
Review comment:
We've now added some that are available but non-required. The ones that were there are required if you define compute='nodegroup' but `create_nodegroup_kwargs` is option with that set. Can you suggest a concise way to rephrase it that still makes sense? The ideas I came up with were long and/or awkward, and this sounded better.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782697271
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
I thought of that when I was looking at @uranusjr 's comment above but wanted to test it to see. Would it have preserved the user's setting the way it was before?
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782684421
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
Thanks, I didn't think of that syntax for the declaration. If we're going that route, how would you feel about simplifying it to
```
resolved_tags = tags or {}
resolved_tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
```
--
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] ferruzzi commented on pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#issuecomment-1012455430
AFAIK, all concerns raised so far have been addressed at this point.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782715126
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
:type selectors: List
+ :param create_fargate_profile_kwargs: Optional parameters to pass to the CreateFargateProfile API
+ (templated)
+ :type: Dict
"""
template_fields: Sequence[str] = (
"cluster_name",
"cluster_role_arn",
"resources_vpc_config",
+ "create_cluster_kwargs",
"compute",
"nodegroup_name",
"nodegroup_role_arn",
+ "create_nodegroup_kwargs",
"fargate_profile_name",
"fargate_pod_execution_role_arn",
"fargate_selectors",
+ "create_fargate_profile_kwargs",
Review comment:
> what do you think about dropping the create_ so it would juts be cluster_kwargs, nodegroup_kwargs, fargate_profile_kwargs?
Could do that. I left it long like this so we didn't paint ourselves into a corner. I was thinking this way gave flexibility in case other endpoints got kwargs later, or maybe we go back and decide to paginate the list operators and need a list_clusters_kwargs or something, but that's maybe needless future-proofing?
> i'm also tempted to suggest deprecating non-kwargs params (so in future we'd can only use the kwargs params) but maybe people usually only need the existing params and it's convenient to have them?
Yeah, that was something I discussed with @o-nikolas. That or implementing a hybrid where you could use them explicitly or within the cluster_kwargs and the logic would handle it for you either way, but that made the code significantly (and needlessly?) more complicated so we settled on this as the easiest non-breaking answer. 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] uranusjr commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782706766
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
If “before” means the code currently in `main`, yes it would, because `{**tags, **kwargs["tags"]}` means that if any key in `kwargs["tags"]` would override the value under the same key in `tags`.
```python
>>> a = {"x": 1}
>>> b = {"x": 2}
>>> {**a, **b}
{"x": 2}
```
So the backward compatible implementation would be something like
```python
resolved_tags = tags or {}
cluster_tag_key = f'kubernetes.io/cluster/{clusterName}'
if cluster_tag_key not in resolved_tags:
resolved_tags[cluster_tag_key] = 'owned'
```
or `setdefault` might work as well.
--
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] kaxil merged pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #20819:
URL: https://github.com/apache/airflow/pull/20819
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782707712
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
Yes, I couldn't remember offhand if the previous/existing implementation `{**tags, **kwargs["tags"]}` overwrote the default with the user's or not. I was going to confirm how it worked before I said anything, but @dstandish caught it first.
I'll get this and the above corrected in the morning.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782716657
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
Resolving this one since we picked up the conversation below in dstandish's thread.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r783459893
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
:type selectors: List
+ :param create_fargate_profile_kwargs: Optional parameters to pass to the CreateFargateProfile API
+ (templated)
+ :type: Dict
"""
template_fields: Sequence[str] = (
"cluster_name",
"cluster_role_arn",
"resources_vpc_config",
+ "create_cluster_kwargs",
"compute",
"nodegroup_name",
"nodegroup_role_arn",
+ "create_nodegroup_kwargs",
"fargate_profile_name",
"fargate_pod_execution_role_arn",
"fargate_selectors",
+ "create_fargate_profile_kwargs",
Review comment:
Let's leave deprecating that for the future. I'd like to come up with some consistent plan on how we're going to handle these throughout the AWS provider package (and beyond?) before we start making breaking changes to one service.
--
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] uranusjr commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r783765493
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
The parameter was retrieved with `kwargs.pop()`, so it can only be passed as a keyword argument to begin with. I don’t have particular reasons to make it keyword-only, but since it already is, the keyword-only argument syntax makes it more explicit.
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r784276314
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
I guess that makes sense. I've added that in https://github.com/apache/airflow/pull/20819/commits/cc12d26eb4fea209ae5c616feaffa94c95cf8a62, just wanted to make sure I understood why. Thanks
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782696197
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
Review comment:
We've now added some that are available but non-required. The ones that were there are required if you define compute='nodegroup' but `create_nodegroup_kwargs` is optional with that set. Can you suggest a concise way to rephrase it that still makes sense? The ideas I came up with were long and/or awkward, and this sounded better.
--
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 change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782727208
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
:type selectors: List
+ :param create_fargate_profile_kwargs: Optional parameters to pass to the CreateFargateProfile API
+ (templated)
+ :type: Dict
"""
template_fields: Sequence[str] = (
"cluster_name",
"cluster_role_arn",
"resources_vpc_config",
+ "create_cluster_kwargs",
"compute",
"nodegroup_name",
"nodegroup_role_arn",
+ "create_nodegroup_kwargs",
"fargate_profile_name",
"fargate_pod_execution_role_arn",
"fargate_selectors",
+ "create_fargate_profile_kwargs",
Review comment:
> Could do that. I left it long like this so we didn't paint ourselves into a corner. I was thinking this way gave flexibility in case other endpoints got kwargs later, or maybe we go back and decide to paginate the list operators and need a list_clusters_kwargs or something, but that's maybe needless future-proofing?
i leave it up to you. i like the shorter form better but being verbose is fine too.
> Yeah, that was something I discussed with @o-nikolas. That or implementing a hybrid where you could use them explicitly or within the cluster_kwargs and the logic would handle it for you either way, but that made the code significantly (and needlessly?) more complicated so we settled on this as the easiest non-breaking answer. What do you think?
yeah if we were to go this route we'd have to go hybrid first (and emit a warning whenever user supplied the non-kwargs params). and then in next major release of the provider we would chop the params. if you want to proceed down that route we could deal with that in a later PR does not have to be dealt with 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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r783453020
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
Review comment:
Resolved in https://github.com/apache/airflow/pull/20819/commits/6d059d4af4fcaeae0d3980b49d86c73cf42751d7
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782684421
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
Thanks, I didn't think of that syntax for the signature. If we're going that route, how would you feel about simplifying it to
```
resolved_tags = tags or {}
resolved_tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
```
--
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 change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782709716
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
Review comment:
unrelated but i think this is supposed to be `fargate_selectors`
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
Review comment:
yeah i understand -- some are optional now
fine with me
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
:type selectors: List
+ :param create_fargate_profile_kwargs: Optional parameters to pass to the CreateFargateProfile API
+ (templated)
+ :type: Dict
"""
template_fields: Sequence[str] = (
"cluster_name",
"cluster_role_arn",
"resources_vpc_config",
+ "create_cluster_kwargs",
"compute",
"nodegroup_name",
"nodegroup_role_arn",
+ "create_nodegroup_kwargs",
"fargate_profile_name",
"fargate_pod_execution_role_arn",
"fargate_selectors",
+ "create_fargate_profile_kwargs",
Review comment:
what do you think about dropping the `create_` so it would juts be `cluster_kwargs`, `nodegroup_kwargs`, `fargate_profile_kwargs`?
it's a bit less wordy, but i think still clear enough given that this a "create cluster" operator
i'm also tempted to suggest deprecating non-kwargs params (so in future we'd can only use the kwargs params) but maybe people usually only need the existing params and it's convenient to have them?
--
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] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782715126
##########
File path: airflow/providers/amazon/aws/operators/eks.py
##########
@@ -89,36 +91,45 @@ class EksCreateClusterOperator(BaseOperator):
If this is None or empty then the default boto3 behaviour is used.
:type region: str
- If compute is assigned the value of 'nodegroup', the following are required:
+ If compute is assigned the value of 'nodegroup':
- :param nodegroup_name: The unique name to give your Amazon EKS managed node group. (templated)
+ :param nodegroup_name: *REQUIRED* The unique name to give your Amazon EKS managed node group. (templated)
:type nodegroup_name: str
- :param nodegroup_role_arn: The Amazon Resource Name (ARN) of the IAM role to associate with the
- Amazon EKS managed node group. (templated)
+ :param nodegroup_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the IAM role to associate with
+ the Amazon EKS managed node group. (templated)
:type nodegroup_role_arn: str
+ :param create_nodegroup_kwargs: Optional parameters to pass to the CreateNodegroup API (templated)
+ :type: Dict
- If compute is assigned the value of 'fargate', the following are required:
- :param fargate_profile_name: The unique name to give your AWS Fargate profile. (templated)
+ If compute is assigned the value of 'fargate':
+
+ :param fargate_profile_name: *REQUIRED* The unique name to give your AWS Fargate profile. (templated)
:type fargate_profile_name: str
- :param fargate_pod_execution_role_arn: The Amazon Resource Name (ARN) of the pod execution role to
- use for pods that match the selectors in the AWS Fargate profile. (templated)
+ :param fargate_pod_execution_role_arn: *REQUIRED* The Amazon Resource Name (ARN) of the pod execution
+ role to use for pods that match the selectors in the AWS Fargate profile. (templated)
:type podExecutionRoleArn: str
:param selectors: The selectors to match for pods to use this AWS Fargate profile. (templated)
:type selectors: List
+ :param create_fargate_profile_kwargs: Optional parameters to pass to the CreateFargateProfile API
+ (templated)
+ :type: Dict
"""
template_fields: Sequence[str] = (
"cluster_name",
"cluster_role_arn",
"resources_vpc_config",
+ "create_cluster_kwargs",
"compute",
"nodegroup_name",
"nodegroup_role_arn",
+ "create_nodegroup_kwargs",
"fargate_profile_name",
"fargate_pod_execution_role_arn",
"fargate_selectors",
+ "create_fargate_profile_kwargs",
Review comment:
> what do you think about dropping the create_ so it would juts be cluster_kwargs, nodegroup_kwargs, fargate_profile_kwargs?
Could do that. I left it long like this so we didn't paint ourselves into a corner. I was thinking this way gave flexibility in case other endpoints got kwargs later, or maybe we go back and decide to paginate the list operators and need a list_clusters_kwargs or something, but that's maybe needless future-proofing?
> i'm also tempted to suggest deprecating non-kwargs params (so in future we'd can only use the kwargs params) but maybe people usually only need the existing params and it's convenient to have them?
Yeah, that was something I discussed with @o-nikolas. That or implementing a hybrid where you could use them explicitly or within the cluster_kwargs and the logic would handle it for you either way, but that made the code significantly (and needlessly?) more complicated so we settled on this as the easiest non-breaking answer. I had asked in the linked Issue if the requester had a preference but never heard back so I went with this. 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] github-actions[bot] commented on pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#issuecomment-1012783923
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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] uranusjr commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r783765493
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
The parameter was retrieved with `kwargs.pop()`, so it can only be passed as a keyword argument to begin with. I don’t have particular reasons to make it keyword-only, but since it already is _logically_ keyword-only, the keyword-only syntax makes the intention more explicit.
--
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] uranusjr commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r782677359
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
How about changing this function to
```python
def create_nodegroup(
self,
clusterName: str,
nodegroupName: str,
subnets: List[str],
nodeRole: str,
*,
tags: Optional[dict] = None
**kwargs,
) -> Dict:
...
if tags is not None:
resolved_tags = tags
else:
resolved_tags = {}
resolved_tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
```
to better leverage Python’s built-in syntax. The `kwargs.pop()` or `if key in kwargs` pattern was a pragmatic workaround on Python 2, but now that keyword-only arguments are available, they make the API both easier to read and write.
--
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] ferruzzi edited a comment on pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi edited a comment on pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#issuecomment-1011461226
https://github.com/apache/airflow/pull/20819/commits/6d059d4af4fcaeae0d3980b49d86c73cf42751d7 should address the tag override issue, and https://github.com/apache/airflow/pull/20819/commits/a84ad15e87e82b240440991ba060956ecda434d0 adds two unit tests to make sure it doesn't happen 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
[GitHub] [airflow] ferruzzi commented on a change in pull request #20819: Adds support for optional kwargs in the EKS Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #20819:
URL: https://github.com/apache/airflow/pull/20819#discussion_r783436088
##########
File path: airflow/providers/amazon/aws/hooks/eks.py
##########
@@ -135,13 +146,12 @@ def create_nodegroup(
:rtype: Dict
"""
eks_client = self.conn
+
# The below tag is mandatory and must have a value of either 'owned' or 'shared'
# A value of 'owned' denotes that the subnets are exclusive to the nodegroup.
# The 'shared' value allows more than one resource to use the subnet.
- tags = {'kubernetes.io/cluster/' + clusterName: 'owned'}
- if "tags" in kwargs:
- tags = {**tags, **kwargs["tags"]}
- kwargs.pop("tags")
+ tags = kwargs.pop("tags", {})
+ tags[f'kubernetes.io/cluster/{clusterName}'] = 'owned'
Review comment:
@uranusjr - Is there a particular reason why you are making the tags parameter keyword-only in your solution 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