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

Switch to Go Modules #197

Merged
merged 27 commits into from
Apr 18, 2019
Merged

Switch to Go Modules #197

merged 27 commits into from
Apr 18, 2019

Conversation

gerred
Copy link
Member

@gerred gerred commented Apr 18, 2019

This PR switches to Go Modules. Specifically, it:

  1. Turns on Go Modules and removes support for dep.
  2. Creates new shell scripts for handling code and manifest generation to workaround Go Modules with Kubernetes code generation tools. See: modules: pin dependencies and fix updategenerated projectcontour/contour#1010 for inspiration.
  3. Introduces tools.go to represent binary tool dependencies with Go Modules and bind them to a semver. This is behind a build flag so that they are never compiled in. See: https://github.com/go-modules-by-example/index/blob/ac9bf72/010_tools/README.md for this method. (this addresses @runyontr's concern about tool versioning)
  4. Updates README.md and CONTRIBUTING_GUIDELINES.md with this new information.
  5. Make staticcheck happy by removing some unused test bootstrapping functions.
  6. Adds module and binary caching to CircleCI. On a ~2-4 minute build, this reduces build times to ~1 assuming no dependencies have changed.
  7. Streamlines the Makefile and removes explicit calls to generate and manifest during test, install, and run. This code should only change when APIs are changed, and are now covered in the Contributing Guidelines.
  8. Kicks vendor/ to the curb by putting it in .gitignore.
  9. Breaks your browser on reviewing files changed.

There's a few other static analysis tools I'd like to add around spelling, ineffectual assignments, etc., but we can separate that into another PR.

After this branch is merged, we should go through and delete all old, unused branches (except dynamic-crds as that is being re-written). There's no chance that code can reasonably merge being so far from master anyway. Additionally, this probably causes some merge conflicts with #166 - sorry @fabianbaier. Probably never a good time to rip this particular bandaid off.

There wasn't really a smaller way to make this PR happen. There's enough infrastructure changes that my additional little changes around the Makefile are incidental. They're also really helpful with speed given how we need to generate code now. Generating on every make task was slow already and IMO hinders a smooth development cycle.

BTW - if you have problems testing this, kill your existing package cache (rm -rf $GOPATH/pkg/) and try again. There seems to be a caching issue when having the same dependencies pulled by dep and modules. Not sure what the issue was there, but could not reproduce after removing that package cache. On a fresh clone with other projects in my $GOPATH this doesn't seem to be an issue.

@gerred gerred requested review from runyontr, fabianbaier and kensipe and removed request for runyontr and fabianbaier April 18, 2019 02:52
Makefile Show resolved Hide resolved
test/formatting.sh Show resolved Hide resolved
@@ -20,19 +20,36 @@ jobs:
working_directory: /go/src/github.com/kudobuilder/kudo
steps:
- checkout
- restore_cache:
keys:
- go-mod-v1-{{ checksum "go.sum" }}

# specify any bash command here prefixed with `run: `
- run: make check-formatting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that tests are working... test target should be part of the run requirement

README.md Outdated Show resolved Hide resolved
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

nice work. I really like the immutable build tooling!

@kensipe
Copy link
Member

kensipe commented Apr 18, 2019

there is an issue with having make test as part of the CI.
If rate limits are going to cause tests to fail... I don't want them to stop a PR build... however I would really rather have tests in CI :)

@gerred
Copy link
Member Author

gerred commented Apr 18, 2019

We should have tests in CI for sure. We need to really talk about removing our reliance on the Github API for day-to-day use anyway.

I'll hold it out for now, let's talk about this all live tomorrow to figure out a resolution.

@gerred
Copy link
Member Author

gerred commented Apr 18, 2019

@fabianbaier Go Modules does not use vendor/ and using GOPROXY is the encouraged form of module caching going forward.

@gerred
Copy link
Member Author

gerred commented Apr 18, 2019

oh, I'll fill you in via Slack. Basically, the diff was too large to reasonably review. We will do another PR removing vendor/ again and re-adding it to .gitignore

Copy link
Member

@fabianbaier fabianbaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@gerred gerred merged commit 36caa8c into master Apr 18, 2019
@gerred gerred deleted the go-modules branch April 18, 2019 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants