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

Add Open Search Provider #34705

Merged
merged 50 commits into from
Oct 15, 2023
Merged

Add Open Search Provider #34705

merged 50 commits into from
Oct 15, 2023

Conversation

cjames23
Copy link
Contributor

@cjames23 cjames23 commented Oct 2, 2023

This provider would be a pre-requisite for adding functionality to the Amazon provider for the AWS managed service for OpenSearch. This was discussed in a different PR which will be blocked by this PR now. both PRs would be pre-work for completing #33619 and adding Open Search log integration.

Open questions to more experienced contributors:

  1. For system test DAG to succeed would an OpenSearch domain need to be setup and if so what is the approach for standing up such infrastructure for integration tests?
  2. Any advice on mypy for some of the operator parameters? The goal is to allow users to either pass in the arguments directly or by passing in a class in the case of Search which works but in the case of adding a document the end user of the Operator needs to subclass the Document class. I am not sure what the best approach is for the typing for that parameter.

Email sent for Lazy Consensus to the dev-list.


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

airflow/providers/opensearch/provider.yaml Outdated Show resolved Hide resolved
generated/provider_dependencies.json Outdated Show resolved Hide resolved
@vincbeck
Copy link
Contributor

vincbeck commented Oct 3, 2023

For system test DAG to succeed would an OpenSearch domain need to be setup and if so what is the approach for standing up such infrastructure for integration tests?

System tests are not triggered as part of the CI. They are just used as example and also as system test/integration test if anybody wants to execute it (and as such, needs to setup the infra). The approach taken by the community is your write system tests in the Airflow repository but you execute them on your own with your infra set up. As an example, in the Amazon provider package, all system tests rely on AWS services (thus AWS account). These system tests are not triggered as part of Airflow CI but AWS team trigger frequently and results are accessible through a dashboard.

@cjames23
Copy link
Contributor Author

@eladkal any advice on the failure that is happening with the CI image build step and the static checks? I ran breeze static checks against all files locally and do not receive the same failure.

@cjames23
Copy link
Contributor Author

Yeah, I saw that, but the only changes in the Amazon package were some whitespace and punctuation in a docstring. Better done separate, but I didn't think it was worth blocking over it.

Those changes were because pre-commit checks failed locally for those things after a merge pull.

@ferruzzi
Copy link
Contributor

ferruzzi commented Oct 13, 2023

What an odd static check to fail, I can't see how it could be related. When the workflow ends I'll bump it and see if it was just flake. It may also be broken in main, it happens some times.

@cjames23
Copy link
Contributor Author

If the checks fail after this merge from main do you want me to include any fixes here for the breeze mypy issue? I think it just needs to be changed from ignore[misc] to ignore[attr-defined]. Not sure about the unit tests that are failing though from core.

@eladkal
Copy link
Contributor

eladkal commented Oct 14, 2023

If the checks fail after this merge from main do you want me to include any fixes here for the breeze mypy issue? I think it just needs to be changed from ignore[misc] to ignore[attr-defined]. Not sure about the unit tests that are failing though from core.

We have identified the issue
#34941 should fix it

@potiuk
Copy link
Member

potiuk commented Oct 14, 2023

Fixed #34941 and few other issues that caused broken main. Rebasing/resolving conflicts (generally regenerating breeze images with. breeze setup regenerate-images should fix it.

@eladkal eladkal merged commit 94f1441 into apache:main Oct 15, 2023
66 checks passed
@eladkal
Copy link
Contributor

eladkal commented Oct 15, 2023

🎉 🎉 🎉 🎉 🎉 🎉

@ferruzzi
Copy link
Contributor

Nice, congrats!

@pateash
Copy link
Contributor

pateash commented Oct 25, 2023

Hi @cjames23,
Just a quick question regarding this one.
as we know OpenSearch is just a fork of Elasticsearch which happened couple of years ago,
Can we not use Elastic search operators for connecting to OpenSearch,
and if these hooks and operators are very similar to elastic search operators.

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 27, 2023
potiuk pushed a commit that referenced this pull request Oct 29, 2023
Add Open Search Provider
---------

Co-authored-by: Hussein Awala <hussein@awala.fr>
Co-authored-by: D. Ferruzzi <ferruzzi@amazon.com>
(cherry picked from commit 94f1441)
@potiuk potiuk added this to the Airflow 2.7.3 milestone Oct 29, 2023
@potiuk potiuk added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:providers area:system-tests changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) kind:documentation type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants