Feature/http check#156
Open
lremes wants to merge 55 commits into
Open
Conversation
* Use `PUT` as the verb for deregistering a service. Consul 1.0 has started to enforce the verbs used: *https://www.consul.io/docs/upgrade-specific.html#http-verbs-are-enforced-in-many-http-apis * Appease rubocop errors
Member
|
If you can resolve the merge conflict, then this looks fine to merge. |
Concerning ACL tokens, the Consul documentation (https://www.consul.io/api/index.html#authentication) says: > Previously this was provided via a ?token= query parameter. This functionality exists on many endpoints for backwards compatibility, but its use is highly discouraged, since it can show up in access logs as part of the URL. The `config.acl_token` setting in Diplomat sets the `token` query string parameter rather than the header. Additionally, it does not set it for all API endpoints (e.g., Services). Setting the header instead fixes both issues.
There is no need for a separate module, all these methods are only used within the rest client.
In some cases you might want to have multiple ::Diplomat::RestClient at the same time but with different configurations. Using a static config forbid this kind of uses. Possible use cases: - Multiple clients with different Tokens - Multiple clients pointing to different Consul cluster This commit should fix WeAreFarmGeek#159
pierresouchay
requested changes
May 25, 2018
pierresouchay
left a comment
pierresouchay
left a comment
Member
There was a problem hiding this comment.
Would you mind rebasing and performing a few changes?
| # @param ttl [String] time (with units) to mark a check down | ||
| # @return [Integer] Status code | ||
| # | ||
| def register_http(check_id, name, notes, url, method, header, interval) |
Member
There was a problem hiding this comment.
There are lots of parameters to this function, would you mind using named parameters instead ?
I would rather use this signature:
def register_http(url, interval, id: nil, id:name, notes: nil, method: 'GET', header: nil)
[...]
endsince both interval and url are mandatory (see https://www.consul.io/api/agent/check.html#register-check )
It would allow to use only the provided parameters.
It also lacks header in ruby documentation as well as calling it headers would probably be more accurate (and specify type)
| ret = @conn.put do |req| | ||
| req.url '/v1/agent/check/register' | ||
| req.body = JSON.generate( | ||
| 'ID' => check_id, 'Name' => name, 'Notes' => notes, 'HTTP' => url, 'Method' => method, |
Member
There was a problem hiding this comment.
timeout might be interesting as well
| expect(check.register_script('foobar-1', 'Foobar', 'Foobar test', '/script/test', '10s')).to eq(true) | ||
| end | ||
|
|
||
| it 'register_http' do |
Member
There was a problem hiding this comment.
Please also test wrong value for parameters:
example:
curl -i --request PUT -d '{"ID":1, "http":"localhost:8500", "interval": "3x"}' http://localhost:8500/v1/agent/check/register
HTTP/1.1 400 Bad Request
Vary: Accept-Encoding
Date: Fri, 25 May 2018 06:52:59 GMT
Content-Length: 78
Content-Type: text/plain; charset=utf-8
…tration_deregistration Support for tokens when registering/unregistering entities on Agent
…figuration Allow to pass Diplomat::Configuration object when using diplomat
Member
|
@lremes Do you mind rebasing to include this PR with my suggestions? |
README: Drop defunct badge, use SVG
Suggest using X-Consul-Token for ACL tokens
Member
|
@lremes Do you mind rebasing to include this PR with my suggestions? |
Support for Output in TTL checks
- Fix incorrect verbs for checks Fix WeAreFarmGeek#173 - Use json_pure to avoid the need for installing a compiler. Fix WeAreFarmGeek#177 - Allow updating Output with TTL checks WeAreFarmGeek#178
simpler convert_to_hash function
Member
|
@lremes ping? Rebase it please, I'll close it if no answer in 1 month |
Using the configs hash target host or token can be different at each call
…where Enable configuration override on each consul api call
Set flags attribute on KVPair during lock acquisition and release
…ek#181) * Avoid having a HTTP 302 if key requested starts with '/' This might fix: WeAreFarmGeek#171 * Fixed rubocop warning
This release cleanup a lot the existing APIs while preserving ascending compatibility. It will avoid relying on side effects to configure diplomat and allow to override most configuration options per API call as implemented in WeAreFarmGeek#179. It is now easy to use one instance of the lib and to perform several calls with different tokens and/or consistency options for instance.
As every data was converted to json, raw data usage was impossible Reverting all json conversion to source function and remove systematic conversion. Fixes: WeAreFarmGeek#187
Author
|
Rebasing. |
Adds register_http to access methods and Updated README and Changelog
… into feature/http-check # Conflicts: # CHANGELOG.md # README.md # lib/diplomat/check.rb
Member
|
@lremes can you fix the tests? |
Author
What do you want the changelog to say, as it is the only conflict and you need to edit it anyway? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for registering a HTTP service check with consul