-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 1.13.7 #7262
Switch to Go 1.13.7 #7262
Conversation
@@ -10,13 +10,6 @@ GOTOOLS = \ | |||
github.com/vektra/mockery/cmd/mockery | |||
|
|||
GOTAGS ?= | |||
GOMODULES ?= ./... ./api/... ./sdk/... | |||
GOFILES ?= $(shell go list $(GOMODULES) | grep -v /vendor/) | |||
ifeq ($(origin GOTEST_PKGS_EXCLUDE), undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the GOTEST_PKGS_EXCLUDE
feature completely rather than figure out how to use that to omit some packages from all 3 submodules and then check to see if we can omit one or more of the submodules completely from tests. Seems like you're better off just doing go test
manually at that point then having a crazy bash one liner nested inside of some make targets.
go tool cover -html=coverage.out | ||
|
||
test: other-consul dev-build vet test-install-deps test-internal | ||
|
||
test-install-deps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go modules has us covered now.
gotestsum --format=short-verbose --junitfile $TEST_RESULTS_DIR/gotestsum-report.xml -- -tags=$GOTAGS -cover -coverprofile=cov_sdk.part $PACKAGE_NAMES | ||
|
||
# save coverage report parts | ||
- persist_to_workspace: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see these coverage reports all merge together and just work. I wasn't sure if it would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check that it actually did something sane with the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did! If you grab the artifact from here: https://circleci.com/gh/hashicorp/consul/129822#artifacts/containers/0 you can browse to the api and sdk packages to check coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few changes in the makefile that are issues from a glance (I didn't run locally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- You can no longer cross submodule boundaries with ./... in go subcommands like `go list` or `go test`. The makefile and CI scripts were updated accordingly. - Also of note: `go mod vendor` now omits things build ignored.
Fixes #6879