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

heartbeat: setup default ports in http monitors #3924

Merged
merged 3 commits into from
Apr 10, 2017
Merged

heartbeat: setup default ports in http monitors #3924

merged 3 commits into from
Apr 10, 2017

Conversation

7AC
Copy link
Contributor

@7AC 7AC commented Apr 5, 2017

Default http to 80 and https to 443

Resolves #3915

@7AC 7AC added the review label Apr 5, 2017
Default http to 80 and https to 443

Resolves #3915
@ruflin
Copy link
Member

ruflin commented Apr 6, 2017

Could you update the changelog with an entry and potentially also the docs? Should we backport this?

@7AC
Copy link
Contributor Author

7AC commented Apr 6, 2017

I updated the changelog, I'm not sure the doc deserves an entry for this. About backporting I'm open to suggestions..

Copy link
Member

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

LGTM. I would be nice to have a quick test to confirm that the default is set correctly.

About backporting: I suggest we backport it to 5.x (5.4).

Also refactor the code, turns out net.SplitHostPort() doesn't like hosts
without ports.
@monicasarbu monicasarbu merged commit 77c5b74 into elastic:master Apr 10, 2017
@tsg tsg added needs_backport PR is waiting to be backported to other branches. v5.4.0 labels Apr 10, 2017
@tsg
Copy link
Contributor

tsg commented Apr 10, 2017

+1 to including this one in 5.4

@7AC 7AC deleted the fix/heartbeat_default_http_port branch April 10, 2017 16:07
@7AC 7AC removed the needs_backport PR is waiting to be backported to other branches. label Apr 10, 2017
tsg pushed a commit that referenced this pull request Apr 11, 2017
…tors (#3978)

* heartbeat: setup default ports in http monitors

Default http to 80 and https to 443

Resolves #3915

(cherry picked from commit c888f3f)

* Add entry to changelog

(cherry picked from commit aa40255)

* Add test

Also refactor the code, turns out net.SplitHostPort() doesn't like hosts
without ports.

(cherry picked from commit bd6898c)

* Add newline
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.

4 participants