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

Update yml test for json ingest processor #25972

Closed
wants to merge 3 commits into from

Conversation

Fahminajjar
Copy link

@Fahminajjar Fahminajjar commented Jul 31, 2017

These tests should pass because JSON can accept one of these values.

This tests should pass because JSON can accept one of these values.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@nik9000 nik9000 requested a review from talevy July 31, 2017 12:43
@nik9000 nik9000 added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >test Issues or PRs that are addressing/adding tests labels Jul 31, 2017
@nik9000 nik9000 changed the title Update 140_json.yml Update yml test for json ingest processor Jul 31, 2017
@talevy
Copy link
Contributor

talevy commented Jul 31, 2017

thanks @Fahminajjar for reporting this. I'll take a look! Originally, the processor was not designed to support this, but I see no reason it shouldn't.

@javanna
Copy link
Member

javanna commented Nov 6, 2017

hi @Fahminajjar could you please sign our CLA? Otherwise we can't get your change in.

@Fahminajjar
Copy link
Author

I signed the CLA.

@javanna
Copy link
Member

javanna commented Nov 7, 2017

thank you! @talevy could you review this please?

@talevy
Copy link
Contributor

talevy commented Nov 9, 2017

thank you for sharing these tests with us! Sorry it took so long to get to.
I have stolen this yaml test into the PR that will allow for these input values.

Here is the PR #27335.

I'll go ahead and close this PR/issue/feature-request once that is merged.

talevy added a commit to talevy/elasticsearch that referenced this pull request Nov 9, 2017
The Json Processor originally only supported parsing field values into Maps even
though the JSON spec specifies that strings, null-values, numbers, booleans, and arrays
are also valid JSON types. This commit enables parsing these values now.

response to elastic#25972.
talevy added a commit that referenced this pull request Nov 13, 2017
The Json Processor originally only supported parsing field values into Maps even
though the JSON spec specifies that strings, null-values, numbers, booleans, and arrays
are also valid JSON types. This commit enables parsing these values now.

response to #25972.
talevy added a commit that referenced this pull request Nov 13, 2017
The Json Processor originally only supported parsing field values into Maps even
though the JSON spec specifies that strings, null-values, numbers, booleans, and arrays
are also valid JSON types. This commit enables parsing these values now.

response to #25972.
@talevy
Copy link
Contributor

talevy commented Nov 13, 2017

Fixed in #27335. Let me know what you think @Fahminajjar! Should be good to go in ES 6.1

@talevy talevy closed this Nov 13, 2017
@Fahminajjar
Copy link
Author

Thank you @talevy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants