fix findings from staticcheck (#897)

* fix most issues from running staticcheck

Signed-off-by: Sandor Szücs <[email protected]>

* fix some low hanging fruits

Signed-off-by: Sandor Szücs <[email protected]>

* use tests in statickcheck, because of dependendencies that would not been found if dropped

Signed-off-by: Sandor Szücs <[email protected]>

* fix the rest of the missing staticcheck violations and add some checks to ignore

Signed-off-by: Sandor Szücs <[email protected]>

* remove leftovers from broken workaround

Signed-off-by: Sandor Szücs <[email protected]>

* add staticcheck to build via CDP

Signed-off-by: Sandor Szücs <[email protected]>

* fix additional staticcheck errors and reduce the ignored checkers

Signed-off-by: Sandor Szücs <[email protected]>

* remove comment

Signed-off-by: Sandor Szücs <[email protected]>

* use setRequestHeader() instead of appendRequestHeader for the Host headers

* more linting fixes:
- unnecessary type conversion
- uint never less than 0

* fix linting after merge from master

* install statickcheck in deps target to GOBIN and activate in travis CI trigger with precommit and check-precommit targets

Signed-off-by: Sandor Szücs <[email protected]>

* use GOPATH/bin and check vet first, then staticcheck and last run the tests to speeup the fail fast

Signed-off-by: Sandor Szücs <[email protected]>

* make staticcheck executable

Signed-off-by: Sandor Szücs <[email protected]>

* create GOPATH/bin and to debug output GOENV

Signed-off-by: Sandor Szücs <[email protected]>

* debug output GOENV

Signed-off-by: Sandor Szücs <[email protected]>

Makefile

@@ -73,8 +73,7 @@ bench: build $(TEST_PLUGINS)
 	#
 	for p in $(PACKAGES); do GO111MODULE=on go test -bench . $$p; done
 
-lint: build
-	gometalinter --enable-all --deadline=60s ./... | tee linter.log
+lint: build staticcheck
 
 clean:
 	go clean -i -cache -testcache ./...
@@ -83,20 +82,33 @@ clean:
 	rm -f ./_test_plugins_fail/*.so
 
 deps:
+	go env
 	./etcd/install.sh $(TEST_ETCD_VERSION)
+	@curl -o /tmp/staticcheck -LO https://github.com/dominikh/go-tools/releases/download/2019.1/staticcheck_linux_amd64
+	@sha256sum /tmp/staticcheck | grep -q a13563b3fe136674a87e174bbedbd1af49e5bd89ffa605a11150ae06ab9fd999
+	@mkdir -p $(GOPATH)/bin
+	@mv /tmp/staticcheck $(GOPATH)/bin/
+	@chmod +x $(GOPATH)/bin/staticcheck
 
 vet: $(SOURCES)
 	GO111MODULE=on go vet $(PACKAGES)
 
+# TODO(sszuecs) review disabling these checks, f.e.:
+# -ST1000 missing package doc in many packages
+# -ST1003 wrong naming convention Api vs API, Id vs ID
+# -ST1012 too many error variables are not having prefix "err"
+staticcheck: $(SOURCES)
+	GO111MODULE=on staticcheck -checks "all,-ST1000,-ST1003,-ST1012" $(PACKAGES)
+
 fmt: $(SOURCES)
 	@gofmt -w -s $(SOURCES)
 
 check-fmt: $(SOURCES)
 	@if [ "$$(gofmt -d $(SOURCES))" != "" ]; then false; else true; fi
 
-precommit: fmt build shortcheck vet
+precommit: fmt build vet staticcheck shortcheck
 
-check-precommit: check-fmt build shortcheck vet
+check-precommit: check-fmt build vet staticcheck shortcheck
 
 .coverprofile-all: $(SOURCES) $(TEST_PLUGINS)
 	# go list -f \

circuit/breaker.go

@@ -74,6 +74,8 @@ func (to BreakerSettings) mergeSettings(from BreakerSettings) BreakerSettings {
 }
 
 // String returns the string representation of a particular set of settings.
+//
+//lint:ignore ST1016 "s" makes sense here and mergeSettings has "to"
 func (s BreakerSettings) String() string {
 	var ss []string
 

cmd/eskip/args.go

@@ -211,7 +211,7 @@ func processFileArg() (*medium, error) {
 
 // if pretty print then check that indent matches pattern
 func processIndentStr() error {
-	if pretty && !(regexp.MustCompile("^[\\s]*$").MatchString(indentStr)) {
+	if pretty && !(regexp.MustCompile(`^[\s]*$`).MatchString(indentStr)) {
 		return invalidIndentStr
 	}
 	return nil

cmd/eskip/media.go

@@ -25,8 +25,7 @@ type (
 )
 
 const (
-	none mediaType = iota
-	stdin
+	stdin mediaType = 1 + iota
 	file
 	etcd
 	innkeeper

cmd/skipper/main.go

@@ -443,7 +443,7 @@ func init() {
 	flag.DurationVar(&readTimeoutServer, "read-timeout-server", defaultReadTimeoutServer, readTimeoutServerUsage)
 	flag.DurationVar(&readHeaderTimeoutServer, "read-header-timeout-server", defaultReadHeaderTimeoutServer, readHeaderTimeoutServerUsage)
 	flag.DurationVar(&writeTimeoutServer, "write-timeout-server", defaultWriteTimeoutServer, writeTimeoutServerUsage)
-	flag.DurationVar(&idleTimeoutServer, "idle-timeout-server", defaultIdleTimeoutServer, idleConnsPerHostUsage)
+	flag.DurationVar(&idleTimeoutServer, "idle-timeout-server", defaultIdleTimeoutServer, idleTimeoutServerUsage)
 	flag.IntVar(&maxHeaderBytes, "max-header-bytes", http.DefaultMaxHeaderBytes, maxHeaderBytesUsage)
 	flag.BoolVar(&enableConnMetricsServer, "enable-connection-metrics", false, enableConnMetricsServerUsage)
 	flag.DurationVar(&timeoutBackend, "timeout-backend", defaultTimeoutBackend, timeoutBackendUsage)

dataclients/kubernetes/kube.go

@@ -194,7 +194,7 @@ type Client struct {
 	eastWestDomainFmt        string
 }
 
-var nonWord = regexp.MustCompile("\\W")
+var nonWord = regexp.MustCompile(`\W`)
 
 var (
 	errServiceNotFound      = errors.New("service not found")

dataclients/kubernetes/kube_test.go

@@ -955,7 +955,7 @@ func TestIngress(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 		if err != nil {
 			t.Error("failed to load initial routes", err)
 			return
@@ -1356,7 +1356,7 @@ func TestConvertPathRule(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 
 		if err != nil {
 			t.Error("failed to load initial routes", err)
@@ -1419,7 +1419,7 @@ func TestConvertPathRule(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 
 		if err != nil {
 			t.Error("failed to load initial routes", err)
@@ -1494,7 +1494,7 @@ func TestConvertPathRule(t *testing.T) {
 			},
 		)
 
-		r, _, err = dc.LoadUpdate()
+		r, _, err := dc.LoadUpdate()
 		if err != nil {
 			t.Errorf("update failed: %v", err)
 		}
@@ -1524,7 +1524,7 @@ func TestConvertPathRuleEastWestEnabled(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 
 		if err != nil {
 			t.Error("failed to load initial routes", err)
@@ -1577,7 +1577,7 @@ func TestConvertPathRuleEastWestEnabled(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 
 		if err != nil {
 			t.Error("failed to load initial routes", err)
@@ -1646,7 +1646,7 @@ func TestConvertPathRuleEastWestEnabled(t *testing.T) {
 
 		defer dc.Close()
 
-		r, err := dc.LoadAll()
+		_, err = dc.LoadAll()
 
 		if err != nil {
 			t.Error("failed to load initial routes", err)
@@ -1721,7 +1721,7 @@ func TestConvertPathRuleEastWestEnabled(t *testing.T) {
 			},
 		)
 
-		r, _, err = dc.LoadUpdate()
+		r, _, err := dc.LoadUpdate()
 		if err != nil {
 			t.Errorf("update failed: %v", err)
 		}
@@ -2127,18 +2127,18 @@ func TestCreateRequest(t *testing.T) {
 	client := &Client{}
 
 	url = "A%"
-	req, err = client.createRequest("GET", url, rc)
+	_, err = client.createRequest("GET", url, rc)
 	if err == nil {
 		t.Error("request creation should fail")
 	}
 
 	url = "https://www.example.org"
-	req, err = client.createRequest("GET", url, rc)
+	_, err = client.createRequest("GET", url, rc)
 	if err != nil {
 		t.Error(err)
 	}
 
-	req, err = client.createRequest("//", url, rc)
+	_, err = client.createRequest("//", url, rc)
 	if err == nil {
 		t.Error("request creation should fail")
 	}
@@ -2225,12 +2225,12 @@ func TestBuildHTTPClient(t *testing.T) {
 		t.Errorf("should return default client if outside the cluster``")
 	}
 
-	httpClient, err = buildHTTPClient("rumplestilzchen", true, quit)
+	_, err = buildHTTPClient("rumplestilzchen", true, quit)
 	if err == nil {
 		t.Errorf("expected to fail for non-existing file")
 	}
 
-	httpClient, err = buildHTTPClient("kube_test.go", true, quit)
+	_, err = buildHTTPClient("kube_test.go", true, quit)
 	if err != errInvalidCertificate {
 		t.Errorf("should return invalid certificate")
 	}
@@ -2253,7 +2253,7 @@ func TestBuildHTTPClient(t *testing.T) {
 	}
 	defer os.Remove("ca.temp.crt")
 
-	httpClient, err = buildHTTPClient("ca.temp.crt", true, quit)
+	_, err = buildHTTPClient("ca.temp.crt", true, quit)
 	if err != nil {
 		t.Error(err)
 	}

delivery.yaml

@@ -13,7 +13,7 @@ pipeline:
         IMAGE="registry-write.opensource.zalan.do/pathfinder/skipper-test:${CDP_BUILD_VERSION}"
       fi
       export IMAGE
-      make deps shortcheck
+      make deps shortcheck staticcheck
       git status
       git diff
       cd packaging && make docker-build && git status && git diff && make docker-push

eskip/eskip.go

@@ -380,7 +380,7 @@ func ParsePredicates(p string) ([]*Predicate, error) {
 
 const randomIdLength = 16
 
-var routeIdRx = regexp.MustCompile("\\W")
+var routeIdRx = regexp.MustCompile(`\W`)
 
 // generate weak random id for a route if
 // it doesn't have one.
@@ -390,7 +390,11 @@ func GenerateIfNeeded(existingId string) string {
 	}
 
 	// using this to avoid adding a new dependency.
-	id, err := flowid.NewFlowId(randomIdLength)
+	g, err := flowid.NewStandardGenerator(randomIdLength)
+	if err != nil {
+		return existingId
+	}
+	id, err := g.Generate()
 	if err != nil {
 		return existingId
 	}

eskip/lexer.go

@@ -1,17 +1,3 @@
-// Copyright 2015 Zalando SE
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
 package eskip
 
 import (
@@ -42,7 +28,6 @@ type eskipLex struct {
 	err           error
 	initialLength int
 	routes        []*parsedRoute
-	filters       []*Filter
 }
 
 type fixedScanner string

eskip/parser.go

@@ -178,8 +178,8 @@ type eskipParserImpl struct {
 	char  int
 }
 
-func (p *eskipParserImpl) Lookahead() int {
-	return p.char
+func (eskiprcvr *eskipParserImpl) Lookahead() int {
+	return eskiprcvr.char
 }
 
 func eskipNewParser() eskipParser {
@@ -567,7 +567,7 @@ eskipdefault:
 			eskipVAL.matchers = append(eskipVAL.matchers, eskipDollar[3].matcher)
 		}
 	case 13:
-		eskipDollar = eskipS[eskippt-1 : eskippt+1]
+		_ = eskipS[eskippt-1 : eskippt+1]
 		//line parser.y:131
 		{
 			eskipVAL.matcher = &matcher{"*", nil}
@@ -641,13 +641,13 @@ eskipdefault:
 			eskipVAL.loopback = false
 		}
 	case 25:
-		eskipDollar = eskipS[eskippt-1 : eskippt+1]
+		_ = eskipS[eskippt-1 : eskippt+1]
 		//line parser.y:189
 		{
 			eskipVAL.shunt = true
 		}
 	case 26:
-		eskipDollar = eskipS[eskippt-1 : eskippt+1]
+		_ = eskipS[eskippt-1 : eskippt+1]
 		//line parser.y:193
 		{
 			eskipVAL.loopback = true

eskip/string_test.go

@@ -195,7 +195,7 @@ func TestParseAndStringAndParse(t *testing.T) {
 		`route2: Path("/some/path") -> "https://www.example.org";`
 	doc = testDoc(t, doc)
 	doc = testDoc(t, doc)
-	doc = testDoc(t, doc)
+	_ = testDoc(t, doc)
 }
 
 func TestNumberString(t *testing.T) {

eskip/template.go

@@ -9,7 +9,7 @@ import (
 	"strings"
 )
 
-var parameterRegexp = regexp.MustCompile("\\$\\{(\\w+)\\}")
+var parameterRegexp = regexp.MustCompile(`\$\{(\w+)\}`)
 
 // TemplateGetter functions return the value for a template parameter name.
 type TemplateGetter func(string) string

etcd/etcd.go

@@ -1,17 +1,3 @@
-// Copyright 2015 Zalando SE
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
 /*
 Package etcd implements a DataClient for reading the skipper route
 definitions from an etcd service.
@@ -35,8 +21,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	log "github.com/sirupsen/logrus"
-	"github.com/zalando/skipper/eskip"
 	"io"
 	"io/ioutil"
 	"net"
@@ -45,6 +29,9 @@ import (
 	"path"
 	"strconv"
 	"time"
+
+	log "github.com/sirupsen/logrus"
+	"github.com/zalando/skipper/eskip"
 )
 
 const (
@@ -136,7 +123,6 @@ var (
 	invalidNode             = errors.New("invalid node")
 	unexpectedHttpResponse  = errors.New("unexpected http response")
 	notFound                = errors.New("not found")
-	missingEtcdIndex        = errors.New("missing etcd index")
 	invalidResponseDocument = errors.New("invalid response document")
 )
 
@@ -248,17 +234,17 @@ func parseResponse(rsp *http.Response) (*response, error) {
 }
 
 // Converts a non-success http status code into an in-memory error object.
-// As the second argument, returns true in case of error.
-func httpError(code int) (error, bool) {
+// As the first argument, returns true in case of error.
+func httpError(code int) (bool, error) {
 	if code == http.StatusNotFound {
-		return notFound, true
+		return true, notFound
 	}
 
 	if code < http.StatusOK || code >= http.StatusMultipleChoices {
-		return unexpectedHttpResponse, true
+		return true, unexpectedHttpResponse
 	}
 
-	return nil, false
+	return false, nil
 }
 
 // Makes a request to an available etcd endpoint, with retries in case of
@@ -296,7 +282,7 @@ func (c *Client) etcdRequest(method, path, data string) (*response, error) {
 
 	defer rsp.Body.Close()
 
-	if err, hasErr := httpError(rsp.StatusCode); hasErr {
+	if hasErr, err := httpError(rsp.StatusCode); hasErr {
 		return nil, err
 	}