Skip to content

Conversation

@vatsrahul1001
Copy link
Contributor

@vatsrahul1001 vatsrahul1001 commented Nov 19, 2024

Description

Adding FileGroupForCi.ASSET_FILES and running Openlineage test when Assets files are changed.

Motivation:

We recently noticed PR broke OL tests as openlineage tests heavily rely on assets and are often broken after asset changes. We want to trigger OL test whenever there is a change in Asset related files.

closes: #44026


^ 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.

@vatsrahul1001 vatsrahul1001 changed the title trigger openlineage test when asset files changes Trigger openlineage test when asset files changes Nov 19, 2024
@vatsrahul1001 vatsrahul1001 marked this pull request as draft November 19, 2024 05:28
@vatsrahul1001 vatsrahul1001 marked this pull request as ready for review November 19, 2024 08:19
@vatsrahul1001 vatsrahul1001 force-pushed the trigger-openlineage-tests-on-asset-change branch from f6f0a93 to cbc69b5 Compare November 19, 2024 11:49
@potiuk
Copy link
Member

potiuk commented Nov 19, 2024

Two small comments.

@vatsrahul1001
Copy link
Contributor Author

Two small comments.

Thank you for the suggestions. I have implemented the suggested changes.

@vatsrahul1001 vatsrahul1001 requested a review from potiuk November 20, 2024 03:26
@uranusjr
Copy link
Member

Can you elaborate what broke in OL? I’m not sure airflow/dag_processing/collection.py and airflow/timetables/assets.py need to trigger OL tests. Those aren’t really about assets, but more using assets.

@vatsrahul1001
Copy link
Contributor Author

#44026

Can you elaborate what broke in OL? I’m not sure airflow/dag_processing/collection.py and airflow/timetables/assets.py need to trigger OL tests. Those aren’t really about assets, but more using assets.

@uranusjr This issue explain more about what broke in OL. Also, this PR was raised to fix OL tests. I asked @Lee-W for the assets file we can remove these files airflow/dag_processing/collection.py and airflow/timetables/assets.py if needed.

@Lee-W
Copy link
Member

Lee-W commented Nov 20, 2024

#44026

Can you elaborate what broke in OL? I’m not sure airflow/dag_processing/collection.py and airflow/timetables/assets.py need to trigger OL tests. Those aren’t really about assets, but more using assets.

@uranusjr This issue explain more about what broke in OL. Also, this PR was raised to fix OL tests. I asked @Lee-W for the assets file we can remove these files airflow/dag_processing/collection.py and airflow/timetables/assets.py if needed.

I believe @uranusjr's point is more convincing. Although those are related to assets, they are using assets and are unlikely to affect OL. I should have thought it through more thoroughly.

@vatsrahul1001
Copy link
Contributor Author

#44026

Can you elaborate what broke in OL? I’m not sure airflow/dag_processing/collection.py and airflow/timetables/assets.py need to trigger OL tests. Those aren’t really about assets, but more using assets.

@uranusjr This issue explain more about what broke in OL. Also, this PR was raised to fix OL tests. I asked @Lee-W for the assets file we can remove these files airflow/dag_processing/collection.py and airflow/timetables/assets.py if needed.

I believe @uranusjr's point is more convincing. Although those are related to assets, they are using assets and are unlikely to affect OL. I should have thought it through more thoroughly.

Thanks, @Lee-W and @uranusjr for the suggestion. I will remove airflow/dag_processing/collection.py and airflow/timetables/assets.py

@vatsrahul1001 vatsrahul1001 requested a review from Lee-W November 20, 2024 14:07
@vatsrahul1001 vatsrahul1001 requested a review from Lee-W November 20, 2024 14:30
@Lee-W
Copy link
Member

Lee-W commented Nov 22, 2024

@vatsrahul1001 should we resolve the discussions or are we waiting for @potiuk 's confirmation? or

@vatsrahul1001
Copy link
Contributor Author

vatsrahul1001 commented Nov 22, 2024

@vatsrahul1001 should we resolve the discussions or are we waiting for @potiuk 's confirmation? or

We can resolve the discussion. @potiuk, is it good to merge?

@potiuk potiuk merged commit 32f064f into apache:main Nov 22, 2024
@potiuk
Copy link
Member

potiuk commented Nov 22, 2024

Good to go, merged :)

got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trigger provider openlineage tests after assets change

5 participants