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

Trigger openlineage test when asset files changes #44172

Merged

Conversation

vatsrahul1001
Copy link
Collaborator

@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
Collaborator Author

Two small comments.

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

@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
Collaborator 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
Collaborator 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

@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
Collaborator 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
95 checks passed
@potiuk
Copy link
Member

potiuk commented Nov 22, 2024

Good to go, merged :)

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