Skip to content

[WIP] Adapt ongoing geoip upstream changes.#445

Draft
mashhurs wants to merge 2 commits intoelastic:mainfrom
mashhurs:adapt-upstream-refactor-geoip-changes
Draft

[WIP] Adapt ongoing geoip upstream changes.#445
mashhurs wants to merge 2 commits intoelastic:mainfrom
mashhurs:adapt-upstream-refactor-geoip-changes

Conversation

@mashhurs
Copy link
Copy Markdown
Collaborator

Pulls this elastic/elasticsearch#145011 PR changes and tests

@mergify
Copy link
Copy Markdown

mergify Bot commented Apr 17, 2026

This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • If no backport is necessary, please add the backport-skip label

Copy link
Copy Markdown
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

There's some nuance with how we get the shaded dependencies lined up for this project, the new ingest-ip-location module, and the dependencies of the ip-location module (but maybe not the functionality from that module, since the ingest-ip-location provides an implementation of elasticsearch-ip-location-api for use with its database providers). But Logstash also provides those dependencies, although it may be a different version which is why we need the shading.

I'll need to validate it next week when my head has the room to think it through.

Comment thread build.gradle Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
task shadeElasticsearchIpLocationModule(type: com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, properly renamed them now as my first very minimal change was to check the tests.

Comment thread build.gradle
dependsOn buildElasticsearchLocalDistro

from(buildElasticsearchLocalDistro.module("ingest-geoip").orElse(objects.fileCollection()))
from(buildElasticsearchLocalDistro.module("ip-location").orElse(objects.fileCollection()))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need everything, or can we get only the geoip dependencies?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, as per my analysis (correct me if I am wrong), we need all (ip-location, ip-location-api and ingest-ip-location) need to exist in the JARs but we have to only shade the ip-location since others are references by the logstash-bridge:

  • IpLocationServiceAdapter from ingest-ip-location
  • no-op IpLocationService from ip-location-api

Comment thread build.gradle Outdated
@@ -472,6 +473,11 @@ task importMinimalElasticsearch() {
}

from(shadeElasticsearchIngestGeoIpModule)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
from(shadeElasticsearchIngestGeoIpModule)
from(shadeElasticsearchIpLocationModule)

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Apr 23, 2026

💛 Build succeeded, but was flaky

Failed CI Steps

History

@mashhurs mashhurs linked an issue Apr 23, 2026 that may be closed by this pull request
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.

Apply upstream ES ingest-geoip module refactoring

3 participants