Skip to content

Provider Migration: Update Oracle for Airflow 3.0 compatibility #52382

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 28, 2025
Merged

Provider Migration: Update Oracle for Airflow 3.0 compatibility #52382

merged 2 commits into from
Jun 28, 2025
+49 −14

Conversation

sjyangkevin
Copy link
Contributor

Follow-up of #52292. Part of #52378

Step 3: Fix BaseOperatorLink.persist methods

  • didn't find persist method in the operator

Step 4: Update test patterns

  • didn't find test invoke operator.run()
  • find a task instance run as shown below. Wondering if any action is needed.
dr = dag_maker.create_dagrun(run_id=task_id)
    ti = TaskInstance(task=task, run_id=dr.run_id)
    with pytest.raises(oracledb.DatabaseError, match=re.escape(error)):
        ti.run()
    assert ti.xcom_pull(task_ids=task.task_id, key="ORA") == ora_exit_code

^ 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 airflow-core/newsfragments.

@potiuk potiuk force-pushed the issues/52378/oracle-provider-port-task-sdk branch from 7c3231e to 3c2b698 Compare June 28, 2025 13:23
dominikhei
dominikhei reviewed Jun 28, 2025
View reviewed changes
Copy link
Contributor

@dominikhei dominikhei left a comment

Choose a reason for hiding this comment

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

You need to add test_version_compat.py to the OVERLOOKED_TESTS in airflow-core/tests/unit/alway/test_project_structure.py.
@potiuk why don't we filter them out by default just like build files, e.g:
modules_files = (f for f in modules_files if "version_compat.py" not in f.parts)

@sjyangkevin
Copy link
Contributor Author

You need to add test_version_compat.py to the OVERLOOKED_TESTS in airflow-core/tests/unit/alway/test_project_structure.py.

Yes. I checked the latest instruction early today, and working on making the changes. Thanks for the feedback! I also noticed the file is not actually present in the test folder. I would appreciate if I could get your help to understand more why we need the OVERLOOKED_TESTS list for this change.

@dominikhei
Copy link
Contributor

dominikhei commented Jun 28, 2025 •
edited
Loading

You need to add test_version_compat.py to the OVERLOOKED_TESTS in airflow-core/tests/unit/alway/test_project_structure.py.

Yes. I checked the latest instruction early today, and working on making the changes. Thanks for the feedback! I also noticed the file is not actually present in the test folder. I would appreciate if I could get your help to understand more why we need the OVERLOOKED_TESTS list for this change.

It ensures that for every file in providers/, there exists a corresponding test file.
For example, if you create a new provider xyz and add an XYZOperator, the test checks that a matching file like test_xyz_operator.py exists and that the operator's functionality is covered.
However some files inside the providers folder are "support" files, not directly part of the provider logic, and thus don't necessarily require unit tests, just like version_compat.py, which is why they are added to the list and thus not checked for :) Hope that helps?

@sjyangkevin
Copy link
Contributor Author

You need to add test_version_compat.py to the OVERLOOKED_TESTS in airflow-core/tests/unit/alway/test_project_structure.py.

Yes. I checked the latest instruction early today, and working on making the changes. Thanks for the feedback! I also noticed the file is not actually present in the test folder. I would appreciate if I could get your help to understand more why we need the OVERLOOKED_TESTS list for this change.

It ensures that for every file in providers/, there exists a corresponding test file. For example, if you create a new provider xyz and add an XYZOperator, the test checks that a matching file like test_xyz_operator.py exists and that the operator's functionality is covered. However some files inside the providers folder are "support" files, not directly part of the provider logic, and thus don't necessarily require unit tests, just like version_compat.py, which is why they are added to the list and thus not checked for :) Hope that helps?

It's really helpful! Appreciate it

@sjyangkevin sjyangkevin force-pushed the issues/52378/oracle-provider-port-task-sdk branch from 3c2b698 to 557a796 Compare June 28, 2025 20:49
@sjyangkevin sjyangkevin requested review from potiuk and dominikhei June 28, 2025 21:12
@potiuk potiuk merged commit aa2342d into apache:main Jun 28, 2025
69 checks passed
@potiuk potiuk mentioned this pull request Jun 28, 2025
Open
97 tasks
@kyungjunleeme
Copy link
Contributor

@potiuk @sjyangkevin hello, plz check!!!

This PR cause failure on ci process in local, and remote github ci environment

in local
image

in github ci
https://github.com/apache/airflow/actions/runs/15951959325/job/44993314888
image

@potiuk
Copy link
Member

potiuk commented Jun 29, 2025

Already fixed. You need to rebase to latest main @kyungjunleeme

(ABR) (A)lways (B)e (R)ebased.

@kyungjunleeme
Copy link
Contributor

@potiuk ah ha! Okay thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@potiuk potiuk potiuk approved these changes

@dominikhei dominikhei Awaiting requested review from dominikhei

Assignees
No one assigned
Labels
area:providers provider:oracle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sjyangkevin @dominikhei @kyungjunleeme @potiuk