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

[DOCS] Create documentation for fields.yml (#6049) #12505

Merged
merged 4 commits into from
Feb 14, 2020

Conversation

bestpath-gb
Copy link
Contributor

Add a new documentation page for _meta/fields.yml. This mentions what I'd consider most of the commonly used options, with a few examples.

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

@dedemorton
Copy link
Contributor

Thanks for your contribution.

WRT writing style, we prefer to avoid passive voice when possible (active verbs are easier to read). Also, you should add line breaks before your code sections (for example, before line 7). We're migrating to asciidoctor, which is less permissive than asciidoc, so it's best to be on the safe side.

I'd like to offer more editorial feedback, but I can't right now. If this doesn't get merged before I come back to it, I'll submit a follow-up edit. Thanks again for doing this!

Copy link
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

LGTM for the technical content, except for one comment that should be addressed. I'll leave editorial changes and final approval to DeDe :-)

copy_to: full_name
description: >
The surname.
multi_fields: <1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Both surname and forenames here are themselves keyword types, which would make this redundant, right? The description above also describes using this on text fields, not keywords. Perhaps this section should use a different example field that would more naturally be of type text.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bestpath-gb Will you be able to update the example as suggested here? I appreciate the effort that you put into creating this PR and would like to help you get this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Sorry for the delay in getting back to this. I'm hoping to review and update the examples this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a great catch, @faec . I've updated the examples based on the Elasticsearch reference for those mapping parameters.

@dedemorton
Copy link
Contributor

jenkins test this please

@dedemorton
Copy link
Contributor

I'm adding the needs_edit label to this PR so I can come back and edit the content after it's approved and merged.

@dedemorton dedemorton added the needs_edit Indicates that the doc changes need an edit after merging. label Nov 1, 2019
@dedemorton
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

@dedemorton
Copy link
Contributor

Sorry I haven't been able to edit this yet. I think it's best to merge the PR since it's technically correct. I'll follow up with edits in a separate PR. Just launched the doc build. When it passes, I will merge. Thanks again for your contribution. My apologies that it's taken me awhile to get back to this.

Copy link
Contributor

@dedemorton dedemorton left a comment

Choose a reason for hiding this comment

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

Approving based on Fae's LGTM. I'll follow up with some edits to streamline the content a bit. Thanks again for contributing!

@dedemorton dedemorton added needs_backport PR is waiting to be backported to other branches. and removed needs_backport PR is waiting to be backported to other branches. labels Feb 14, 2020
@dedemorton dedemorton merged commit 8805853 into elastic:master Feb 14, 2020
kvch pushed a commit to kvch/beats that referenced this pull request Feb 20, 2020
@dedemorton
Copy link
Contributor

I've created a meta issue to track the content that requires an edit, so I'm removing the needs_edit
label.

@dedemorton dedemorton removed the needs_edit Indicates that the doc changes need an edit after merging. label Mar 20, 2020
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Apr 22, 2020
dedemorton pushed a commit to dedemorton/beats that referenced this pull request Apr 22, 2020
dedemorton added a commit that referenced this pull request Apr 23, 2020
* [DOCS] Create documentation for fields.yml (#6049) (#12505)

* [docs] Edit docs about field mappings (#17740)

Co-authored-by: George Bridgeman <[email protected]>
dedemorton added a commit that referenced this pull request Apr 23, 2020
* [DOCS] Create documentation for fields.yml (#6049) (#12505)

* [docs] Edit docs about field mappings (#17740)

Co-authored-by: George Bridgeman <[email protected]>
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* [DOCS] Create documentation for fields.yml (elastic#6049) (elastic#12505)

* [docs] Edit docs about field mappings (elastic#17740)

Co-authored-by: George Bridgeman <[email protected]>
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.

5 participants