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 reference docs for Highlighter fragmenter #23754

Merged
merged 2 commits into from Apr 17, 2017

Conversation

sudo-suhas
Copy link
Contributor

Highlighting supports an option fragmenter which is supported by the Plain fragmenter but the reference documentation does not mention it.

Reference documentation for the same was added by collating information from the following sources:

Closes #23736

@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?

@sudo-suhas
Copy link
Contributor Author

@javanna Does this documentation look okay?

@javanna
Copy link
Member

javanna commented Mar 27, 2017

hi @sudo-suhas the addition looks good thanks! The json is malformed though, which makes our build fail as we validate json snippets. You can reproduce that by running gradle check from the docs directory in our repo. Would you have time to also add the response and compare the two different options available?

@javanna javanna self-assigned this Mar 27, 2017
@javanna javanna added >docs General docs changes >enhancement labels Mar 27, 2017
@sudo-suhas
Copy link
Contributor Author

Ah.. Should have paid more attention while copying. Also, did not realise docs had a gradle build. Will do the check before committing next time. For giving the response and compare, can you suggest an appropriate example from the examples repo? I have loaded the NYC accident data and NGINX logs previously but I don't think they will be appropriate for this exercise.

@javanna
Copy link
Member

javanna commented Mar 27, 2017

@sudo-suhas start with fixing your query. as for examples, I would look in our docs and take inspiration from there.

@sudo-suhas
Copy link
Contributor Author

@javanna I have pushed a commit with requested changes. However, setup of gradle, running tests, setting up test indices etc was non-trivial. I think it would be good if there was a slightly more detailed guide for contributors. I have created a gist here with CONSOLE script for setting up the test indices for anybody else who might want to use it.

@sudo-suhas
Copy link
Contributor Author

@javanna Are there any changes required for this pull request? I wanted to get 1 right before doing the changes in the other pull request.

@javanna
Copy link
Member

javanna commented Apr 13, 2017

jenkins test this please

@javanna
Copy link
Member

javanna commented Apr 14, 2017

jenkins retest this please

@sudo-suhas
Copy link
Contributor Author

@javanna I know this is probably a low priority. However, please note that I have modified only one file in docs for which I ran the gradle docs:check as suggested. Is there anything I need to do to fix the failing build?

@nik9000
Copy link
Member

nik9000 commented Apr 17, 2017

It looks like @javanna is doing something else. I reviewed and it looks good. I'll test it locally and merge if all looks good.

@nik9000 nik9000 merged commit f97d8bc into elastic:master Apr 17, 2017
nik9000 pushed a commit that referenced this pull request Apr 17, 2017
Explain the fragmenter and add examples.
nik9000 pushed a commit that referenced this pull request Apr 17, 2017
Explain the fragmenter and add examples.
@nik9000
Copy link
Member

nik9000 commented Apr 17, 2017

I've merged to master and cherry-picked to 5.x and 5.4.

@nik9000
Copy link
Member

nik9000 commented Apr 17, 2017

Thanks @sudo-suhas! The docs really did look nice.

@sudo-suhas sudo-suhas deleted the docs_for_highlight_fragmenter branch April 17, 2017 23:16
@sudo-suhas
Copy link
Contributor Author

sudo-suhas commented Apr 18, 2017

@javanna
Copy link
Member

javanna commented Apr 18, 2017

thanks @nik9000 and @sudo-suhas !

@nik9000
Copy link
Member

nik9000 commented Apr 18, 2017

Does it need to be cherry-picked to 5.3 as well?

I was trying not to push anything to 5.3 because we're trying to keep it stable. But I already pushed a bunch of docs to it last night so I'm happy to cherry-pick there.

nik9000 pushed a commit that referenced this pull request Apr 18, 2017
Explain the fragmenter and add examples.
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.

None yet

5 participants