Skip to content

Add AWS EMR System tests#8618

Merged
feluelle merged 8 commits intoapache:masterfrom
xinbinhuang:aws_system_tests
May 13, 2020
Merged

Add AWS EMR System tests#8618
feluelle merged 8 commits intoapache:masterfrom
xinbinhuang:aws_system_tests

Conversation

@xinbinhuang
Copy link
Contributor

@xinbinhuang xinbinhuang commented Apr 28, 2020

Add system tests for two EMR related example DAGs.

The credentials are provided from ~/.aws/ following as in boto3 configuration. I feel like I should add some docs somewhere to indicate how to provide credentials for the system test, but not sure where... Please let me know if you would like me to add the documentation somewhere.


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Apr 28, 2020
@mik-laj
Copy link
Member

mik-laj commented Apr 28, 2020

I feel like I should add some docs somewhere to indicate how to provide credentials for the system test, but not sure where...

We have documentation about credentials in system tests:
https://github.com/apache/airflow/blob/master/TESTING.rst#forwarding-authentication-from-the-host.
You can still add a small note in the docstring for the file: tests/providers/amazon/aws/operators/test_emr_system.py

The best solution is to add a marker that will skip the tests if the file with credentials is not available. If you try to run tests for GCP and you do not have credentials then you will see the message below:

========================================================================================= short test summary info ==========================================================================================
SKIPPED [1] /Users/kamilbregula/devel/google-airflow/airflow/tests/conftest.py:308: The test requires credential file /files/airflow-breeze-config/keys/gcp_life_sciences.json: <TestCaseFunction test_run_example_dag_function>
============================================================================================ 1 skipped in 1.06s ============================================================================================

@xinbinhuang
Copy link
Contributor Author

I feel like I should add some docs somewhere to indicate how to provide credentials for the system test, but not sure where...

We have documentation about credentials in system tests:
https://github.com/apache/airflow/blob/master/TESTING.rst#forwarding-authentication-from-the-host.
You can still add a small note in the docstring for the file: tests/providers/amazon/aws/operators/test_emr_system.py

The best solution is to add a marker that will skip the tests if the file with credentials is not available. If you try to run tests for GCP and you do not have credentials then you will see the message below:

========================================================================================= short test summary info ==========================================================================================
SKIPPED [1] /Users/kamilbregula/devel/google-airflow/airflow/tests/conftest.py:308: The test requires credential file /files/airflow-breeze-config/keys/gcp_life_sciences.json: <TestCaseFunction test_run_example_dag_function>
============================================================================================ 1 skipped in 1.06s ============================================================================================

@mik-laj Thanks for the suggestion. I took a look at the marker you mention, it needs to copy the credentials in the files/breeze_config/keys folder. For AWS system test, I think we can defer the discussion on sourcing credentials later as right now, we are still trying to add more helpers functions for AWS system test (i.e. #8581). So for this PR, I think it will better to use the default credentials for now, and we can change it after we have standardized the workflow.

What do you think?

@mik-laj
Copy link
Member

mik-laj commented Apr 29, 2020

@xinbinhuang I agree. We don't need it now, but you asked how to make the documentation. The best documentation is an intuitive program that has user-friendly messages

@xinbinhuang
Copy link
Contributor Author

cc @feluelle @mustafagok - Feel free to take a look :)

@feluelle I can rebase on your changes once your PR is merged into master

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

Please add emr.rst docs in the howto section. Besides that LGTM.

And if you PR is merged first I can rebase onto your changes (aws_system_helpers.py) - that's not a problem - you don't have to wait for my PR to be merged :)

@feluelle
Copy link
Member

feluelle commented May 7, 2020

@xinbinhuang you can rebase onto latest master. The amazon_system_helpers.py is there :)

@xinbinhuang
Copy link
Contributor Author

@mustafagok @feluelle Sorry for the late update, I have been swamped by other things in the last two weeks. I update the PR with your suggestions. PTAL :)

@xinbinhuang xinbinhuang requested a review from feluelle May 13, 2020 18:10
@feluelle feluelle merged commit e61b9bb into apache:master May 13, 2020
@xinbinhuang
Copy link
Contributor Author

@feluelle @mustafagok Thanks for reviewing and merging!

@potiuk
Copy link
Member

potiuk commented May 13, 2020

Fantastics! Thanks @feluelle !

@xinbinhuang xinbinhuang mentioned this pull request May 14, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments