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

testutil: Should ensure consul executable is correct version or use embedded code #2465

Open
stevehorsfield opened this issue Nov 3, 2016 · 5 comments
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/enhancement Proposed improvement or new feature

Comments

@stevehorsfield
Copy link

Client: 0.7.0
Server: 0.7.0

Description of the Issue (and unexpected/desired result)

Products such as consul-template use testutil to spin up Consul servers. While developing a patch I was hitting unexpected issues in tests. The cause was a local consul binary being out of date. Note that consul-template includes the full consul repo as a vendor source.

Reproduction steps

  • Install consul binary 0.6.3 into the path
  • Download consul-template and issue make test

Various failures are experienced.

Upgrade consul binary to 0.7.0 and issues are resolved.

Suggestion

It would be nice if the test util did one of the following:

  • validated the version of the Consul binary it uses to match the configured version
  • validated the version of the Consul binary it uses to a supported version
  • used the local Consul code to spin up the server
  • At least indicated the mismatch
@slackpad slackpad added the type/enhancement Proposed improvement or new feature label Nov 3, 2016
@slackpad
Copy link
Contributor

slackpad commented Nov 3, 2016

Hi @stevehorsfield I've been bitten by this kind of thing, too. We could probably leverage go-getter to pull in a known revision that you point at, with maybe an optional way to say "build my local Consul".

@slackpad
Copy link
Contributor

slackpad commented Nov 3, 2016

Actually thinking more on this, we should probably just ensure the version is >= some expected version - that'll hit both cases and should work well and be a simple change.

@stevehorsfield
Copy link
Author

That sounds like a good solution. I agree there's a lot of permutations to consider here. Thanks for the quick response.

@magiconair
Copy link
Contributor

IMO, the test code should not depend on an external binary. We should spin up the consul server from within. I have that on my plate.

@magiconair
Copy link
Contributor

#3037 is work towards solving that properly.

@slackpad slackpad added the theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization label May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/enhancement Proposed improvement or new feature
Projects
None yet
Development

No branches or pull requests

3 participants