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

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

Closed
jwreagor opened this issue Oct 17, 2017 · 6 comments
Closed

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

jwreagor opened this issue Oct 17, 2017 · 6 comments

Comments

@jwreagor
Copy link
Contributor

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

@jwreagor jwreagor changed the title Migrate to Go 1.9.x Migrate to Go 1.9.x and Consul 1.0.0 Nov 12, 2017
@jwreagor
Copy link
Contributor Author

jwreagor commented Nov 12, 2017

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.

@jwreagor jwreagor changed the title Migrate to Go 1.9.x and Consul 1.0.0 Migrate to Go 1.9.x and Consul 1.0.1 Nov 12, 2017
@magiconair
Copy link

@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
Copy link
Contributor Author

jwreagor commented Nov 13, 2017

@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
Copy link
Contributor Author

Created #528 to track that testing work separately.

@magiconair
Copy link

magiconair commented Nov 13, 2017

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
Copy link
Contributor Author

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

@jwreagor jwreagor changed the title Migrate to Go 1.9.x and Consul 1.0.1 Migrate to Go 1.9.x and Consul 1.0.x Nov 14, 2017
@jwreagor jwreagor added released and removed open PR labels Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants