Skip to content

templated fields logic checks for cloud_storage_transfer_service - #37519

Merged
eladkal merged 5 commits into
apache:mainfrom
okirialbert:templated_fields_logic_checks
Mar 16, 2024
Merged

templated fields logic checks for cloud_storage_transfer_service#37519
eladkal merged 5 commits into
apache:mainfrom
okirialbert:templated_fields_logic_checks

Conversation

@okirialbert

@okirialbert okirialbert commented Feb 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Feb 18, 2024
@okirialbert okirialbert mentioned this pull request Feb 18, 2024
Closed
27 tasks done
@okirialbert

okirialbert commented Feb 18, 2024

Copy link
Copy Markdown
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

Taragolis commented Feb 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

And also required fix failing tests

@romsharon98

romsharon98 commented Feb 18, 2024

Copy link
Copy Markdown
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

eladkal commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

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

airflow/.pre-commit-config.yaml

Lines 328 to 335 in e5688b9

^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

okirialbert commented Feb 19, 2024

Copy link
Copy Markdown
Contributor Author

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

@Taragolis

Taragolis commented Feb 19, 2024

Copy link
Copy Markdown
Contributor

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

@okirialbert

okirialbert commented Feb 19, 2024

Copy link
Copy Markdown
Contributor Author

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

@okirialbert

okirialbert commented Feb 20, 2024

Copy link
Copy Markdown
Contributor Author

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

@eladkal

eladkal commented Mar 10, 2024

Copy link
Copy Markdown
Contributor

@okirialbert can you rebase and fix conflicts?

@okirialbert

okirialbert commented Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

@eladkal yeah sure

@okirialbert
okirialbert force-pushed the templated_fields_logic_checks branch from 2bcfe54 to c6f3439 Compare March 11, 2024 13:57
@okirialbert okirialbert reopened this Mar 11, 2024
shahar1
shahar1 approved these changes Mar 11, 2024
View reviewed changes

@shahar1 shahar1 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

eladkal commented Mar 16, 2024

Copy link
Copy Markdown
Contributor

Needs rebase and resolve conflicts

@eladkal
eladkal merged commit 0acd6b2 into apache:main Mar 16, 2024
@eladkal eladkal mentioned this pull request Apr 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shahar1 shahar1 shahar1 approved these changes
@eladkal eladkal eladkal approved these changes
@Taragolis Taragolis Awaiting requested review from Taragolis
@pankajastro pankajastro Awaiting requested review from pankajastro

Assignees

No one assigned

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@okirialbert @Taragolis @romsharon98 @eladkal @shahar1 @pankajastro