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 gomod #1208

Closed
wants to merge 10 commits into from
Closed

Migrate to gomod #1208

wants to merge 10 commits into from

Conversation

syndbg
Copy link

@syndbg syndbg commented Apr 22, 2019

Goals:

  • Migrate consul-template to go modules. This fixes troubles for anyone attempting to use this project as a library.
  • Update Makefile to support above go modules change.
  • Bump Travis and Golang image used for building to latest Golang 1.12.4 . From what I've seen so far in the project, supporting/testing against multiple Golang versions has never been a target, am I right? If I was to support both 1.11.x and 1.12.x it would be a huge trouble, so that's why I skipped it.
  • Update documentation to remove outdated/stale information about the Makefile and dep.
  • Migrate "download consul" logic from .travis.yml to a Makefile target. This eases everyone's project setup. Now to run the tests all I do is sudo make download-consul followed by make test. No need to download Consul manually by following a link. Also, I make sure that I test using the same Consul used in CI. Docs are also updated to reflect this.

Did I miss something?

Imo this is a huge milestone that would modernize the project a bit, since it's pretty stale by nowadays.

They're supposed to be env vars for any builds, not to produce multiple builds.
@syndbg
Copy link
Author

syndbg commented Apr 22, 2019

Ready for review. Test suite passes and overall looks good to me.

@pearkes

@greut
Copy link
Contributor

greut commented May 25, 2019

It looks good. Although I'd keep the vendor directory and avoid the go.sum. That way, it's backward compatible with older Go versions.

edit the vault, vault/api, vault/sdk split creates some trouble. The tests should not import vault itself but vault/api and vault/sdk.
cf. google/go-cloud#1958 - google/go-cloud#1983

@eikenb
Copy link
Contributor

eikenb commented Jun 18, 2019

Hey @syndbg, thanks for this...

.. and while I appreciate this PR @greut is right in that we really want to refactor before doing this so as to not pull in Vault and all of its dependencies. I'll leave this open for now as you might want to just rework this once that work is done.

@tustvold
Copy link

tustvold commented Jul 5, 2019

I presume the plan is to modify the circle CI configuration to run vault as a service? At the same time might it also be worth moving to doing the same with consul given that consul/testutil no longer exists and doing so removes the additional download step? I'd be happy to submit a PR based on this one making those changes if that is of interest - it should just be some minor changes to dependency_test.go as it is the only thing that depends on anything outside of vault/api?

As an aside depending directly on vault is made particularly fun at the moment by the fact vault currently depends on its API but at a different version to itself, so you end up with an ambiguous import (see here).

Edit: consul/testutil does still exist it has just moved to within an SDK package, it seems similar changes have been made to vault as well.

@eikenb
Copy link
Contributor

eikenb commented Jul 16, 2019

I didn't use docker, as I don't like requiring it for basic tests as it's generally easier to wrap things in docker at a higher level if you don't integrate docker into things. Otherwise see #1230 for details on how I implemented it.

@eikenb
Copy link
Contributor

eikenb commented Aug 5, 2019

@syndbg Thanks for doing this and even though I ended up doing this myself having your version around was handy for checking some things.

@eikenb eikenb closed this Aug 5, 2019
@syndbg syndbg deleted the migrate-to-gomod branch July 2, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants