TritonDataCenter / containerpilot

A service for autodiscovery and configuration of applications running in containers
Mozilla Public License 2.0
1.13k stars 135 forks source link

Migrate to Go 1.9.x and Consul 1.0.x #519

Closed jwreagor closed 6 years ago

jwreagor commented 7 years ago

Stub for migrating our build steps and releases to Go 1.9.x.

jwreagor commented 6 years ago

Ok, so I have all tests passing. Interesting side bit... when I bumped Consul to 1.0.0 and made a few small adjustments to how our tests run, it looks like Consul's testutil package was broken. Upon further investigation I found that @magiconair and others have made some required fixes into Consul post-1.0.0 for the testing facilities that ContainerPilot also relies on (re: freeport). Besides our "unit" tests (which rely on a live consul server) all integration tests passed so I knew it was internal to v1.0.0. I've now pegged my feature branch at v1.0.1-rc1 and will continue tracking the progress until the final GM is released.

/cc @misterbisson

EDIT: Minor changes to reflect back story and current status.

magiconair commented 6 years ago

@cheapRoc testutil wasn't broken since it is an internal package of consul that is required for consul's own integration testing and you shouldn't rely on that to be stable. The nomad team ran into this issue as well.

To speed up integration tests and make them more reliable I've initially added an external tool porter to provide tests with unused port numbers on the localhost interface. In a subsequent step we replaced that with the freeport library to remove the dependency on the porter tool.

Having said that, if you need a consul instance for your tests you should start one by executing the binary and control the process. I've significantly sped up the startup time for a -dev instance to be ~100ms and added a -hcl command line option with 1.0.0 so that you can provide arbitrary configuration on the command line without generating a config file since there is no parity between config flags and config files.

Just run consul agent -dev -hcl '<your HCL config>' and you're set.

jwreagor commented 6 years ago

@cheapRoc testutil wasn't broken since it is an internal package of consul that is required for consul's own integration testing and you shouldn't rely on that to be stable. The nomad team ran into this issue as well.

You're correct, I should have phrased that a tad better, it was broken in our use case. The function call worked perfectly fine outside Consul in another test of mine. I also noticed the Nomad team's commits and your added context makes this much clearer.

Having said that, if you need a consul instance for your tests you should start one by executing the binary and control the process. I've significantly sped up the startup time for a -dev instance to be ~100ms and added a -hcl command line option with 1.0.0 so that you can provide arbitrary configuration on the command line without generating a config file since there is no parity between config flags and config files.

Just run consul agent -dev -hcl '' and you're set.

I'm with you, we shouldn't depend on an internal testing utility of a third party dependency.

But I'd also imagine that controlling a consul process ourselves would end up looking vaguely similar to what the testutil package is already providing us through testutil.NewTestServerConfigT. My guess is that's why it was originally used. I'm not against removing this, just playing devil's advocate.

Another (long term) answer would be to stub out the Consul API's HTTP client interface, stop testing Consul's API in our own unit tests, then ensure our integration tests continue to exercise the same behavior we expect today. Something for later.

Really appreciate the feedback Frank.

jwreagor commented 6 years ago

Created #528 to track that testing work separately.

magiconair commented 6 years ago

I think what the nomad team has done is to vendor in the testutil package. I wouldn't mock the API since that is a lot of work. Consul's API tests all run against a running consul instance and that works fine. I can have a look if you want help.

jwreagor commented 6 years ago

That works for me. I should be able to make the change and continue including you in the review. We'll go from there.