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.

Sign up for GitHub

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

Jump to bottom

Change type of text fields to keyword #10542

Merged
merged 3 commits into from
Feb 5, 2019
Merged

Change type of text fields to keyword #10542

merged 3 commits into from
Feb 5, 2019

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 4, 2019

@kvch kvch requested a review from a team as a code owner February 4, 2019 19:07
@kvch kvch mentioned this pull request Feb 4, 2019
Closed
@kvch
Copy link
Contributor Author

kvch commented Feb 4, 2019

jenkins test this

@kvch kvch changed the title Change type of text fileds to keyword Change type of text fields to keyword Feb 4, 2019
webmat
webmat approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Any idea why only 2/7 of the fields end up changing in the asciidoc? Are the other ones (which are nested deeper) skipped because there's too many?

@ruflin
Copy link
Contributor

ruflin commented Feb 5, 2019

Very good point by @webmat I assume something is wrong with the indentation or naming in the fields.yml which means all the other fields are not part of the template either.

@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019 •
edited
Loading

@ruflin Indeed. I've fixed the indentation problem. Now all required fields are changed.

ruflin
ruflin approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Sounds like we probably had the indentation issue already in 6.x which would be a bug. Could you backport this also to 6.6 and 6.x?

@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

Sure.

@kvch kvch added the needs_backport PR is waiting to be backported to other branches. label Feb 5, 2019
@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

Opened two backports:

webmat
webmat approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@kvch kvch force-pushed the fix-journalbeat-make-texts-keywords branch from 440abd0 to be8ba58 Compare February 5, 2019 21:17
@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

Rebased

webmat
webmat approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Still LGTM ;-)

@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

Travis refuses to start, so I am waiting for jenkins.

@kvch kvch force-pushed the fix-journalbeat-make-texts-keywords branch from be8ba58 to c44de40 Compare February 5, 2019 21:52
@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

All relevant tests are green.

@kvch kvch merged commit 3b20a57 into elastic:master Feb 5, 2019
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
@kvch
Change type of text fields to keyword (elastic#10542)
89dc1b8
@kvch kvch removed the needs_backport PR is waiting to be backported to other branches. label Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@urso urso urso approved these changes

@webmat webmat webmat approved these changes

Assignees
No one assigned
Labels
Journalbeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kvch @ruflin @webmat @urso