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.

Sign up for GitHub

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

Jump to bottom

Add a fmt build target and make use of a go build cache. #1558

Merged
merged 2 commits into from
Dec 17, 2018
Merged

Add a fmt build target and make use of a go build cache. #1558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c8ee78
build: Add a `fmt` target which runs `gofmt` on all files.
Dec 10, 2018
d5de835
Use a go build cache to speed up builds.
Dec 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ test: test-unit ## run tests
test-coverage: ## run test coverage
./scripts/test/unit-with-coverage $(shell go list ./... | grep -vE '/vendor/|/e2e/')

.PHONY: fmt
fmt:
go list -f {{.Dir}} ./... | xargs gofmt -w -s -d
Copy link
Member

Choose a reason for hiding this comment

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

oh, actually; do we need to exclude vendor here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not according to my testing when I wrote it:

$ go list -f {{.Dir}} ./... | grep vendor
$

Copy link
Member

Choose a reason for hiding this comment

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

Interesting; wonder why it's needed in the other parts of the Makefile then 🤔. Maybe if they're running from inside a different directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it wasn't always the case with older versions of the go tooling. I suspect the two places which grep the vendor dir out are just out of date, but they both also want to exclude e2e so there isn't a whole lot of worthwhile cleanup to be done.


.PHONY: lint
lint: ## run all the lint tools
gometalinter --config gometalinter.json ./...
Expand Down
10 changes: 10 additions & 0 deletions docker.Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ LINTER_IMAGE_NAME = docker-cli-lint$(IMAGE_TAG)
CROSS_IMAGE_NAME = docker-cli-cross$(IMAGE_TAG)
VALIDATE_IMAGE_NAME = docker-cli-shell-validate$(IMAGE_TAG)
E2E_IMAGE_NAME = docker-cli-e2e$(IMAGE_TAG)
GO_BUILD_CACHE ?= y
MOUNTS = -v "$(CURDIR)":/go/src/github.com/docker/cli
CACHE_VOLUME_NAME := docker-cli-dev-cache
ifeq ($(GO_BUILD_CACHE),y)
MOUNTS += -v "$(CACHE_VOLUME_NAME):/root/.cache/go-build"
endif
VERSION = $(shell cat VERSION)
ENVVARS = -e VERSION=$(VERSION) -e GITCOMMIT -e PLATFORM

Expand Down Expand Up @@ -49,6 +54,7 @@ build: binary ## alias for binary
.PHONY: clean
clean: build_docker_image ## clean build artifacts
docker run --rm $(ENVVARS) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make clean
docker volume rm -f $(CACHE_VOLUME_NAME)

.PHONY: test-unit
test-unit: build_docker_image # run unit tests (using go test)
Expand Down Expand Up @@ -81,6 +87,10 @@ shell: dev ## alias for dev
lint: build_linter_image ## run linters
docker run -ti $(ENVVARS) $(MOUNTS) $(LINTER_IMAGE_NAME)

.PHONY: fmt
fmt:
docker run --rm $(ENVVARS) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make fmt

.PHONY: vendor
vendor: build_docker_image vendor.conf ## download dependencies (vendor/) listed in vendor.conf
docker run -ti --rm $(ENVVARS) $(MOUNTS) $(DEV_DOCKER_IMAGE_NAME) make vendor
Expand Down