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

Adding MSGraphOperator in Microsoft Azure provider #38111

Merged
merged 172 commits into from
Apr 14, 2024

Conversation

dabla
Copy link
Contributor

@dabla dabla commented Mar 13, 2024

As already discussed before I did a proposition to add the MS Graph Operator. After discussion on the airflow dev list we came to an agreement that the operator could be added as part of the Microsoft Azure provider. This is my initial commit so any suggestions are welcome, I still need to add examples and propably need to update some docstrings but this PR can already give an idea of what will be added. I already created a PR so I can see what comes out of the Airflow QA.

In the meantime, our article on how we use the operator at Infrabel has been published on the Apache Airflow medium:

https://medium.com/apache-airflow/optimizing-integration-with-the-ms-graph-api-and-power-bi-with-the-msgraphasyncoperator-in-airflow-d1071f7c1b62


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

davidblain-infrabel and others added 27 commits March 13, 2024 17:19
…w connection and request adapter + make multiple patches into one context manager Python 3.8 compatible
@potiuk
Copy link
Member

potiuk commented Apr 12, 2024

You should remove your fix and rebase - in change you applied you removed the markers on tests but it needs pytestmark on the module level instead,

@dabla
Copy link
Contributor Author

dabla commented Apr 12, 2024

You should remove your fix and rebase - in change you applied you removed the markers on tests but it needs pytestmark on the module level instead,

So I need to do like this then? Not completely understanding what you meant (I reverted my fix containing the db markers on the test-methods):

@pytest.mark.db_test
class TestMSGraphAsyncOperator(Base):
    def test_execute(self):
        ...

@potiuk
Copy link
Member

potiuk commented Apr 12, 2024

See how it it's done in main (and you rebased to it) pytestmark is assigned at the whole module level- so all those tests are nowdb_tests` - your previous attempt removed all of the existing markers makiung them more susceptible to pytest-xdist/async bug we seem to be hitting

@dabla
Copy link
Contributor Author

dabla commented Apr 12, 2024

See how it it's done in main (and you rebased to it) pytestmark is assigned at the whole module level- so all those tests are nowdb_tests` - your previous attempt removed all of the existing markers makiung them more susceptible to pytest-xdist/async bug we seem to be hitting

Ok then I don't understand why sometimes it's still failing, it's not alway the case as on some python version it succeeds but on others not

@potiuk
Copy link
Member

potiuk commented Apr 12, 2024

Those are new tests you addded that are now failing, It's a different error you get:

You also need to mark the test as db_test per instruction:

FAILED tests/providers/microsoft/azure/operators/test_msgraph.py::TestMSGraphAsyncOperator::test_execute - airflow.exceptions.AirflowInternalRuntimeError: Your test accessed the DB but _AIRFLOW_SKIP_DB_TESTS is set.
Either make sure your test does not use database or mark the test with @pytest.mark.db_test
See https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#best-practices-for-db-tests on how to deal with it and consult examples.

Now why they are not failing in 3.10+ I am not sure - maybe your tests follow different path on those python versions without trying to access DB ?

@dabla
Copy link
Contributor Author

dabla commented Apr 12, 2024

Those are new tests you addded that are now failing, It's a different error you get:

You also need to mark the test as db_test per instruction:

FAILED tests/providers/microsoft/azure/operators/test_msgraph.py::TestMSGraphAsyncOperator::test_execute - airflow.exceptions.AirflowInternalRuntimeError: Your test accessed the DB but _AIRFLOW_SKIP_DB_TESTS is set.
Either make sure your test does not use database or mark the test with @pytest.mark.db_test
See https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#best-practices-for-db-tests on how to deal with it and consult examples.

Now why they are not failing in 3.10+ I am not sure - maybe your tests follow different path on those python versions without trying to access DB ?

Hmm those tests are not new, the only new one is the TestMSGraphSensor and that one doesn't fail apparently. Weird...

@potiuk
Copy link
Member

potiuk commented Apr 12, 2024

some main errrs fixed already - you will need to rebase

@potiuk
Copy link
Member

potiuk commented Apr 14, 2024

The docker example error workarounded in main. Merging

@potiuk potiuk merged commit 1c9a660 into apache:main Apr 14, 2024
91 of 92 checks passed
@dabla
Copy link
Contributor Author

dabla commented Apr 15, 2024

Thank you @potiuk :)

@utkarsharma2 utkarsharma2 added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:microsoft-azure Azure-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants