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.

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

Migrate gcs to new system tests design #22778

Merged
merged 18 commits into from
Apr 28, 2022
Merged

Migrate gcs to new system tests design #22778

merged 18 commits into from
Apr 28, 2022

Conversation

bhirsz
Copy link
Contributor

@bhirsz bhirsz commented Apr 6, 2022

Migrate GCS system tests/example dags to be compliant with new design (AIP-47).

Covers
airflow/providers/google/cloud/example_dags/example_gcs.py
airflow/providers/google/cloud/example_dags/example_gcs_timespan_file_transform.py
airflow/providers/google/cloud/example_dags/example_gcs_to_gcs.py
airflow/providers/google/cloud/example_dags/example_gcs_to_local.py
airflow/providers/google/cloud/example_dags/example_local_to_gcs.py
from #22447

@boring-cyborg boring-cyborg bot added area:providers kind:documentation provider:google Google (including GCP) related issues labels Apr 6, 2022
@bhirsz
Copy link
Contributor Author

bhirsz commented Apr 6, 2022

The PR is bigger than I want but it's mostly because there are several setup fixtures in old design (in pytest classes) that can now be covered by operators - thus the structure changed and multiple examples had to be done in one go. Everything relates to gcs though.

@Bowrna
Copy link
Contributor

Bowrna commented Apr 10, 2022

@bhirsz
I tried to write a similar system tests design for Elasticsearch and I was running into issues. (#22811)
Can you help me by telling me if you faced similar issues like the one in the comments in this PR?
It would be helpful if you could check this PR and point out mistakes if there is any in it.

@bhirsz
Copy link
Contributor Author

bhirsz commented Apr 11, 2022

I tried to write a similar system tests design for Elasticsearch and I was running into issues. (#22811)
Can you help me by telling me if you faced similar issues like the one in the comments in #22811 (comment) PR?
It would be helpful if you could check this PR and point out mistakes if there is any in it.

I've commented in your PR. But I've also encountered this problem whenever I run the same DAG_ID twice in the same environment (resetting env or using unique name between runs helps). I will investigate why it happens.

Bartlomiej Hirsz added 2 commits April 12, 2022 06:10
Migrate gcs to new system tests design
e48aae7 
Change-Id: I2aeb518d236296bcdbd489850ed1813cefc0ce3f
Remove unnecessary chain from DAG dependencies
82a00c3 
Change-Id: I2558eab8a98706e693c0cc9bed8bc33354e6a23f
@bhirsz bhirsz mentioned this pull request Apr 12, 2022
Merged
Bartlomiej Hirsz added 2 commits April 12, 2022 08:53
Fix issue with running system tests with not unique DAG ID
4c08fa6 
Change-Id: I4d7d88e13de00b25cbce66b0319870cae1be249b
Migrate GCS sensor examples
5bb09f6 
Change-Id: Iec9be2369a93f2b9bab62f6e5eef298492852c90
@bhirsz
Copy link
Contributor Author

bhirsz commented Apr 13, 2022

I tried to write a similar system tests design for Elasticsearch and I was running into issues. (#22811)
Can you help me by telling me if you faced similar issues like the one in the comments in #22811 (comment) PR?
It would be helpful if you could check this PR and point out mistakes if there is any in it.

I've commented in your PR. But I've also encountered this problem whenever I run the same DAG_ID twice in the same environment (resetting env or using unique name between runs helps). I will investigate why it happens.

Resolved by changing State.NONE (which works only for Tasks) to State.Queued (default state for dag)

bhirsz and others added 7 commits April 13, 2022 07:41
@bhirsz
Merge branch 'main' into gcs_system_tests
a1a685c
Fix system tests failing on importable checks
3bffe04 
Change-Id: I8af21344a836c232ff505c1442b960253eec31b7
Merge branch 'gcs_system_tests' of https://github.com/lwyszomi/airflow …
6492a1e 
…into gcs_system_tests

Change-Id: Ie7a572289872d10c649d8697a5819ddcac9d9e99
@potiuk
Migrate gcs to new system tests design
16edd36 
Change-Id: I2aeb518d236296bcdbd489850ed1813cefc0ce3f
@potiuk
Remove unnecessary chain from DAG dependencies
838f865 
Change-Id: I2558eab8a98706e693c0cc9bed8bc33354e6a23f
@potiuk
Migrate GCS sensor examples
93ea975 
Change-Id: Iec9be2369a93f2b9bab62f6e5eef298492852c90
@potiuk
Fix system tests failing on importable checks
6e8baa6 
Change-Id: I8af21344a836c232ff505c1442b960253eec31b7
@potiuk
Copy link
Member

potiuk commented Apr 25, 2022

Rebased it - to test it with the new CI based on Breeze mostly.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 25, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Apr 25, 2022

Stuff to fix :)

bhirsz and others added 6 commits April 27, 2022 09:59
@bhirsz
Merge branch 'main' into gcs_system_tests
7feec73
Merge branch 'gcs_system_tests' of https://github.com/lwyszomi/airflow …
c3ffea5 
…into gcs_system_tests

Change-Id: I3b7f9c184a54a52a39ead74345990eccdc3571b1
@bhirsz
Merge branch 'main' into gcs_system_tests
eb6794c
Update docs path to GCS Sensor examples
eac4727 
Change-Id: Ia75d3d3df42cd708425eb8bbbf3e3978907b6a71
Merge branch 'gcs_system_tests' of https://github.com/lwyszomi/airflow …
247d4df 
…into gcs_system_tests

Change-Id: I63b511054ea694c8f8548fc68e21fd77ee2f88ad
Remove migrated system tests
5c88e1c 
Change-Id: I349a3f0555c04c22d5b095a790ec3be43c343ca9
@bhirsz
Copy link
Contributor Author

bhirsz commented Apr 28, 2022

@potiuk All green :)

@potiuk potiuk merged commit 8a7b61a into apache:main Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joppevos joppevos joppevos left review comments

@potiuk potiuk potiuk approved these changes

@turbaszek turbaszek Awaiting requested review from turbaszek

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees
No one assigned
Labels
area:providers full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bhirsz @Bowrna @potiuk @joppevos