You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2025/04/11 17:58:12 UTC
[PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
vincbeck opened a new pull request, #49138:
URL: https://github.com/apache/airflow/pull/49138
#48192 introduced a bug in `EksClusterStateSensor`. `region` is specified in the list templated fields but ot saved as attribute which causes:
```
[2025-04-11 17:18:06,028] {taskinstance.py:2948} ERROR - Task failed with exception
Traceback (most recent call last):
File "/opt/airflow/task-sdk/src/airflow/sdk/definitions/_internal/abstractoperator.py", line 353, in _do_render_template_fields
value = getattr(parent, attr_name)
AttributeError: 'EksClusterStateSensor' object has no attribute 'region'
```
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
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 an 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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040173385
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
It shouldn't break. Templated fields are evaluated before execute() runs.
We can test this esasly by removing region from the list and add unit test that set `region="{{ ds }}" `, then verify region_name has the templated value
--
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
Re: [PR] Fix `region` parameter in EKS operators/sensors [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2042404109
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
@eladkal
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040123420
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
Correct me if I am wrong but...
If region_name is templated and we assign the vakue to it, then we can just remive region from the list without causing a breaking change
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040147279
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
That would not break dags if a user set `region` using a template?
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040123420
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
Correct me if I am wrong but...
If region_name is templated and we assign the value to it, then we can just remove region from the list without causing a breaking change
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040153596
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
Since we save the value in `region_name` and it is the templated field, maybe not actually. As far as I know, the interpretation of templated fields happens between the init and the execute? If yes, then yes I think you're right
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040368582
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
I updated the PR, I think now it makes more sense.
--
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
Re: [PR] Fix `region` parameter in EKS operators/sensors [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #49138:
URL: https://github.com/apache/airflow/pull/49138
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040173385
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
It shouldn't break. Templated fields are evaluated before execute() runs.
We can test this esasly by unit test that set region="{{ ds }}" and then verify region_name had the templated value
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040117667
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
`region_name` is already injected by `aws_template_fields`. Here we keep `region` to be backward compatible. If `region` is set, then we assign it to `region_name`
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040048930
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
Is this the right fix? I think we should replace the template field to region_name in a backward compatible way
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040120268
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
We need to save it as an attribute because it is listed in template fields. Any field in template fields should also be an attribute.
--
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
Re: [PR] Fix `EksClusterStateSensor`. Save `region` as attribute [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49138:
URL: https://github.com/apache/airflow/pull/49138#discussion_r2040173385
##########
providers/amazon/src/airflow/providers/amazon/aws/sensors/eks.py:
##########
@@ -145,6 +145,7 @@ def __init__(
**kwargs,
):
super().__init__(target_state=target_state, target_state_type=ClusterStates, **kwargs)
+ self.region = region
Review Comment:
It shouldn't break. Templated fields are evaluated before execute() runs.
We can test this esasly by unit test that set region="{{ ds }}" and then verify region_name has the templated value
--
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