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.

Sign up for GitHub

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

Jump to bottom

CI: Geo-Spatial Plugin integration test #3244

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

CI: Geo-Spatial Plugin integration test #3244

wants to merge 2 commits into from
+263 −82

Conversation

andy-k-improving
Copy link
Contributor

@andy-k-improving andy-k-improving commented Jan 13, 2025 •
edited
Loading

Description

This is a PR which aim to introduce a new IT test to assert and make sure the existing project setup and logic will work when Geo-Spatial installed within the same cluster.

High-level code changes include:

  • New build target integTestWithGeo to demonstrate the compatibility of Geo-Spatial and SQL plugin
  • New CI build definition integ-tests-with-geo.yml to run IT as the separate GitHub job.
  • Refactor existing build.gradle to promote code reuse between integTestWithGeo and integTestWithSecurity build target
  • Remove redundant new Callable<RegularFile>() wrapper

Test-plan:

  • Security Plugin IT should continue to pass
  • GeoSpatial IT passed
image

Related Issues

Resolves: #3037

Check List

  • New functionality includes testing.
  • [] New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@andy-k-improving andy-k-improving marked this pull request as ready for review January 13, 2025 21:05
@andy-k-improving andy-k-improving mentioned this pull request Jan 13, 2025
Open
7 tasks
@LantaoJin LantaoJin added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Jan 14, 2025
@LantaoJin
Copy link
Member

Could you update the code base and re-run the CI workflows again?

YANG-DB
YANG-DB previously approved these changes Jan 14, 2025
View reviewed changes
@andy-k-improving andy-k-improving force-pushed the ft-ak-geo-spatial-integration branch from 7037704 to d2d4c84 Compare January 14, 2025 18:16
@andy-k-improving
Copy link
Contributor Author

Checked the log from CI build, it's blocked by #3235.

YANG-DB
YANG-DB previously approved these changes Jan 15, 2025
View reviewed changes
@andy-k-improving andy-k-improving force-pushed the ft-ak-geo-spatial-integration branch from b288a80 to efe579f Compare January 15, 2025 18:07
@andy-k-improving
Refactor Gradle file for GeoSpatial build target
cf58dd4 
Signed-off-by: Andy Kwok <[email protected]>

Remove redundent wrapper

Signed-off-by: Andy Kwok <[email protected]>

Github CI

Signed-off-by: Andy Kwok <[email protected]>

Update spotless

Signed-off-by: Andy Kwok <[email protected]>

Update gradle def

Signed-off-by: Andy Kwok <[email protected]>

Exclude geo IT from main integ-test

Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving
Update regular
cf83890 
Signed-off-by: Andy Kwok <[email protected]>
@andy-k-improving andy-k-improving force-pushed the ft-ak-geo-spatial-integration branch from efe579f to cf83890 Compare January 15, 2025 18:24
@andy-k-improving
Copy link
Contributor Author

@LantaoJin All tests are passing except bwc, can you have another look?
Thanks!

penghuo
penghuo reviewed Jan 15, 2025
View reviewed changes
integ-test/build.gradle
@@ -261,6 +297,11 @@ testClusters {
plugin(getJobSchedulerPlugin())
plugin ":opensearch-sql-plugin"
}
integTestWithGeo {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason for separating integTestWithGeo from integTest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

integTestWithGeo got separated as it requires an additional plugin Geo-spatial, by having a separate build target with external deps will help troubleshot in the case of build failure.
Similar to the existing build target: integTestWithSecurity.

Copy link
Collaborator

@penghuo penghuo Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

by having a separate build target with external deps will help troubleshot in the case of build failure.

could u elberate more on this. i think integTest already have clear hint on failed test cases.

If Geo command/function is native PPL feature, and OpenSearch release by defalut included geo-spatial plugin It not necessary to sepreate it from integTest. The geo-spatial use case is more similar to job-scheduler, instead of security plugin.

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

@penghuo penghuo penghuo left review comments

@YANG-DB YANG-DB YANG-DB left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE]Add iplocation function to PPL for IP address geolocation
4 participants
@andy-k-improving @LantaoJin @penghuo @YANG-DB