You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2023/03/09 19:37:58 UTC
[GitHub] [airflow] vincbeck opened a new pull request, #30003: Move and convert all AWS example dags to system tests
vincbeck opened a new pull request, #30003:
URL: https://github.com/apache/airflow/pull/30003
Per discussion in #22438, we decided to move all example dags in folder `airflow/providers/amazon/aws/example_dags/` to the system test folder `tests/system/providers/amazon/aws/`.
Sorry for the length of this PR but the essential modifications are:
- Moving files from `airflow/providers/amazon/aws/example_dags/` to `tests/system/providers/amazon/aws/`
- Convertir the example DAGs to system test format
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #30003: Move and convert all AWS example dags to system tests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #30003:
URL: https://github.com/apache/airflow/pull/30003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1470603601
The current state where we have examples speared throughout the project is not intuitive. Which is why I raised this as an issue https://github.com/apache/airflow/issues/22438#issuecomment-1374516903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on a diff in pull request #30003: Move and convert all AWS example dags to system tests
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #30003:
URL: https://github.com/apache/airflow/pull/30003#discussion_r1131663587
##########
docs/exts/docs_build/lint_checks.py:
##########
@@ -270,38 +270,6 @@ def find_example_dags(provider_dir):
yield from glob(f"{ROOT_PROJECT_DIR}/tests/system/{system_tests_dir}/*/", recursive=True)
-def check_example_dags_in_provider_tocs() -> list[DocBuildError]:
Review Comment:
I removed this test because, I think, it no longer make sense. Please let me know if any concerns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1476901560
> > > I'm with Elad on this. Having just a couple of examples left in examples folder is rather confusing for user's pov. Providing all examples / system tests in one place makes it easier to reference other tests as examples when writing a DAG.
> >
> >
> > > Are people going to be confused why we don't run many of these tests? So far we only have migrated examples-->tests that we are committed to running in our test setup. We won't be running many of the tests in this PR since we don't have the external accounts and resources required to run them (many of the transfers are a good example of this).
> >
> >
> > Yes it's a valid worry. But maybe we could make it clearer by slight modification of the pytest code so that it will raise Skip exception for those tests unless we setup some variable (RUN_EXPERIMENTAL_TESTS and providing skip reason). This will be visible in the code, as well as in your dashboard you could run whole folder, and possibly even report number of skipped tests. I think this both removes the confusion and makes it clear which tests are "tested".
>
> My concern with this approach is we would end up having 2 categories of system tests: experimental and non experimental ones. IMHO, we should not have such categorization, users are welcome to run them if they want regardless of whether AWS run them as part of their health dashboard. Moreover, forcing users to have the flag `RUN_EXPERIMENTAL_TESTS` on to run these tests would make it more difficult for users to run them (or would incentivize them to NOT run them).
>
> We made some recent changes in the health dashboard so that system tests which are not run are listed in the dashboard but with no information (since they are not run). [See dashboard](https://aws-mwaa.github.io/open-source/system-tests/dashboard.html). If we go with this change, then all example DAGs would show up in the dashboard and styled like `example_dms` or `example_emr_notebook_execution`. To me this makes sense. We can go further and move system tests which are not run to the bottom of the dashboard.
>
> WDYT?
Great discussion everyone! It appears I'm out voted and most folks would like to see them moved. I'm happy to commit to that approach :)
And I think this is a fair counter point from Vincent, maybe we shouldn't over complicate things. We can try first with the more simplified approach you have now, this is not a one-way door, if it does cause confusion with users we can make some adjustments/optimizations later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1470818675
> Yes it's a valid worry. But maybe we could make it clearer by slight modification of the pytest code so that it will raise Skip exception for those tests unless we setup some variable (RUN_EXPERIMENTAL_TESTS and providing skip reason). This will be visible in the code, as well as in your dashboard you could run whole folder, and possibly even report number of skipped tests. I think this both removes the confusion and makes it clear which tests are "tested".
+1
Ny initial tought was lets make all examples in the system tests structure but touse we know that are not working/have issues we mark them as quarantined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] shubham22 commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "shubham22 (via GitHub)" <gi...@apache.org>.
shubham22 commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1470643869
I'm with Elad on this. Having just a couple of examples left in examples folder is rather confusing for user's pov. Providing all examples / system tests in one place makes it easier to reference other tests as examples when writing a DAG.
> Are people going to be confused why we don't run many of these tests?
I think this is why we added the reasoning on System test dashboard. We can add it in System test rst as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1478467857
Yep. Let's merge it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a diff in pull request #30003: Move and convert all AWS example dags to system tests
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #30003:
URL: https://github.com/apache/airflow/pull/30003#discussion_r1134484461
##########
tests/system/providers/amazon/aws/example_eks_templated.py:
##########
@@ -128,14 +134,28 @@
target_state=ClusterStates.NONEXISTENT,
)
- (
- create_cluster
- >> await_create_cluster
- >> create_nodegroup
- >> await_create_nodegroup
- >> start_pod
- >> delete_nodegroup
- >> await_delete_nodegroup
- >> delete_cluster
- >> await_delete_cluster
+ chain(
+ # TEST SETUP
+ test_context,
+ # TEST BODY
+ create_cluster,
+ await_create_cluster,
+ create_nodegroup,
+ await_create_nodegroup,
+ start_pod,
+ delete_nodegroup,
Review Comment:
```suggestion
start_pod,
# TEST TEARDOWN
delete_nodegroup,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vincbeck commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1476841581
> > I'm with Elad on this. Having just a couple of examples left in examples folder is rather confusing for user's pov. Providing all examples / system tests in one place makes it easier to reference other tests as examples when writing a DAG.
>
> > Are people going to be confused why we don't run many of these tests? So far we only have migrated examples-->tests that we are committed to running in our test setup. We won't be running many of the tests in this PR since we don't have the external accounts and resources required to run them (many of the transfers are a good example of this).
>
> Yes it's a valid worry. But maybe we could make it clearer by slight modification of the pytest code so that it will raise Skip exception for those tests unless we setup some variable (RUN_EXPERIMENTAL_TESTS and providing skip reason). This will be visible in the code, as well as in your dashboard you could run whole folder, and possibly even report number of skipped tests. I think this both removes the confusion and makes it clear which tests are "tested".
My concern with this approach is we would end up having 2 categories of system tests: experimental and non experimental ones. IMHO, we should not have such categorization, users are welcome to run them if they want regardless of whether AWS run them as part of their health dashboard. Moreover, forcing users to have the flag `RUN_EXPERIMENTAL_TESTS` on to run these tests would make it more difficult for users to run them (or would incentivize them to NOT run them).
We made some recent changes in the health dashboard so that system tests which are not run are listed in the dashboard but with no information (since they are not run). [See dashboard](https://aws-mwaa.github.io/open-source/system-tests/dashboard.html). If we go with this change, then all example DAGs would show up in the dashboard and styled like `example_dms` or `example_emr_notebook_execution`. To me this makes sense. We can go further and move system tests which are not run to the bottom of the dashboard.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1470588425
I agree with Niko, I find it confusing to mix together examples that are actual test being run and dags that are just sitting there without being tested...
What's the problem with having the tests that we run in `tests/system`, and the ones that we don't in `example_dags` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #30003: Move and convert all AWS example dags to system tests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30003:
URL: https://github.com/apache/airflow/pull/30003#issuecomment-1470810141
> I'm with Elad on this. Having just a couple of examples left in examples folder is rather confusing for user's pov. Providing all examples / system tests in one place makes it easier to reference other tests as examples when writing a DAG.
> Are people going to be confused why we don't run many of these tests? So far we only have migrated examples-->tests that we are committed to running in our test setup. We won't be running many of the tests in this PR since we don't have the external accounts and resources required to run them (many of the transfers are a good example of this).
Yes it's a valid worry. But maybe we could make it clearer by slight modification of the pytest code so that it will raise Skip exception for those tests unless we setup some variable (RUN_EXPERIMENTAL_TESTS and providing skip reason). This will be visible in the code, as well as in your dashboard you could run whole folder, and possibly even report number of skipped tests. I think this both removes the confusion and makes it clear which tests are "tested".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org