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

agent: support custom header and method for http checks (#3106) #3107

Merged
merged 9 commits into from Jun 6, 2017

Conversation

@magiconair
Copy link
Contributor

@magiconair magiconair commented Jun 4, 2017

Still need to write a test

@magiconair magiconair force-pushed the issue-3106-custom-header-and-method branch from fdbe311 to fcb8621 Jun 4, 2017
Copy link
Contributor

@slackpad slackpad left a comment

Noted a couple small things - otherwise looks good once tests are added. You'll also need to update the documentation.

Loading

Timeout string `json:",omitempty"`
TTL string `json:",omitempty"`
HTTP string `json:",omitempty"`
Header map[string][]string `json:",omitempty"`
Copy link
Contributor

@slackpad slackpad Jun 4, 2017

Choose a reason for hiding this comment

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

Headers, maybe?

Loading

Copy link
Contributor Author

@magiconair magiconair Jun 4, 2017

Choose a reason for hiding this comment

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

I used https://golang.org/pkg/net/http/#Header as an example. But we also have header configuration in the config file which is Headers and allows only a single value per header name. :( (https://github.com/hashicorp/consul/blob/master/command/agent/config.go#L704-L705)

Generally, I prefer to use what the stdlib is using (Header with map[string][]string) since then there is no necessity for type conversion and the names are the same. I'd also try/deprecate the other value but I'm open for suggestions.

Loading

Copy link
Contributor

@slackpad slackpad Jun 4, 2017

Choose a reason for hiding this comment

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

Ok - better to match the std lib for new things.

Loading

@@ -1437,6 +1437,20 @@ func FixupCheckType(raw interface{}) error {
case "service_id":
rawMap["serviceid"] = v
delete(rawMap, k)
case "header":
vm, ok := v.(map[string]interface{})
Copy link
Contributor

@slackpad slackpad Jun 4, 2017

Choose a reason for hiding this comment

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

This seems like it should be returning errors if things aren't what we expect, otherwise we could silently skip these.

Loading

Copy link
Contributor Author

@magiconair magiconair Jun 4, 2017

Choose a reason for hiding this comment

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

I'll add that but I want to have a closer look at this method in general since it looks like mangling map[string]interface{} is the wrong approach to this. But that could be fixed with the decoupling of user and runtime config where user config represents exactly what the user is providing and then we just parse the JSON and interpret it afterwards instead of mangling it.

Loading

@magiconair magiconair force-pushed the issue-3106-custom-header-and-method branch from fcb8621 to bde7c4a Jun 4, 2017
This patch adds support for custom headers and
method for HTTP checks.

Fixes #2474
Fixes #2657
Fixes #3106
@magiconair magiconair force-pushed the issue-3106-custom-header-and-method branch from bde7c4a to 0767510 Jun 6, 2017
@magiconair magiconair force-pushed the issue-3106-custom-header-and-method branch 3 times, most recently from de8d239 to f51bc0e Jun 6, 2017
@magiconair magiconair changed the title Issue 3106 custom header and method agent: support custom header and method for http checks (#3106) Jun 6, 2017
@magiconair magiconair force-pushed the issue-3106-custom-header-and-method branch from f51bc0e to 3df4999 Jun 6, 2017
Copy link
Contributor

@slackpad slackpad left a comment

Loading

@magiconair
Copy link
Contributor Author

@magiconair magiconair commented Jun 6, 2017

🤦‍♂️ forgot the docs. Will add.

Loading

Copy link
Contributor

@slackpad slackpad left a comment

LGTM

Loading

@magiconair magiconair merged commit 825f72f into master Jun 6, 2017
1 check failed
Loading
@magiconair magiconair deleted the issue-3106-custom-header-and-method branch Jun 6, 2017
@ashald
Copy link
Contributor

@ashald ashald commented Jun 7, 2017

@magiconair thank you! 👍

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants