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

Move to Go Modules #230

Merged
merged 7 commits into from
Mar 5, 2019
Merged

Conversation

jacobbednarz
Copy link
Member

Updates the provider to move to go mod for dependency management along
with some helpful documentation on how we expect dependency management
to work.

Closes #206

cc @appilon @radeksimko

Documents the dependency management process for the provider and how we
expect `go mod` to be used.

Closes #206
@ghost ghost added the size/XXL label Mar 4, 2019
@appilon
Copy link

appilon commented Mar 4, 2019

Looking good so far thank you @jacobbednarz . We would just ask for a few extra tweaks in our effort to keep all the providers consistent.

  1. Could you please add the following to the travis config to ensure compatibility
env:
  - GO111MODULE=on GOFLAGS=-mod=vendor

I also noticed this travis config contains several versions of Go. I would recommend just sticking to one, while we continue to vendor for the time being, eventually we plan to fully adopt go modules and older versions (pre 1.11) would not be supported.

  1. This also means that for development go1.11 is required, could you update the README to reflect that.

  2. Unfortunately go modules has had some growing pains through go1.11's releases, as such we have begun encoding the specific version of go used into a goenv file: .go-version. In this case I can see you have used go1.12 (it was a new change from go1.11 to go1.12 that they would add a line of the version of go used for modules in the go.mod file). I think right now we prefer that file to have 1.11.5, but 1.12.0 is likely fine (basically anything >= go1.11.4 is fine, there was a bug prior to that release).

  3. Could you delete all mention/use of govendor and the vendor-status make target from the travis config and makefile. Go modules does not have an equivalent to govendor status

  4. (Optional) We have a reference to a version of Terraform v0.11 that uses go modules, this will significantly clean up this project's go.mod file. To update, simply run

GO111MODULE=on go get github.com/hashicorp/terraform@sdk-v0.11-with-go-modules
GO111MODULE=on go mod tidy
GO111MODULE=on go mod vendor

My apologies I know that is a lot of little requests. To better illustrate the changes I am linking a merged PR from another provider. Thank you for your time and contribution 😄 !
https://github.com/terraform-providers/terraform-provider-gitlab/pull/95/files

@jacobbednarz
Copy link
Member Author

Could you please add the following to the travis config to ensure compatibility

env:
  - GO111MODULE=on GOFLAGS=-mod=vendor

I also noticed this travis config contains several versions of Go. I would recommend just sticking to one, while we continue to vendor for the time being, eventually we plan to fully adopt go modules and older versions (pre 1.11) would not be supported.

Sounds good, addressed in 79dbf68. I agree we should only test on the single version considering we intend to package it up anyway. I was only perusing the Travis CI UI when you mentioned this and realised we weren't actually using go modules so great find!

This also means that for development go1.11 is required, could you update the README to reflect that.

Fixed in 3db01ea

Unfortunately go modules has had some growing pains through go1.11's releases, as such we have begun encoding the specific version of go used into a goenv file: .go-version. In this case I can see you have used go1.12 (it was a new change from go1.11 to go1.12 that they would add a line of the version of go used for modules in the go.mod file). I think right now we prefer that file to have 1.11.5, but 1.12.0 is likely fine (basically anything >= go1.11.4 is fine, there was a bug prior to that release).

I added the .go-version file in 2f80b43 however for anyone following along with this, I needed to manually fetch 1.11.5 to get it to work due to the version not being in the latest 1.x release of goenv (via Homebrew, probably won't be an issue if you clone it).

$ brew install goenv
$ cd $(brew --prefix)/Cellar/goenv/1.23.3/plugins/go-build/share/go-build
$ wget https://raw.githubusercontent.com/syndbg/goenv/master/plugins/go-build/share/go-build/1.11.5
$ goenv install 1.11.5

Could you delete all mention/use of govendor and the vendor-status make target from the travis config and makefile. Go modules does not have an equivalent to govendor status

Thanks for raising this. I did remove it in the README but missed the Makefile references. Cleaned up in b6c22cc.

(Optional) We have a reference to a version of Terraform v0.11 that uses go modules, this will significantly clean up this project's go.mod file.

Awesome, done in 24495ca.

Have confirmed this PR builds and applies changes to infrastructure as expected.

@appilon
Copy link

appilon commented Mar 5, 2019

@jacobbednarz I ran the acceptance tests, there are no new failures except 1 test that is reported as being flaky, feel free to merge this whenever

@jacobbednarz
Copy link
Member Author

Thanks for taking the time to review this one! I've also just had @radeksimko mention the same thing about the flakey test so I'll address that before getting a new release cut later today (potentially tomorrow).

@jacobbednarz jacobbednarz merged commit 42d4974 into cloudflare:master Mar 5, 2019
@jacobbednarz jacobbednarz deleted the move-to-go-modules branch March 5, 2019 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants