setup ConsulConfig and ConsulExtras #252
Conversation
|
So far as I can tell, there's actually no way to get the value of |
| @@ -99,6 +96,7 @@ consul: | |||
| docker rm -f containerpilot_consul > /dev/null 2>&1 || true | |||
| docker run -d -m 256m --name containerpilot_consul \ | |||
| consul:latest agent -dev -client 0.0.0.0 -bind=0.0.0.0 | |||
| docker run --rm --link containerpilot_consul:consul flyinprogrammer/wait4consul | |||
There was a problem hiding this comment.
Let's check this in the test runner and not the makefile; it's possible to run the tests against an existing Consul.
| var consulExtras *discovery.ConsulExtras | ||
| if s.ConsulConfig != nil { | ||
| consulExtras = &discovery.ConsulExtras{ | ||
| DeregisterCriticalServiceAfter: s.ConsulConfig.DeregisterCriticalServiceAfter, |
There was a problem hiding this comment.
Do we ever validate this is a valid time?
| // ConsulConfig handles additional Consul configuration. | ||
| type ConsulConfig struct { | ||
| EnableTagOverride bool `mapstructure:"enabletagoverride"` | ||
| DeregisterCriticalServiceAfter string `mapstructure:"deregistercriticalserviceafter"` |
There was a problem hiding this comment.
I feel like these deserialization annotations should probably be camelCase, given that we use it elsewhere (ex. preStart)
Don't we only ever write it though? Not read it? Or are you concerned (reasonably) about testing it? |
| "timeout": "1ms", | ||
| "tags": ["tag1","tag2"], | ||
| "consul": { | ||
| "enabletagoverride": true |
There was a problem hiding this comment.
We definitely should be testing for misconfigurations for both of these features too.
|
Looking pretty good with just the few questions and comments. It's just about as ugly as I expected in terms of what configuration we end up with. 😀 We should update the documentation as well as part of the same PR. |
|
Alright see what you think of this @tgross and I'll start working on documentation tonight. |
| @@ -91,7 +91,7 @@ func TestReloadSignal(t *testing.T) { | |||
| if err == nil { | |||
| t.Errorf("Invalid configuration did not return error") | |||
| } | |||
| app.ConfigFlag = `{ "consul": "newconsul:8500" }` | |||
| app.ConfigFlag = `{ "consul": "consul:8500" }` | |||
There was a problem hiding this comment.
Why'd we change this? We want to make sure we have a different configuration than we had before during the reload.
There was a problem hiding this comment.
A side effect of the timeout in NewConsulConfig is this needing to be a valid endpoint.
|
|
||
| maxRetry := 30 | ||
| retry := 0 | ||
| // we need to wait for Consul to start and self-elect |
There was a problem hiding this comment.
This section appears to have been added to support #164 but I'd really like that behavior change to be a separate PR and design discussion.
I'm not comfortable adding a hard-coded retry here nor making it non-optional. It's entirely reasonable to have an application start up without Consul being ready, and to have it simply fail to make health checks until Consul is available (so long as ContainerPilot can handle this without crashing, which as far as I'm aware it can currently do just fine).
There was a problem hiding this comment.
Hahaha you give me too much credit @tgross, I'm much lazier than this 😄. I went to go add a WaitForLeader() type method to our tests, and figured why not add it to the code that actually creates the client. I'm in full agreement with you, this will be gone in a moment, and I'd love to work on #164 in a separate PR as I have a similar desire/use case on wanting an app on startup to crash, but if it's already up, to stay running.
| t.Fatalf("First read of %s should show `false` for change", id) | ||
| } | ||
| consul.SendHeartbeat(service) // force registration | ||
| consul.SendHeartbeat(service) // write TTL |
There was a problem hiding this comment.
You should be able to do without the second heartbeat now that #247 has been merged.
There was a problem hiding this comment.
Awesome, I'll try this out in a few minutes.
|
Looks good, with some comments. There are some integration tests failing intermittently -- I kicked the build again to see if it was a flaky test and a different one failed. I suspect that we're seeing the bug I hopefully fixed in #256 yesterday here, so once that's merged we should rebase on master to double-check. |
I await the merge. This is fun, thanks for your help, and I'll refactor this again based on your comments! |
|
Ok #256 has been merged, let's give that rebase a go. |
|
This all LGTM. Let's merge it and I'll release this as 2.6.0. Thanks again for your help @flyinprogrammer! |
This might address:
#213
#218
#243
I should write more tests.