Skip to content

Conversation

@kumargauravin
Copy link

Initial version of code for review.
Needs to be tested with real account which has permissions which I do not have at this time.
Some ToDo left added as comments.

@boring-cyborg
Copy link

boring-cyborg bot commented Aug 10, 2020

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://apache-airflow-slack.herokuapp.com/

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I see a lot of breaking changes commits per version of these libraries.
Further, the real thing to look for a library would be if it handles the split functionality due to API limit. Which I did not found here. But will check more. For our purpose and full control though, I think this will provide more control. Let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mik-laj @turbaszek what do you think?

Copy link
Contributor

@ephraimbuddy ephraimbuddy Aug 10, 2020

Choose a reason for hiding this comment

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

@kumargauravin , I actually don't think it's ok to provide access key in the way you're currently doing it. We have connection table to manage authentication information. You can take inspiration from the other hooks in the Azure package. That's what I think. :\

Copy link
Author

Choose a reason for hiding this comment

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

OK. That is a nice idea. Can you refer to anyone and I will try to work in same way. Also for pylint, I may need some help setting up on that breeze setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

In the case of Stackdriver, we used configurations via airflow.cfg. It is easier to use, but it is not a generally accepted rule.

@ephraimbuddy
Copy link
Contributor

You need to install pre-commit in your development environment https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks

@kumargauravin
Copy link
Author

kumargauravin commented Aug 10, 2020 via email

@turbaszek turbaszek requested a review from feluelle August 12, 2020 06:38
@feluelle feluelle changed the title Azure Log Aanalytics Workspace Task Logs Ingestion Azure Log Analytics Workspace Task Logs Ingestion Aug 13, 2020
@kumargauravin
Copy link
Author

@feluelle @ephraimbuddy Request to check once and suggest. I want to complete once cycle of build tests and then continue to work.

#
"""
This module contains integration with Azure LAWS.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +102 to +104
uri = (
'https://' + self.account_id + '.ods.opinsights.azure.com' + resource + '?api-version=2016-04-01'
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uri = (
'https://' + self.account_id + '.ods.opinsights.azure.com' + resource + '?api-version=2016-04-01'
)
uri = f'https://{self.account_id}.ods.opinsights.azure.com{resource}?api-version=2016-04-01'

Comment on lines +114 to +115
response = requests.post(uri, data=body, headers=headers, verify=ssl_verify)
response.raise_for_status()
Copy link
Member

Choose a reason for hiding this comment

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

I think thats what a basic HttpHook also does? WDYT of inheriting from HttpHook instead of BaseHook? There are a lot of similiarities.

Copy link
Author

Choose a reason for hiding this comment

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

OK will try that out. Thanks for your comments.

Copy link
Member

Choose a reason for hiding this comment

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

Did that work? Or haven't you tried that yet?

@stale
Copy link

stale bot commented Oct 12, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 12, 2020
@feluelle
Copy link
Member

Hey @kumargauravin how is this going? Do you need any help?

The tests are failing because you must add tests for the provider class you added. The config test is also failing. Can you fix that?

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Oct 22, 2020
@kumargauravin
Copy link
Author

kumargauravin commented Oct 22, 2020 via email

@feluelle
Copy link
Member

feluelle commented Oct 28, 2020

For testing the task handler you can check out test_es_task_handler.py for example. It contains unittest by simply mocking elasticsearch. You would have to mock azlaws. You can do this by simply using unittest.mock.

@kumargauravin
Copy link
Author

kumargauravin commented Oct 29, 2020 via email

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 25, 2020
@github-actions github-actions bot closed this Mar 15, 2021
@malthe
Copy link
Contributor

malthe commented Dec 16, 2021

@kumargauravin I am interested in this functionality. Does the code basically work and what's needed from here is to get the pull request into good merge shape (i.e., sort out whatever issues have been mentioned).

@kumargauravin
Copy link
Author

kumargauravin commented Dec 16, 2021 via email

@kumargauravin
Copy link
Author

kumargauravin commented Dec 16, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logging provider:microsoft-azure Azure-related issues stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants