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

Add HTTP status phrase in reponse for metricsbeat http module (#5470) #5484

Merged
merged 3 commits into from
Nov 8, 2017

Conversation

liketic
Copy link

@liketic liketic commented Oct 31, 2017

Closes #5470

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@exekias exekias 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!! You will need to run make update to update docs with the new field

@liketic
Copy link
Author

liketic commented Nov 1, 2017

@exekias Yes, thanks. I pushed 0997023 . Please review again.

@christiangalsterer
Copy link
Contributor

Actually I was just to submit a PR as well... seems that @liketic was a little bit faster ;-)
I would like to make one additional suggestion:

@@ -37,6 +37,10 @@
type: keyword
description: >
The HTTP status code
- name: status_phrase
type: keyword
description: >
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an example, e.g. OK for a 200 response?

@christiangalsterer
Copy link
Contributor

Just curious if you have seen my previous comment on harmonizing the names.
@ruflin: What do you think?

@ruflin
Copy link
Contributor

ruflin commented Nov 6, 2017

@christiangalsterer +1 on harmonizing field names.
@liketic WDYT?

@liketic
Copy link
Author

liketic commented Nov 6, 2017

@christiangalsterer @ruflin I think it's OK to me.

@ruflin
Copy link
Contributor

ruflin commented Nov 6, 2017

@liketic Could you update the PR accordingly in case @andrewkroh has no objections?

@liketic
Copy link
Author

liketic commented Nov 8, 2017

@ruflin @christiangalsterer Apologises for the delay. I pushed 5a381dd . Please review again.

@christiangalsterer
Copy link
Contributor

LGTM

@ruflin ruflin merged commit a5c9c5e into elastic:master Nov 8, 2017
@ruflin
Copy link
Contributor

ruflin commented Nov 8, 2017

jenkins, test it

@liketic liketic deleted the feature/issues/5470 branch November 9, 2017 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants