Gomod Updates #4
Conversation
boilerplate/update.sh
Outdated
| @@ -10,7 +10,7 @@ set -e | |||
| DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null && pwd )" | |||
|
|
|||
| OUT="$(mktemp -d)" | |||
| git clone [email protected]:lyft/boilerplate.git "${OUT}" | |||
| git clone --single-branch --branch gomod [email protected]:lyft/boilerplate.git "${OUT}" | |||
There was a problem hiding this comment.
will revert this before merging, just here for testing.
|
|
||
| .PHONY: lint | ||
| lint: #lints the package for common code smells | ||
| which golangci-lint || curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | bash -s -- -b $$GOPATH/bin v1.16.0 | ||
| golangci-lint run --exclude deprecated | ||
| which golangci-lint || make download_tooling |
There was a problem hiding this comment.
Calling make from make feels a bit weird. Is there any reason we have to do this? Can we just make lint depend on download_tooling and push down which check to https://github.com/lyft/boilerplate/pull/4/files#diff-3ec8ae048736f7fca33bd837e03342cbR31
|
|
||
| # If code is failing goimports linter, this will fix. | ||
| # skips 'vendor' | ||
| .PHONY: goimports | ||
| goimports: | ||
| @boilerplate/lyft/golang_test_targets/goimports | ||
|
|
||
| .PHONY: install download_tooling mod_download |
There was a problem hiding this comment.
install is declared as phony at https://github.com/lyft/boilerplate/pull/4/files#diff-aff368f43e9bd90b088a8b4e6c01bf8fR25 already. Maybe remove that line? Is the convention here to have phony defined separately for each goal?
| which dep || (curl "https://raw.githubusercontent.com/golang/dep/${DEP_SHA}/install.sh" | sh) | ||
| dep ensure | ||
| install: | ||
| download_tooling mod_download |
There was a problem hiding this comment.
I think this should be right after :.
install: download_tooling mod_download
| # The reason we cannot turn off module entirely and install is that we need the replace statement in go.mod | ||
| # because we are installing a mockery fork. Turning it off would result installing the original not the fork. | ||
| # However, because the installation of these tools themselves sometimes modifies the go.mod/go.sum files. We don't | ||
| # want this either. So instead, we're going to copy those files into a temporary directory, do the installation, and= |
|
|
||
| for tool in "${tools[@]}" | ||
| do | ||
| go_install_tool $tool |
There was a problem hiding this comment.
Maybe do a which ... || ... here?
|
Tried addressing comments in #5 |
| # List of tools to go get | ||
| # In the format of "<cli>:<package>" or ":<package>" if no cli | ||
| tools=( | ||
| "mockery:github.com/vektra/mockery/cmd/mockery" |
There was a problem hiding this comment.
@honnix let me take one more look tomorrow before merging this PR. I want to add version numbers in here as well.
| # TO OPT OUT OF UPDATES, SEE https://github.com/lyft/boilerplate/blob/master/Readme.rst | ||
|
|
||
| DEP_SHA=1f7c19e5f52f49ffb9f956f64c010be14683468b | ||
|
|
There was a problem hiding this comment.
Oops it seems I introduced this empty line.
| cli=$(echo "$tool" | cut -d':' -f1) | ||
| package=$(echo "$tool" | cut -d':' -f2) | ||
| echo "Installing ${package}" | ||
| GO111MODULE=on go install "$package" |
There was a problem hiding this comment.
So we install it anyway without checking existence? If that is the case we don't need that column hack.
There was a problem hiding this comment.
yeah, cuz we want to make sure the version is the right one
There was a problem hiding this comment.
Yeh I see that point totally. We can remove that column cut thing so the code became much simpler. Also I would suggest we enable shellcheck on TravisCI. NVM there is no CI on this repo.
|
Minor comment otherwise LGTM. 👍 |
| "mockery:github.com/vektra/mockery/cmd/mockery" | ||
| "pflags:github.com/lyft/flytestdlib/cli/pflags" | ||
| "golangci-lint:github.com/golangci/golangci-lint/cmd/golangci-lint" | ||
| "enumer:github.com/alvaroloes/enumer" |
|
👏 |
This reflects changes done in flyteorg/boilerplate#4
This reflects changes done in flyteorg/boilerplate#4
This updates the boilerplate to use go modules instead of dep and follows the changes that have already been made: