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

Support HEAD request for http check #2474

Closed
wants to merge 1 commit into from
Closed

Support HEAD request for http check #2474

wants to merge 1 commit into from

Conversation

kamaradclimber
Copy link
Contributor

This will allow to avoid http checks whose output churn a lot (statistics
endpoint for instance). Such checks are quite common when the checked
service does not provide a /health endpoint.

Fix #2473.

@kamaradclimber
Copy link
Contributor Author

If you are interested in such change, tell me and I'll write associated tests.

@kamaradclimber
Copy link
Contributor Author

tests are failing on the master branch https://travis-ci.org/hashicorp/consul/builds/173459471

I'll try to rebase when master goes green again.

In the meantime, feel free to provide feedback

@slackpad
Copy link
Contributor

slackpad commented Nov 8, 2016

Hi @kamaradclimber this looks good! If you'd like to add the associated tests I think we can pull this in.

@kamaradclimber kamaradclimber force-pushed the head branch 2 times, most recently from d8dbfd4 to 4068d02 Compare November 10, 2016 11:04
@kamaradclimber
Copy link
Contributor Author

@slackpad I've added tests. Would you have advices regarding failing tests? I'm unable to see if it is related to my patch or not.

simplechris
simplechris reviewed Nov 17, 2016
View reviewed changes
command/agent/check.go
@@ -420,7 +422,11 @@ func (c *CheckHTTP) run() {

// check is invoked periodically to perform the HTTP check
func (c *CheckHTTP) check() {
req, err := http.NewRequest("GET", c.HTTP, nil)
method := "GET"
Copy link

@simplechris simplechris Nov 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

use http.MethodGet here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

command/agent/check.go
req, err := http.NewRequest("GET", c.HTTP, nil)
method := "GET"
if c.Head {
method = "HEAD"
Copy link

Choose a reason for hiding this comment

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

use http.MethodHead here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@slackpad slackpad added this to the 0.7.4 milestone Nov 18, 2016
@kamaradclimber
Copy link
Contributor Author

thanks for the feedback @simplechris

@kamaradclimber kamaradclimber force-pushed the head branch 2 times, most recently from bc58c26 to 680016e Compare November 28, 2016 10:59
@kamaradclimber
Copy link
Contributor Author

Hi @slackpad,

I finally managed to make test pass. Would you have any feedback regarding this PR?

@kamaradclimber
Copy link
Contributor Author

I've just rebased. @slackpad any feedback?

@kamaradclimber kamaradclimber force-pushed the head branch 2 times, most recently from 5d40964 to c5b40f5 Compare December 27, 2016 11:01
@kamaradclimber
Support HEAD request for http check
d5a492a 
This will allow to avoid http checks whose output churn a lot (statistics
endpoint for instance). Such checks are quite common when the checked
service does not provide a `/health` endpoint.

Fix: hashicorp#2473

Change-Id: Ice13ccf0ecb58be13e17a2843cdac7871e6579ac
@kamaradclimber
Copy link
Contributor Author

Hello @slackpad,

would you have some feedback on this PR?
If it suits you, could you consider it for merge?

Thanks

@kamaradclimber kamaradclimber mentioned this pull request Jan 18, 2017
Closed
@kamaradclimber
Copy link
Contributor Author

Hello @slackpad,

would you have some comments?
Thanks,

@hvindin hvindin mentioned this pull request Feb 26, 2017
Closed
@kamaradclimber
Copy link
Contributor Author

@slackpad @simplechris any feedback on that PR. What can I do to make it progress?

@simplechris
Copy link

¯\_(ツ)_/¯ LGTM

@slackpad slackpad removed this from the Triaged milestone Apr 18, 2017
@slackpad slackpad added type/docs Documentation needs to be created/updated/clarified pr/needs-rebase PR needs to be rebased before merging labels May 1, 2017
@slackpad slackpad added quick-win and removed type/docs Documentation needs to be created/updated/clarified labels May 1, 2017
@slackpad
Copy link
Contributor

slackpad commented May 1, 2017

Sorry for the delay on this one and that I didn't notice this earlier - we should make the HTTP method more generic and not special-case for HEAD here. We will look at converting this over.

@magiconair magiconair mentioned this pull request Jun 4, 2017
Closed
magiconair added a commit that referenced this pull request Jun 4, 2017
@magiconair
agent: Allow custom HTTP headers and method for checks (#3106)
fcb8621 
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
magiconair added a commit that referenced this pull request Jun 4, 2017
@magiconair
agent: Allow custom HTTP headers and method for checks (#3106)
bde7c4a 
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
magiconair added a commit that referenced this pull request Jun 6, 2017
@magiconair
agent: Allow custom HTTP headers and method for checks (#3106)
6f2ab43 
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
@magiconair magiconair closed this in 825f72f Jun 6, 2017
@kamaradclimber
Copy link
Contributor Author

will use the new feature in 0.8.4 thanks!

@kamaradclimber kamaradclimber mentioned this pull request Jul 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simplechris simplechris simplechris left review comments

Assignees
No one assigned
Labels
pr/needs-rebase PR needs to be rebased before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kamaradclimber @slackpad @simplechris