diff --git a/.circleci/config.yml b/.circleci/config.yml index f4560e53410..2f9e8a0f91a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -3,7 +3,7 @@ version: 2.1 # https://circleci.com/blog/circleci-hacks-reuse-yaml-in-your-circleci-config-with-yaml/ defaults: &defaults docker: - - image: cortexproject/build-image:master-d74af5958 + - image: quay.io/cortexproject/build-image:update-lint-ae47f740a working_directory: /go/src/github.com/cortexproject/cortex workflows: @@ -68,6 +68,10 @@ jobs: - run: name: Lint command: make BUILD_IN_CONTAINER=false lint + # fails to run everything first time - see https://github.com/golangci/golangci-lint/issues/866 + - run: + name: Lint again + command: make BUILD_IN_CONTAINER=false lint - run: name: Check vendor directory is consistent. command: make BUILD_IN_CONTAINER=false mod-check diff --git a/.errcheck-exclude b/.errcheck-exclude new file mode 100644 index 00000000000..d52dc39c00f --- /dev/null +++ b/.errcheck-exclude @@ -0,0 +1,3 @@ +io/ioutil.WriteFile +io/ioutil.ReadFile +(github.com/go-kit/kit/log.Logger).Log diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000000..cb5c5295b73 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,8 @@ +output: + format: line-number + +linters-settings: + errcheck: + # path to a file containing a list of functions to exclude from checking + # see https://github.com/kisielk/errcheck#excluding-functions for details + exclude: ./.errcheck-exclude diff --git a/Makefile b/Makefile index fd07dd6a4e8..a7b55d62a05 100644 --- a/Makefile +++ b/Makefile @@ -78,16 +78,16 @@ GO_FLAGS := -ldflags "-extldflags \"-static\" -s -w" -tags netgo ifeq ($(BUILD_IN_CONTAINER),true) +GOVOLUMES= -v $(shell pwd)/.cache:/go/cache:delegated \ + -v $(shell pwd)/.pkg:/go/pkg:delegated \ + -v $(shell pwd):/go/src/github.com/cortexproject/cortex:delegated + exes $(EXES) protos $(PROTO_GOS) lint test shell mod-check check-protos web-build web-pre web-deploy: build-image/$(UPTODATE) @mkdir -p $(shell pwd)/.pkg @mkdir -p $(shell pwd)/.cache @echo @echo ">>>> Entering build container: $@" - @$(SUDO) time docker run $(RM) $(TTY) -i \ - -v $(shell pwd)/.cache:/go/cache \ - -v $(shell pwd)/.pkg:/go/pkg \ - -v $(shell pwd):/go/src/github.com/cortexproject/cortex \ - $(BUILD_IMAGE) $@; + @$(SUDO) time docker run $(RM) $(TTY) -i $(GOVOLUMES) $(BUILD_IMAGE) $@; configs-integration-test: build-image/$(UPTODATE) @mkdir -p $(shell pwd)/.pkg @@ -95,10 +95,7 @@ configs-integration-test: build-image/$(UPTODATE) @DB_CONTAINER="$$(docker run -d -e 'POSTGRES_DB=configs_test' postgres:9.6)"; \ echo ; \ echo ">>>> Entering build container: $@"; \ - $(SUDO) docker run $(RM) $(TTY) -i \ - -v $(shell pwd)/.cache:/go/cache \ - -v $(shell pwd)/.pkg:/go/pkg \ - -v $(shell pwd):/go/src/github.com/cortexproject/cortex \ + $(SUDO) docker run $(RM) $(TTY) -i $(GOVOLUMES) \ -v $(shell pwd)/cmd/cortex/migrations:/migrations \ --workdir /go/src/github.com/cortexproject/cortex \ --link "$$DB_CONTAINER":configs-db.cortex.local \ @@ -121,11 +118,8 @@ protos: $(PROTO_GOS) protoc -I $(GOPATH)/src:./vendor:./$(@D) --gogoslick_out=plugins=grpc,Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types,:./$(@D) ./$(patsubst %.pb.go,%.proto,$@) lint: - ./tools/lint -notestpackage -novet -ignorespelling queriers -ignorespelling Queriers . - - # -stdmethods=false disables checks for non-standard signatures for methods with familiar names. - # This is needed because the Prometheus storage interface requires a non-standard Seek() method. - go vet -stdmethods=false ./pkg/... + misspell -error docs + golangci-lint run --new-from-rev ed7c302fd968 --build-tags netgo --timeout=5m --enable golint --enable misspell --enable gofmt test: ./tools/test -netgo diff --git a/build-image/Dockerfile b/build-image/Dockerfile index 10e2a27c0f3..89f10ee396d 100644 --- a/build-image/Dockerfile +++ b/build-image/Dockerfile @@ -4,9 +4,6 @@ RUN apt-get update && apt-get install -y curl python-requests python-yaml file j RUN curl -sL https://deb.nodesource.com/setup_6.x | sh - RUN apt-get install -y nodejs npm && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* RUN npm install -g postcss-cli autoprefixer -RUN go clean -i net && \ - go install -tags netgo std && \ - go install -race -tags netgo std ENV HUGO_VERSION=v0.59.1 RUN git clone https://github.com/gohugoio/hugo.git --branch ${HUGO_VERSION} --depth 1 && \ cd hugo && go install --tags extended && cd ../ && \ @@ -15,17 +12,13 @@ RUN curl -fsSLo shfmt https://github.com/mvdan/sh/releases/download/v1.3.0/shfmt echo "b1925c2c405458811f0c227266402cf1868b4de529f114722c2e3a5af4ac7bb2 shfmt" | sha256sum -c && \ chmod +x shfmt && \ mv shfmt /usr/bin +RUN curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b /usr/bin v1.21.0 RUN GO111MODULE=on go get -tags netgo \ - github.com/fzipp/gocyclo \ - golang.org/x/lint/golint \ - github.com/kisielk/errcheck@v1.2.0 \ github.com/client9/misspell/cmd/misspell@v0.3.4 \ github.com/golang/protobuf/protoc-gen-go@v1.3.1 \ github.com/gogo/protobuf/protoc-gen-gogoslick@v1.2.1 \ github.com/gogo/protobuf/gogoproto@v1.2.1 && \ rm -rf /go/pkg /go/src -RUN curl -Ls https://github.com/golang/dep/releases/download/v0.5.0/dep-linux-amd64 -o $GOPATH/bin/dep && \ - chmod +x $GOPATH/bin/dep ENV NODE_PATH=/usr/lib/node_modules COPY build.sh / diff --git a/pkg/configs/configs.go b/pkg/configs/configs.go index 933c57cfe8f..9f9514a4fcb 100644 --- a/pkg/configs/configs.go +++ b/pkg/configs/configs.go @@ -203,10 +203,8 @@ func (c RulesConfig) parseV2Formatted() (map[string]rulefmt.RuleGroups, error) { for fn, content := range c.Files { rgs, errs := rulefmt.Parse([]byte(content)) - if errs != nil { - for _, err := range errs { - return nil, err - } + for _, err := range errs { // return just the first error, if any + return nil, err } ruleMap[fn] = *rgs diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index fcda5be3270..0043f8ef5bd 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -312,7 +312,7 @@ func (t *Cortex) initQueryFrontend(cfg *Config) (err error) { func (t *Cortex) stopQueryFrontend() (err error) { t.frontend.Close() if t.cache != nil { - t.cache.Stop() + _ = t.cache.Stop() t.cache = nil } return diff --git a/pkg/ingester/ingester_v2.go b/pkg/ingester/ingester_v2.go index 1dda52233ec..491fdfa150a 100644 --- a/pkg/ingester/ingester_v2.go +++ b/pkg/ingester/ingester_v2.go @@ -407,12 +407,14 @@ func (i *Ingester) getOrCreateTSDB(userID string, force bool) (*tsdb.DB, error) i.done.Add(1) go func() { defer i.done.Done() - runutil.Repeat(i.cfg.TSDBConfig.ShipInterval, i.quit, func() error { + if err := runutil.Repeat(i.cfg.TSDBConfig.ShipInterval, i.quit, func() error { if uploaded, err := s.Sync(context.Background()); err != nil { level.Warn(util.Logger).Log("err", err, "uploaded", uploaded) } return nil - }) + }); err != nil { + level.Warn(util.Logger).Log("err", err) + } }() i.TSDBState.dbs[userID] = db diff --git a/pkg/ring/lifecycler_test.go b/pkg/ring/lifecycler_test.go index 0938372cb11..0890af0ef88 100644 --- a/pkg/ring/lifecycler_test.go +++ b/pkg/ring/lifecycler_test.go @@ -340,7 +340,6 @@ func TestCheckReady(t *testing.T) { } type noopFlushTransferer struct { - lifecycler *Lifecycler } func (f *noopFlushTransferer) StopIncomingRequests() {} diff --git a/pkg/ring/model.go b/pkg/ring/model.go index 191f7c5c3fe..7b36a682585 100644 --- a/pkg/ring/model.go +++ b/pkg/ring/model.go @@ -171,7 +171,7 @@ func (i *IngesterDesc) IsHealthy(op Operation, heartbeatTimeout time.Duration) b healthy = true } - return healthy && time.Now().Sub(time.Unix(i.Timestamp, 0)) <= heartbeatTimeout + return healthy && time.Since(time.Unix(i.Timestamp, 0)) <= heartbeatTimeout } // Merge merges other ring into this one. Returns sub-ring that represents the change, diff --git a/pkg/ruler/rules/store_test.go b/pkg/ruler/rules/store_test.go index fbd8a9cb41e..7824222d8eb 100644 --- a/pkg/ruler/rules/store_test.go +++ b/pkg/ruler/rules/store_test.go @@ -33,7 +33,7 @@ func Test_ConfigRuleStoreError(t *testing.T) { } store := NewConfigRuleStore(mock) - _, err := store.ListAllRuleGroups(nil) + _, err := store.ListAllRuleGroups(context.Background()) assert.Equal(t, mock.err, err, "Unexpected error returned") } @@ -52,7 +52,7 @@ func Test_ConfigRuleStoreReturn(t *testing.T) { } store := NewConfigRuleStore(mock) - rules, _ := store.ListAllRuleGroups(nil) + rules, _ := store.ListAllRuleGroups(context.Background()) assert.Equal(t, 1, len(rules["user"])) assert.Equal(t, id, store.since) @@ -71,7 +71,7 @@ func Test_ConfigRuleStoreDelete(t *testing.T) { } store := NewConfigRuleStore(mock) - store.ListAllRuleGroups(nil) + _, _ = store.ListAllRuleGroups(context.Background()) mock.cfgs["user"] = configs.VersionedRulesConfig{ ID: 1, @@ -79,7 +79,7 @@ func Test_ConfigRuleStoreDelete(t *testing.T) { DeletedAt: time.Unix(0, 1), } - rules, _ := store.ListAllRuleGroups(nil) + rules, _ := store.ListAllRuleGroups(context.Background()) assert.Equal(t, 0, len(rules["user"])) } @@ -97,7 +97,7 @@ func Test_ConfigRuleStoreAppend(t *testing.T) { } store := NewConfigRuleStore(mock) - store.ListAllRuleGroups(nil) + _, _ = store.ListAllRuleGroups(context.Background()) delete(mock.cfgs, "user") mock.cfgs["user2"] = configs.VersionedRulesConfig{ @@ -106,7 +106,7 @@ func Test_ConfigRuleStoreAppend(t *testing.T) { DeletedAt: zeroTime, } - rules, _ := store.ListAllRuleGroups(nil) + rules, _ := store.ListAllRuleGroups(context.Background()) assert.Equal(t, 2, len(rules)) } @@ -134,7 +134,7 @@ func Test_ConfigRuleStoreSinceSet(t *testing.T) { } store := NewConfigRuleStore(mock) - store.ListAllRuleGroups(nil) + _, _ = store.ListAllRuleGroups(context.Background()) assert.Equal(t, configs.ID(100), store.since) delete(mock.cfgs, "user") @@ -145,7 +145,7 @@ func Test_ConfigRuleStoreSinceSet(t *testing.T) { DeletedAt: zeroTime, } - store.ListAllRuleGroups(nil) + _, _ = store.ListAllRuleGroups(context.Background()) assert.Equal(t, configs.ID(100), store.since) mock.cfgs["user2"] = configs.VersionedRulesConfig{ @@ -154,7 +154,7 @@ func Test_ConfigRuleStoreSinceSet(t *testing.T) { DeletedAt: zeroTime, } - store.ListAllRuleGroups(nil) + _, _ = store.ListAllRuleGroups(context.Background()) assert.Equal(t, configs.ID(101), store.since) } diff --git a/pkg/util/limiter/rate_limiter.go b/pkg/util/limiter/rate_limiter.go index 9677d1eca98..48fc6a42623 100644 --- a/pkg/util/limiter/rate_limiter.go +++ b/pkg/util/limiter/rate_limiter.go @@ -99,7 +99,7 @@ func (l *RateLimiter) recheckTenantLimiter(now time.Time, tenantID string) *rate l.tenantsLock.Lock() defer l.tenantsLock.Unlock() - entry, _ := l.tenants[tenantID] + entry := l.tenants[tenantID] // We check again if the recheck period elapsed, cause it may // have already been rechecked in the meanwhile. diff --git a/tools/lint b/tools/lint deleted file mode 100755 index 1dca8c2fff0..00000000000 --- a/tools/lint +++ /dev/null @@ -1,262 +0,0 @@ -#!/bin/bash -# This scipt lints files for common errors. -# -# For go files, it runs gofmt and go vet, and optionally golint and -# gocyclo, if they are installed. -# -# For shell files, it runs shfmt. If you don't have that installed, you can get -# it with: -# curl -fsSLo shfmt https://github.com/mvdan/sh/releases/download/v1.3.0/shfmt_v1.3.0_linux_amd64 -# chmod +x shfmt -# -# With no arguments, it lints the current files staged -# for git commit. Or you can pass it explicit filenames -# (or directories) and it will lint them. -# -# To use this script automatically, run: -# ln -s ../../bin/lint .git/hooks/pre-commit - -set -e - -LINT_IGNORE_FILE=${LINT_IGNORE_FILE:-".lintignore"} - -IGNORE_LINT_COMMENT= -IGNORE_SPELLINGS= -PARALLEL= -NOVET= -while true; do - case "$1" in - -nocomment) - IGNORE_LINT_COMMENT=1 - shift 1 - ;; - -notestpackage) - # NOOP, still accepted for backwards compatibility - shift 1 - ;; - -ignorespelling) - IGNORE_SPELLINGS="$2,$IGNORE_SPELLINGS" - shift 2 - ;; - -novet) - NOVET=1 - shift 1 - ;; - -p) - PARALLEL=1 - shift 1 - ;; - *) - break - ;; - esac -done - -spell_check() { - local filename="$1" - local lint_result=0 - - # misspell is completely optional. If you don't like it - # don't have it installed. - if ! type misspell >/dev/null 2>&1; then - return $lint_result - fi - - if ! misspell -error -i "$IGNORE_SPELLINGS" "${filename}"; then - lint_result=1 - fi - - return $lint_result -} - -lint_go() { - local filename="$1" - local lint_result=0 - - if [ -n "$(gofmt -s -l "${filename}")" ]; then - lint_result=1 - echo "${filename}: run gofmt -s -w ${filename}" - fi - - if [ -z "$NOVET" ]; then - # -methods=false disables checks for non-standard signatures for methods with familiar names. - # This is needed because the Prometheus storage interface requires a non-standard Seek() method. - go tool vet -methods=false "${filename}" || lint_result=$? - fi - - # golint is completely optional. If you don't like it - # don't have it installed. - if type golint >/dev/null 2>&1; then - # golint doesn't set an exit code it seems - if [ -z "$IGNORE_LINT_COMMENT" ]; then - lintoutput=$(golint "${filename}") - else - lintoutput=$(golint "${filename}" | grep -vE 'comment|dot imports|ALL_CAPS') - fi - if [ -n "$lintoutput" ]; then - lint_result=1 - echo "$lintoutput" - fi - fi - - # gocyclo is completely optional. If you don't like it - # don't have it installed. Also never blocks a commit, - # it just warns. - if type gocyclo >/dev/null 2>&1; then - gocyclo -over 25 "${filename}" | while read -r line; do - echo "${filename}": higher than 25 cyclomatic complexity - "${line}" - done - fi - - return $lint_result -} - -lint_sh() { - local filename="$1" - local lint_result=0 - - if ! diff -u "${filename}" <(shfmt -i 4 "${filename}"); then - lint_result=1 - echo "${filename}: run shfmt -i 4 -w ${filename}" - fi - - # the shellcheck is completely optional. If you don't like it - # don't have it installed. - if type shellcheck >/dev/null 2>&1; then - shellcheck "${filename}" || lint_result=1 - fi - - return $lint_result -} - -lint_tf() { - local filename="$1" - local lint_result=0 - - if ! diff -u <(hclfmt "${filename}") "${filename}"; then - lint_result=1 - echo "${filename}: run hclfmt -w ${filename}" - fi - - return $lint_result -} - -lint_md() { - local filename="$1" - local lint_result=0 - - for i in '=======' '>>>>>>>'; do - if grep -q "${i}" "${filename}"; then - lint_result=1 - echo "${filename}: bad merge/rebase!" - fi - done - - return $lint_result -} - -lint_py() { - local filename="$1" - local lint_result=0 - - if yapf --diff "${filename}" | grep -qE '^[+-]'; then - lint_result=1 - echo "${filename}: run yapf --in-place ${filename}" - else - # Only run flake8 if yapf passes, since they pick up a lot of similar issues - flake8 "${filename}" || lint_result=1 - fi - - return $lint_result -} - -lint() { - filename="$1" - ext="${filename##*\.}" - local lint_result=0 - - # Don't lint deleted files - if [ ! -f "$filename" ]; then - return - fi - - # Don't lint specific files - case "$(basename "${filename}")" in - static.go) return ;; - coverage.html) return ;; - *.pb.go) return ;; - esac - - mimetype=$(file --mime-type "${filename}" | awk '{print $2}') - - case "$mimetype.$ext" in - text/x-shellscript.*) lint_sh "${filename}" || lint_result=1 ;; - *.go) lint_go "${filename}" || lint_result=1 ;; - *.tf) lint_tf "${filename}" || lint_result=1 ;; - *.md) lint_md "${filename}" || lint_result=1 ;; - *.py) lint_py "${filename}" || lint_result=1 ;; - esac - - # we don't want to spell check tar balls, binaries, Makefile and json files - case "$mimetype.$ext" in - *.tar | *.gz | *.json) ;; - *.req | *.key | *.pem | *.crt) ;; - application/x-executable.*) ;; - text/x-makefile.*) ;; - *) spell_check "${filename}" || lint_result=1 ;; - esac - - return $lint_result -} - -lint_files() { - local lint_result=0 - while read -r filename; do - lint "${filename}" || lint_result=1 - done - exit $lint_result -} - -matches_any() { - local filename="$1" - local patterns="$2" - while read -r pattern; do - # shellcheck disable=SC2053 - # Use the [[ operator without quotes on $pattern - # in order to "glob" the provided filename: - [[ "$filename" == $pattern ]] && return 0 - done <<<"$patterns" - return 1 -} - -filter_out() { - local patterns_file="$1" - if [ -n "$patterns_file" ] && [ -r "$patterns_file" ]; then - local patterns - patterns=$(sed '/^#.*$/d ; /^\s*$/d' "$patterns_file") # Remove blank lines and comments before we start iterating. - [ -n "$DEBUG" ] && echo >&2 "> Filters:" && echo >&2 "$patterns" - local filtered_out=() - while read -r filename; do - matches_any "$filename" "$patterns" && filtered_out+=("$filename") || echo "$filename" - done - [ -n "$DEBUG" ] && echo >&2 "> Files filtered out (i.e. NOT linted):" && printf >&2 '%s\n' "${filtered_out[@]}" - else - cat # No patterns provided: simply propagate stdin to stdout. - fi -} - -list_files() { - if [ $# -gt 0 ]; then - find "$@" \( -name vendor -o -name .git \) -prune -o -type f - else - git ls-files --exclude-standard | grep -vE '(^|/)vendor/' - fi -} - -if [ $# = 1 ] && [ -f "$1" ]; then - lint "$1" -elif [ -n "$PARALLEL" ]; then - list_files "$@" | filter_out "$LINT_IGNORE_FILE" | xargs -n1 -P16 "$0" -else - list_files "$@" | filter_out "$LINT_IGNORE_FILE" | lint_files -fi diff --git a/tools/test b/tools/test index c284e49471f..cac70713fc5 100755 --- a/tools/test +++ b/tools/test @@ -100,9 +100,6 @@ fi PACKAGE_BASE=$(go list -e ./) -# Speed up the tests by compiling and installing their dependencies first. -go test -i "${GO_TEST_ARGS[@]}" "${TESTDIRS[@]}" - run_test() { local dir=$1 if [ -z "$NO_GO_GET" ]; then