You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "okirialbert (via GitHub)" <gi...@apache.org> on 2024/02/18 15:27:33 UTC
[PR] templated fields logic checks for cloud_storage_transfer_service [airflow]
okirialbert opened a new pull request, #37519:
URL: https://github.com/apache/airflow/pull/37519
<!--
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 [newsfragments](https://github.com/apache/airflow/tree/main/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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951525606
The list of files that needs to be fixed is in:
https://github.com/apache/airflow/blob/e5688b9ae9556f3e6da3b55437bea803fd0bb444/.pre-commit-config.yaml#L328-L335
Thus like what @romsharon98 mentioned. i am not sure how this PR relates to https://github.com/apache/airflow/issues/36484 ?
Can you explain?
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951930764
Just wondering, anyone noticed this lines above of the changes 🙄 😵💫
https://github.com/apache/airflow/blob/2bcfe543c7781cd06430ff055864b811768b46ba/airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py#L549-L556
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1493804426
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
shall we consider it 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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1987333925
@okirialbert can you rebase and fix conflicts?
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951624806
The service had minor deprecating changes I opted to take care of that were relating to #36484
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951367537
@romsharon98 Kindly review.
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1493806450
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
Agree there is should be also. We never know is someone overwrite some logic of the operators
```python
@property
@deprecated(
reason="`filter` is deprecated and will be removed in the future. Please use `request_filter` instead.",
category=AirflowProviderDeprecationWarning,
)
def filter(self) -> dict | None:
"""Alias for ``request_filter``, used for compatibility (deprecated)."""
return self.request_filter
```
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-2001933547
Needs rebase and resolve conflicts
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1493806253
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
Not really. I don't think the scope of the effect of the changes makes it so.
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951942895
Hey @Taragolis, I presumed the already implemented `filter` check ensures parameter backward compatibility and your suggestion accounts for property declaration
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1988097011
@eladkal yeah sure
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #37519:
URL: https://github.com/apache/airflow/pull/37519
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1521227464
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -547,22 +547,27 @@ def __init__(
**kwargs,
) -> None:
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
self.project_id = project_id
self.gcp_conn_id = gcp_conn_id
self.api_version = api_version
self.google_impersonation_chain = google_impersonation_chain
+ @property
Review Comment:
Yes, I realized that's how the parameter is documented in Google's 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
Re: [PR] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1520234154
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -547,22 +547,27 @@ def __init__(
**kwargs,
) -> None:
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
self.project_id = project_id
self.gcp_conn_id = gcp_conn_id
self.api_version = api_version
self.google_impersonation_chain = google_impersonation_chain
+ @property
Review Comment:
Did you overlook the suggested `@deprecated` decorator?
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1494109913
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
You're right. I think the suggestion by @Taragolis should be added. The push was to make `request_filter` the standard while allowing for backward compatibility.
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1953809792
Hi @Taragolis should I add your code, replace or leave the changes as they are?
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1527144657
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -547,22 +547,27 @@ def __init__(
**kwargs,
) -> None:
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
self.project_id = project_id
self.gcp_conn_id = gcp_conn_id
self.api_version = api_version
self.google_impersonation_chain = google_impersonation_chain
+ @property
Review Comment:
@okirialbert can you make the 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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1527147516
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -547,22 +547,27 @@ def __init__(
**kwargs,
) -> None:
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
self.project_id = project_id
self.gcp_conn_id = gcp_conn_id
self.api_version = api_version
self.google_impersonation_chain = google_impersonation_chain
+ @property
Review Comment:
> @okirialbert can you make the change?
It should be ok without the deprecation, see discussion above.
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1493806450
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
Agree there is should be also. We never know is someone overwrite some logic of the operators
```python
@property
@deprecated(
reason="`filter` is deprecated and will be removed in the future. Please use `request_filter` instead.",
category=AirflowProviderDeprecationWarning,
)
def filter(self) -> str | None:
"""Alias for ``request_filter``, used for compatibility (deprecated)."""
return self.request_filter
```
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951389283
And also required adopt failing tests
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #37519:
URL: https://github.com/apache/airflow/pull/37519#issuecomment-1951451185
can u explain the purpose of this PR?
In this issue https://github.com/apache/airflow/issues/36484
[cloud_storage_transfer_service.py](https://github.com/apache/airflow/issues/36484#:~:text=airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py) is shown as not implemented but when running pre-commit on it it looks like it already taken care.
also in the [pre-commit-config](https://github.com/apache/airflow/blob/main/.pre-commit-config.yaml#L325-L335) the file is not excluded (meaning template field works)
maybe someone has fixed it but not related it to the story.
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #37519:
URL: https://github.com/apache/airflow/pull/37519#discussion_r1494070490
##########
airflow/providers/google/cloud/operators/cloud_storage_transfer_service.py:
##########
@@ -556,24 +556,24 @@ def __init__(
TypeError("__init__() missing 1 required positional argument: 'request_filter'")
super().__init__(**kwargs)
- self.filter = request_filter
+ self.request_filter = request_filter
Review Comment:
the instance variable is usually breaking change because a user might be using it in an extended class.
@okirialbert I like the code above that @Taragolis has shared. I was thinking either we should add the above code or remove `filter` param completely and make `request_filter` required. wdyt ?
--
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] templated fields logic checks for cloud_storage_transfer_service [airflow]
Posted by "okirialbert (via GitHub)" <gi...@apache.org>.
okirialbert closed pull request #37519: templated fields logic checks for cloud_storage_transfer_service
URL: https://github.com/apache/airflow/pull/37519
--
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