Skip to content

Add geo-grid ingest processor #2844

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

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Add geo-grid ingest processor #2844

merged 2 commits into from
Sep 3, 2024

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Sep 2, 2024

While this does not turn any status to green, this fixes most of the currently reported errors for ingest.put_pipeline requests.

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ingest.delete_geoip_database 🟢 1/1 1/1
ingest.delete_pipeline 🟢 15/15 15/15
ingest.geo_ip_stats 🟢 1/1 1/1
ingest.get_geoip_database 🟢 4/4 4/4
ingest.get_pipeline 🔴 22/22 21/22
ingest.processor_grok 🟢 1/1 1/1
ingest.put_geoip_database 🟢 3/3 3/3
ingest.put_pipeline 🔴 57/59 59/59
ingest.simulate 🔴 6/9 9/9

You can validate these APIs yourself by using the make validate target.

Comment on lines +407 to +408
geojson,
wkt
Copy link
Member Author

Choose a reason for hiding this comment

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

While the documentation says it should be GeoJSON and WKT, the YAML tests are using geojson and wkt. I talked to @craigtaverner who confirms that this processor was specifically designed to be case-insensitive so lowercase is OK.

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

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

Nice, more processors 🙌

ignore_missing?: boolean
/**
* Which format to save the generated polygon in.
* @server_default GeoJSON
Copy link
Member

Choose a reason for hiding this comment

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

Let's use full lowercase here as well to match the enum value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Done in b7eaafb (#2844).

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Following you can find the validation results for the APIs you have changed.

API Status Request Response
ingest.delete_geoip_database 🟢 1/1 1/1
ingest.delete_pipeline 🟢 15/15 15/15
ingest.geo_ip_stats 🟢 1/1 1/1
ingest.get_geoip_database 🟢 4/4 4/4
ingest.get_pipeline 🔴 22/22 21/22
ingest.processor_grok 🟢 1/1 1/1
ingest.put_geoip_database 🟢 3/3 3/3
ingest.put_pipeline 🔴 57/59 59/59
ingest.simulate 🔴 6/9 9/9

You can validate these APIs yourself by using the make validate target.

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@pquentin pquentin merged commit e171d9f into main Sep 3, 2024
6 checks passed
@pquentin pquentin deleted the geo-grid branch September 3, 2024 07:23
github-actions bot pushed a commit that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants