Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

templated fields logic checks for cloud_storage_transfer_service #37519

Merged
merged 5 commits into from
Mar 16, 2024

Conversation

okirialbert
Copy link
Contributor

@okirialbert okirialbert commented Feb 18, 2024


The PR fixes assignment of parameters in constructor initialization for the google cloud_storage_transfer_service ensuring templated fields' parameters correspond to constructor initialization.

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@okirialbert
Copy link
Contributor Author

@romsharon98 Kindly review.

@okirialbert okirialbert marked this pull request as draft February 18, 2024 15:50
@okirialbert okirialbert marked this pull request as ready for review February 18, 2024 15:50
@Taragolis
Copy link
Contributor

Taragolis commented Feb 18, 2024

And also required fix failing tests

@romsharon98
Copy link
Contributor

can u explain the purpose of this PR?
In this issue #36484
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 the file is not excluded (meaning template field works)
maybe someone has fixed it but not related it to the story.

@eladkal
Copy link
Contributor

eladkal commented Feb 19, 2024

The list of files that needs to be fixed is in:

^airflow\/providers\/google\/cloud\/operators\/bigquery\.py$|
^airflow\/providers\/amazon\/aws\/transfers\/gcs_to_s3\.py$|
^airflow\/providers\/databricks\/operators\/databricks\.py$|
^airflow\/providers\/google\/cloud\/transfers\/bigquery_to_mysql\.py$|
^airflow\/providers\/amazon\/aws\/transfers\/redshift_to_s3\.py$|
^airflow\/providers\/google\/cloud\/operators\/compute\.py$|
^airflow\/providers\/amazon\/aws\/operators\/emr\.py$|
^airflow\/providers\/amazon\/aws\/operators\/eks\.py$

Thus like what @romsharon98 mentioned. i am not sure how this PR relates to #36484 ?
Can you explain?

@okirialbert
Copy link
Contributor Author

The service had minor deprecating changes I opted to take care of that were relating to #36484

@Taragolis
Copy link
Contributor

Just wondering, anyone noticed this lines above of the changes 🙄 😵‍💫

if request_filter is None:
if "filter" in kwargs:
request_filter = kwargs["filter"]
AirflowProviderDeprecationWarning(
"Use 'request_filter' instead 'filter' to pass the argument."
)
else:
TypeError("__init__() missing 1 required positional argument: 'request_filter'")

@okirialbert
Copy link
Contributor Author

Hey @Taragolis, I presumed the already implemented filter check ensures parameter backward compatibility and your suggestion accounts for property declaration

@okirialbert
Copy link
Contributor Author

Hi @Taragolis should I add your code, replace or leave the changes as they are?

@eladkal
Copy link
Contributor

eladkal commented Mar 10, 2024

@okirialbert can you rebase and fix conflicts?

@okirialbert
Copy link
Contributor Author

@eladkal yeah sure

@okirialbert okirialbert reopened this Mar 11, 2024
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with small comment :)

@eladkal
Copy link
Contributor

eladkal commented Mar 16, 2024

Needs rebase and resolve conflicts

@eladkal eladkal merged commit 0acd6b2 into apache:main Mar 16, 2024
52 checks passed
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
…che#37519)

* templated fields init check and test update

* update pre-commit file list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants