-
Notifications
You must be signed in to change notification settings - Fork 959
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
fix: Change databuilder search data extractors to publish name
in user document.
#2274
Conversation
Congratulations on your first Pull Request and welcome to Amundsen community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/amundsen-io/amundsen/blob/main/CONTRIBUTING.md) |
e4b986b
to
e7ccc06
Compare
…ocument. Signed-off-by: Jackson Goerner <[email protected]>
e7ccc06
to
b134aac
Compare
@glipR thanks for your contribution! I triggered the CI tests and see a couple minor test failures. if you can resolve, I'd be happy to approve |
Signed-off-by: Jackson Goerner <[email protected]>
b188e3a
to
8de4326
Compare
Thanks @kristenarmes ! I've edited the two failing tests to reflect the new return values / expected arguments. Apologies I'm having trouble triggering the tests locally due to some version conflicts so may need a retrigger to confirm that that has fixed everything 🙏 |
Signed-off-by: Jackson Goerner <[email protected]>
Didn't notice they'd been rerun, apologies for the delay :) Looks like everything is passing now, thanks again for triggering these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I was about to merge, but realized did you want to bump the databuilder version? here's the version spot |
Signed-off-by: Jackson Goerner <[email protected]>
Sure thing - have bumped the patch version. |
Awesome work, congrats on your first merged pull request! |
Description
Changes the search data databuilder extractors to extract a
name
field, for publishing to elasticsearch.Currently, the user document schema in elasticsearch expects a
name
keyword, which is the primary field for searching users. (Expected from Databuilder publish, and expected from search service)Motivation and Context
The change is required because currently the elasticsearch query does not use the
name
field to create better matches for search results, and instead is likely just using first/last and key of the user. This is fine when searching for just first or last, but when searching both the search results currently aren't great for this reason.This is actually fixed in the example query (PR) for posting from neo4j to elasticsearch, but this query is never used.
How Has This Been Tested?
This has only been tested with the neo4j_search_data_extractor, and not the other extractors. Testing was done by:
localhost:9200
and inspecting document valuesDocumentation
No change in documentation
CheckList
Believe this fix has no need for:
Updates Documentation and DocstringsAdds testsAdds instrumentation (logs, or UI events)