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 phone number analysis plugin #609

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rursprung
Copy link

@rursprung rursprung commented Oct 11, 2024

Description

this is part of opensearch-project/OpenSearch#11326. the actual implementation was done opensearch-project/OpenSearch#15915. see the commit message on the PR for further details.

the new test group analysis has been added so that it can later be extended with all other optional language analyzers (which are currently also not covered).

note that the CI currently needs to fetch the image from opensearchstaging as 2.18.0 hasn't been released yet. the hub and ref config can be removed once 2.18.0 has been released.

TODO: the plugin is not yet released => installation fails. is there a snapshot somewhere which can be used instead?

Issues Resolved

n/a

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

github-actions bot commented Oct 11, 2024

Changes Analysis

Commit SHA: 6f3e2d6
Comparing To SHA: 49b7c68

API Changes

Summary

└─┬Components
  ├──[➕] schemas (36974:7)
  └─┬_common.analysis:Analyzer
    ├──[➕] oneOf (36198:7)
    ├─┬ONEOF
    │ └──[🔀] $ref (36974:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (35966:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36870:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (37224:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (37140:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36039:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36401:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (37280:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36117:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36604:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (37129:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (35894:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (37169:13)❌ 
    ├─┬ONEOF
    │ └──[🔀] $ref (36458:13)❌ 
    └─┬ONEOF
      └──[🔀] $ref (36777:13)❌ 

Document Element Total Changes Breaking Changes
components 17 15
  • BREAKING Changes: 15 out of 17
  • Modifications: 15
  • Additions: 2
  • Breaking Modifications: 15

Report

The full API changes report is available at: https:/opensearch-project/opensearch-api-specification/actions/runs/11296186194/artifacts/2046519496

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 29 29 0

@dblock
Copy link
Member

dblock commented Oct 11, 2024

Looks great! Needs CHANGELOG, and a passing build (plugin installation likely needs work, ./bin/opensearch-plugin install -b analysis-phonenumber is failing).

@rursprung rursprung force-pushed the add-spec-for-opensearch-phone-plugin branch from 45270e5 to 05fcc57 Compare October 11, 2024 16:46
this is part of opensearch-project/OpenSearch#11326. the actual
implementation was done opensearch-project/OpenSearch#15915. see the
commit message on the PR for further details.

the new test group `analysis` has been added so that it can later be
extended with all other optional language analyzers (which are currently
also not covered).

note that the CI currently needs to fetch the image from
`opensearchstaging` as 2.18.0 hasn't been released yet. the `hub` and
`ref` config can be removed once 2.18.0 has been released.

Signed-off-by: Ralph Ursprung <[email protected]>
@rursprung rursprung force-pushed the add-spec-for-opensearch-phone-plugin branch from 05fcc57 to 6f3e2d6 Compare October 11, 2024 16:48
@rursprung
Copy link
Author

i've updated the path & added the changelog. i'm keeping this as a draft until this TODO is resolved:

TODO: the plugin is not yet released => installation fails. is there a snapshot somewhere which can be used instead?

acc. to @dblock the plugin should be part of the docker image (but doesn't seem to be as it tries to fetch it from the web)

@dblock
Copy link
Member

dblock commented Oct 11, 2024

i've updated the path & added the changelog. i'm keeping this as a draft until this TODO is resolved:

TODO: the plugin is not yet released => installation fails. is there a snapshot somewhere which can be used instead?

acc. to @dblock the plugin should be part of the docker image (but doesn't seem to be as it tries to fetch it from the web)

I can confirm that anlaysis-phone is not in the docker image.

[opensearch@6b69e2640682 ~]$ ls -R -la | grep analysis-
-rw-r--r-- 1 opensearch opensearch  1723718 Sep 12 10:57 lucene-analysis-common-9.11.1.jar
drwxr-xr-x  2 opensearch opensearch 4096 Sep 12 10:57 analysis-common
./modules/analysis-common:
-rw-r--r--  1 opensearch opensearch 199046 Sep 12 10:57 analysis-common-2.17.0.jar
-rw-r--r--  1 opensearch opensearch  34776 Sep 12 10:57 asm-analysis-9.7.jar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants