Upgrade ContainerPilot to Go 1.9.x and Consul 1.0.x #527
Conversation
jwreagor
changed the title
discovery/consul_test.go
Outdated
| defer testServer.Stop() | ||
|
|
||
| time.Sleep(300 * time.Millisecond) |
There was a problem hiding this comment.
I'm not crazy about this and would much rather add something that checks for Consul's HTTP API endpoint being live. Any suggestions on that?
There was a problem hiding this comment.
Query localhost:8500/v1/status/leader for a non-empty 200 OK response but do that as part of the Start() call. You can use/clone the testutil/retry package for that.
There was a problem hiding this comment.
Our tests don't require leadership election so I was able to just throw a naive GET request at /v1/agent/self which also uses testutil/retry.
|
@magiconair Last commit in this PR adds the exec for Also, I didn't yet remove the old dependencies from our dep manager. I'll do that after we've discussed the patch. |
discovery/test_server.go
Outdated
| "consul or skip this test") | ||
| } | ||
|
|
||
| args := []string{"agent", "-dev"} |
There was a problem hiding this comment.
You're not configuring the port. Add "-http-port", strconv.Itoa(httpPort) to args
| return nil, errors.New("failed starting command") | ||
| } | ||
|
|
||
| httpAddr := fmt.Sprintf("127.0.0.1:%d", httpPort) |
There was a problem hiding this comment.
You can inline this in the struct definition.
return &TestServer{cmd, fmt.Sprintf("127.0.0.1:%d", httpPort)}, nilThere was a problem hiding this comment.
I have a personal preference around inlining. Normally I enjoy the extra bits for readability and searching. In this case I agree, the line can be kept fairly short. 👍
discovery/consul_test.go
Outdated
| defer testServer.Stop() | ||
|
|
||
| time.Sleep(300 * time.Millisecond) |
There was a problem hiding this comment.
Query localhost:8500/v1/status/leader for a non-empty 200 OK response but do that as part of the Start() call. You can use/clone the testutil/retry package for that.
discovery/consul_test.go
Outdated
| @@ -77,18 +77,18 @@ a Consul server for testing. The 'consul' binary must be in the $PATH | |||
| ref https://github.com/hashicorp/consul/tree/master/testutil | |||
| */ | |||
|
|
|||
| var testServer *testutil.TestServer | |||
| var testServer *TestServer | |||
There was a problem hiding this comment.
Don't use global state for tests. Pass the test server or better just the http address into the sub tests since that is all they need AFAICT.
There was a problem hiding this comment.
I agree, artifact of the previous implementation.
While upgrading Consul to version 1.0.0 I found that our tests caused a null pointer exception if there's an error configuring a Consul process to test against. This was due to the fact that we were ignoring errors when configuring Consul and continuing forward as if Consul was functional. This commit properly causes a fatal return from the test and includes the actual error returned by Consul's own internal testing framework (which we use for bootstrapping our own testing).
This commit removes the use case within the discovery package's tests that depended on an internal Consul test package, `testutil`. We replace this with our own TestServer object which is responsible for executing a locally installed `consul` binary. Consul is installed by our Makefile target `tools` for local development use. Ref: TritonDataCenter#528
* Don't use a global variable in tests * Actually use HTTP port passed into `TestServer` * Use `testutil/retry` for waiting for the HTTP API to come up * Inline returned struct definition within `NewTestServer`
|
Rebased against latest master and added @magiconair's CR comments. I also backed down to pinning Consul v1.0.0. I believe we're in good shape. |
jwreagor
changed the title
Ref: #519
This is a WIP PR.
EDIT: Note that Consul is pegged at 1.0.1-rc1 and we'll want to finalize once a GM is released.