*: Fixed further not check errors [PART2] (thanos-io#403)

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Author: bwplotka

.circleci/config.yml

@@ -20,6 +20,7 @@ jobs:
               echo "Awesome! GCS integration tests are enabled."
             fi
       - run: make format
+      - run: make errcheck
       - run:
           name: "Run all tests"
           # TODO(bplotka): Setup some S3 tests for CI.
@@ -38,7 +39,6 @@ jobs:
     working_directory: /home/circleci/.go_workspace/src/github.com/improbable-eng/thanos
     steps:
       - checkout
-      - run: make promu
       - run: make crossbuild
       - persist_to_workspace:
           root: .

.errcheck_excludes.txt

@@ -0,0 +1,3 @@
+(github.com/improbable-eng/thanos/vendor/github.com/go-kit/kit/log.Logger).Log
+fmt.Fprintln
+fmt.Fprint

Makefile

@@ -11,91 +11,122 @@ DOCKER_IMAGE_TAG  ?= $(subst /,-,$(shell git rev-parse --abbrev-ref HEAD))-$(she
 # but for simplicity sake we just make sure they exist in the first one, and
 # then keep using those.
 FIRST_GOPATH      ?= $(firstword $(subst :, ,$(shell go env GOPATH)))
-PROMU             ?= $(FIRST_GOPATH)/bin/promu
 GOIMPORTS         ?= $(FIRST_GOPATH)/bin/goimports
+PROMU             ?= $(FIRST_GOPATH)/bin/promu
 DEP               ?= $(FIRST_GOPATH)/bin/dep
+ERRCHECK          ?= $(FIRST_GOPATH)/bin/errcheck
 
-all: install-tools deps format build
+.PHONY: all
+all: deps format errcheck build
+
+# assets repacks all statis assets into go file for easier deploy.
+.PHONY: assets
+assets:
+	@echo ">> deleting asset file"
+	@rm pkg/query/ui/bindata.go || true
+	@echo ">> writing assets"
+	@go get -u github.com/jteeuwen/go-bindata/...
+	@go-bindata $(bindata_flags) -pkg ui -o pkg/query/ui/bindata.go -ignore '(.*\.map|bootstrap\.js|bootstrap-theme\.css|bootstrap\.css)'  pkg/query/ui/templates/... pkg/query/ui/static/...
+	@go fmt ./pkg/query/ui
+
+# build builds Thanos binary using `promu`.
+.PHONY: build
+build: deps $(PROMU)
+	@echo ">> building binaries"
+	@$(PROMU) build --prefix $(PREFIX)
 
+# crossbuild builds all binaries for all platforms.
+.PHONY: crossbuild
+crossbuild: deps $(PROMU)
+	@echo ">> crossbuilding all binaries"
+	$(PROMU) crossbuild -v
+
+# deps fetches all necessary golang dependencies, since they are not checked into repository.
+.PHONY: deps
 deps: vendor
 
-vendor: Gopkg.toml Gopkg.lock | $(DEP)
-	@echo ">> dep ensure"
-	@$(DEP) ensure
+# docker builds docker with no tag.
+.PHONY: docker
+docker: build
+	@echo ">> building docker image '${DOCKER_IMAGE_NAME}'"
+	@docker build -t "${DOCKER_IMAGE_NAME}" .
 
-format: $(GOIMPORTS) deps
-	@echo ">> formatting code"
-	@$(GOIMPORTS) -w $(FILES)
+# docker-push pushes docker image build under `${DOCKER_IMAGE_NAME}` to improbable/"$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG)"
+.PHONY: docker-push
+docker-push:
+	@echo ">> pushing image"
+	@docker tag "${DOCKER_IMAGE_NAME}" improbable/"$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG)"
+	@docker push improbable/"$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG)"
 
-vet:
-	@echo ">> vetting code"
-	@go vet ./...
+# docs regenerates flags in docs for all thanos commands.
+.PHONY: docs
+docs:
+	@go get -u github.com/campoy/embedmd
+	@go build ./cmd/thanos/...
+	@scripts/genflagdocs.sh
 
-# TODO(bplotka): Make errcheck required stage and validate it on CI (once we fix all the issues claimed by errcheck).
-errcheck:
+# errcheck performs static analysis and returns error if any of the errors is not checked.
+.PHONY: errcheck
+errcheck: $(ERRCHECK)
 	@echo ">> errchecking the code"
-	@errcheck -verbose -exclude .errcheck_excludes.txt ./...
+	$(ERRCHECK) -verbose -exclude .errcheck_excludes.txt ./...
 
-build: deps promu
-	@echo ">> building binaries"
-	@$(PROMU) build --prefix $(PREFIX)
+# format formats the code (including imports format).
+.PHONY: format
+format: $(GOIMPORTS) deps
+	@echo ">> formatting code"
+	@$(GOIMPORTS) -w $(FILES)
 
-.PHONY: crossbuild
-crossbuild: deps promu
-	@echo ">> crossbuilding all binaries"
-	$(PROMU) crossbuild -v
+# proto generates golang files from Thanos proto files.
+.PHONY: proto
+proto:
+	@go get -u github.com/gogo/protobuf/protoc-gen-gogofast
+	@./scripts/genproto.sh
 
+# tarball builds release tarball.
 .PHONY: tarball
-tarball: promu
+tarball: $(PROMU)
 	@echo ">> building release tarball"
 	$(PROMU) tarball --prefix $(PREFIX) $(BIN_DIR)
 
-$(GOIMPORTS):
-	@echo ">> fetching goimports"
-	@go get -u golang.org/x/tools/cmd/goimports
-
-.PHONY: promu
-promu:
-	@echo ">> fetching promu"
-	GOOS= GOARCH= go get -u github.com/prometheus/promu
+# test runs all Thanos golang tests.
+.PHONY: test
+test: test-deps
+	@echo ">> running all tests. Do export THANOS_SKIP_GCS_TESTS="true" or/and  export THANOS_SKIP_S3_AWS_TESTS="true" if you want to skip e2e tests against real store buckets"
+	@go test $(shell go list ./... | grep -v /vendor/)
 
-$(DEP):
-	@echo ">> fetching dep"
-	@go get -u github.com/golang/dep/cmd/dep
 
+# test-deps installs dependency for e2e tets.
+.PHONY: test-deps
 test-deps: deps
 	@go install github.com/improbable-eng/thanos/cmd/thanos
 	@go get -u github.com/prometheus/prometheus/cmd/prometheus
 	@go get -u github.com/prometheus/alertmanager/cmd/alertmanager
 
-proto:
-	@go get -u github.com/gogo/protobuf/protoc-gen-gogofast
-	@./scripts/genproto.sh
+# vet vets the code.
+.PHONY: vet
+vet:
+	@echo ">> vetting code"
+	@go vet ./...
 
-test: test-deps
-	@echo ">> running all tests. Do export THANOS_SKIP_GCS_TESTS="true" or/and  export THANOS_SKIP_S3_AWS_TESTS="true" if you want to skip e2e tests against real store buckets"
-	@go test $(shell go list ./... | grep -v /vendor/)
+# non-phony targets
 
-assets:
-	@echo ">> deleting asset file"
-	@rm pkg/query/ui/bindata.go || true
-	@echo ">> writing assets"
-	@go get -u github.com/jteeuwen/go-bindata/...
-	@go-bindata $(bindata_flags) -pkg ui -o pkg/query/ui/bindata.go -ignore '(.*\.map|bootstrap\.js|bootstrap-theme\.css|bootstrap\.css)'  pkg/query/ui/templates/... pkg/query/ui/static/...
-	@go fmt ./pkg/query/ui
+vendor: Gopkg.toml Gopkg.lock | $(DEP)
+	@echo ">> dep ensure"
+	@$(DEP) ensure
 
-docker: build
-	@echo ">> building docker image '${DOCKER_IMAGE_NAME}'"
-	@docker build -t "${DOCKER_IMAGE_NAME}" .
+$(GOIMPORTS):
+	@echo ">> fetching goimports"
+	@go get -u golang.org/x/tools/cmd/goimports
 
-docker-push:
-	@echo ">> pushing image"
-	@docker tag "${DOCKER_IMAGE_NAME}" improbable/"$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG)"
-	@docker push improbable/"$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_TAG)"
+$(PROMU):
+	@echo ">> fetching promu"
+	GOOS= GOARCH= go get -u github.com/prometheus/promu
 
-docs:
-	@go get -u github.com/campoy/embedmd
-	@go build ./cmd/thanos/...
-	@scripts/genflagdocs.sh
+$(DEP):
+	@echo ">> fetching dep"
+	@go get -u github.com/golang/dep/cmd/dep
 
-.PHONY: all install-tools format vet errcheck build assets docker docker-push docs deps
+$(ERRCHECK):
+	@echo ">> fetching errcheck"
+	@go get -u github.com/kisielk/errcheck

cmd/thanos/bucket.go

@@ -58,23 +58,23 @@ func registerBucket(m map[string]setupFunc, app *kingpin.Application, name strin
 	verifyIDWhitelist := verify.Flag("id-whitelist", "Block IDs to verify (and optionally repair) only. "+
 		"If none is specified, all blocks will be verified. Repeated field").Strings()
 	m[name+" verify"] = func(g *run.Group, logger log.Logger, reg *prometheus.Registry, _ opentracing.Tracer, _ bool) error {
-		bkt, err := client.NewBucket(gcsBucket, *s3Config, reg, name)
+		bkt, err := client.NewBucket(logger, gcsBucket, *s3Config, reg, name)
 		if err != nil {
 			return err
 		}
-		defer runutil.LogOnErr(logger, bkt, "bucket client")
+		defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 
 		backupS3Config := *s3Config
 		backupS3Config.Bucket = *verifyBackupS3Bucket
-		backupBkt, err := client.NewBucket(verifyBackupGCSBucket, backupS3Config, reg, name)
+		backupBkt, err := client.NewBucket(logger, verifyBackupGCSBucket, backupS3Config, reg, name)
 		if err == client.ErrNotFound {
 			if *verifyRepair {
 				return errors.Wrap(err, "repair is specified, so backup client is required")
 			}
 		} else if err != nil {
 			return err
 		} else {
-			defer runutil.LogOnErr(logger, backupBkt, "backup bucket client")
+			defer runutil.CloseWithLogOnErr(logger, backupBkt, "backup bucket client")
 		}
 
 		// Dummy actor to immediately kill the group after the run function returns.
@@ -126,15 +126,15 @@ func registerBucket(m map[string]setupFunc, app *kingpin.Application, name strin
 	lsOutput := ls.Flag("output", "Format in which to print each block's information. May be 'json' or custom template.").
 		Short('o').Default("").String()
 	m[name+" ls"] = func(g *run.Group, logger log.Logger, reg *prometheus.Registry, _ opentracing.Tracer, _ bool) error {
-		bkt, err := client.NewBucket(gcsBucket, *s3Config, reg, name)
+		bkt, err := client.NewBucket(logger, gcsBucket, *s3Config, reg, name)
 		if err != nil {
 			return err
 		}
 
 		// Dummy actor to immediately kill the group after the run function returns.
 		g.Add(func() error { return nil }, func(error) {})
 
-		defer runutil.LogOnErr(logger, bkt, "bucket client")
+		defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
 		defer cancel()
@@ -155,7 +155,7 @@ func registerBucket(m map[string]setupFunc, app *kingpin.Application, name strin
 			enc.SetIndent("", "\t")
 
 			printBlock = func(id ulid.ULID) error {
-				m, err := block.DownloadMeta(ctx, bkt, id)
+				m, err := block.DownloadMeta(ctx, logger, bkt, id)
 				if err != nil {
 					return err
 				}
@@ -167,7 +167,7 @@ func registerBucket(m map[string]setupFunc, app *kingpin.Application, name strin
 				return errors.Wrap(err, "invalid template")
 			}
 			printBlock = func(id ulid.ULID) error {
-				m, err := block.DownloadMeta(ctx, bkt, id)
+				m, err := block.DownloadMeta(ctx, logger, bkt, id)
 				if err != nil {
 					return err
 				}

cmd/thanos/compact.go

@@ -82,15 +82,15 @@ func runCompact(
 
 	reg.MustRegister(halted)
 
-	bkt, err := client.NewBucket(&gcsBucket, *s3Config, reg, component)
+	bkt, err := client.NewBucket(logger, &gcsBucket, *s3Config, reg, component)
 	if err != nil {
 		return err
 	}
 
 	// Ensure we close up everything properly.
 	defer func() {
 		if err != nil {
-			runutil.LogOnErr(logger, bkt, "bucket client")
+			runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 		}
 	}()
 
@@ -185,7 +185,7 @@ func runCompact(
 		}
 
 		g.Add(func() error {
-			defer runutil.LogOnErr(logger, bkt, "bucket client")
+			defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 
 			if !wait {
 				return f()

cmd/thanos/downsample.go

@@ -53,15 +53,15 @@ func runDownsample(
 	component string,
 ) error {
 
-	bkt, err := client.NewBucket(&gcsBucket, *s3Config, reg, component)
+	bkt, err := client.NewBucket(logger, &gcsBucket, *s3Config, reg, component)
 	if err != nil {
 		return err
 	}
 
 	// Ensure we close up everything properly.
 	defer func() {
 		if err != nil {
-			runutil.LogOnErr(logger, bkt, "bucket client")
+			runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 		}
 	}()
 
@@ -70,7 +70,7 @@ func runDownsample(
 		ctx, cancel := context.WithCancel(context.Background())
 
 		g.Add(func() error {
-			defer runutil.LogOnErr(logger, bkt, "bucket client")
+			defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client")
 
 			level.Info(logger).Log("msg", "start first pass of downsampling")
 
@@ -118,7 +118,7 @@ func downsampleBucket(
 		if err != nil {
 			return errors.Wrapf(err, "get meta for block %s", id)
 		}
-		defer runutil.LogOnErr(logger, rc, "block reader")
+		defer runutil.CloseWithLogOnErr(logger, rc, "block reader")
 
 		var m block.Meta
 		if err := json.NewDecoder(rc).Decode(&m); err != nil {
@@ -206,13 +206,13 @@ func processDownsampling(ctx context.Context, logger log.Logger, bkt objstore.Bu
 	begin := time.Now()
 	bdir := filepath.Join(dir, m.ULID.String())
 
-	err := block.Download(ctx, bkt, m.ULID, bdir)
+	err := block.Download(ctx, logger, bkt, m.ULID, bdir)
 	if err != nil {
 		return errors.Wrapf(err, "download block %s", m.ULID)
 	}
 	level.Info(logger).Log("msg", "downloaded block", "id", m.ULID, "duration", time.Since(begin))
 
-	if err := block.VerifyIndex(filepath.Join(bdir, block.IndexFilename), m.MinTime, m.MaxTime); err != nil {
+	if err := block.VerifyIndex(logger, filepath.Join(bdir, block.IndexFilename), m.MinTime, m.MaxTime); err != nil {
 		return errors.Wrap(err, "input block index not valid")
 	}
 
@@ -229,9 +229,9 @@ func processDownsampling(ctx context.Context, logger log.Logger, bkt objstore.Bu
 	if err != nil {
 		return errors.Wrapf(err, "open block %s", m.ULID)
 	}
-	defer runutil.LogOnErr(log.With(logger, "outcome", "potential left mmap file handlers left"), b, "tsdb reader")
+	defer runutil.CloseWithLogOnErr(log.With(logger, "outcome", "potential left mmap file handlers left"), b, "tsdb reader")
 
-	id, err := downsample.Downsample(m, b, dir, resolution)
+	id, err := downsample.Downsample(logger, m, b, dir, resolution)
 	if err != nil {
 		return errors.Wrapf(err, "downsample block %s to window %d", m.ULID, resolution)
 	}
@@ -240,13 +240,13 @@ func processDownsampling(ctx context.Context, logger log.Logger, bkt objstore.Bu
 	level.Info(logger).Log("msg", "downsampled block",
 		"from", m.ULID, "to", id, "duration", time.Since(begin))
 
-	if err := block.VerifyIndex(filepath.Join(resdir, block.IndexFilename), m.MinTime, m.MaxTime); err != nil {
+	if err := block.VerifyIndex(logger, filepath.Join(resdir, block.IndexFilename), m.MinTime, m.MaxTime); err != nil {
 		return errors.Wrap(err, "output block index not valid")
 	}
 
 	begin = time.Now()
 
-	err = block.Upload(ctx, bkt, resdir)
+	err = block.Upload(ctx, logger, bkt, resdir)
 	if err != nil {
 		return errors.Wrapf(err, "upload downsampled block %s", id)
 	}

cmd/thanos/main.go

@@ -243,7 +243,7 @@ func metricHTTPListenGroup(g *run.Group, logger log.Logger, reg *prometheus.Regi
 		level.Info(logger).Log("msg", "Listening for metrics", "address", httpBindAddr)
 		return errors.Wrap(http.Serve(l, mux), "serve metrics")
 	}, func(error) {
-		runutil.LogOnErr(logger, l, "metric listener")
+		runutil.CloseWithLogOnErr(logger, l, "metric listener")
 	})
 	return nil
 }