From a1da65a368b3c26dbb7b2a6cfd91355593d901f2 Mon Sep 17 00:00:00 2001 From: sayboras Date: Wed, 23 Oct 2019 05:34:14 +1100 Subject: [PATCH] Enabled golangci-lint (#650) * Enable golangci-lint * Corrected typo * Update azure pipeline to run `make lint` in CI * Hope that wget works for windows * Another try with windows * Last try with .exe, pray it work * Another try * Another try :) * Last for the day :) * One more try :) * Update .gitignore * Give up on windows :( * Disabled fail fast to get all error at once * :) * Clean up * Update misspell locale --- .gitignore | 3 +- .golangci.yml | 244 ++++++++++++++++++++++++++ Makefile | 9 + build-binary-template.yml | 8 + pkg/channel/http/http_channel.go | 1 + pkg/components/bindings/loader.go | 4 +- pkg/components/pubsub/loader.go | 5 +- pkg/components/secretstores/loader.go | 9 +- pkg/diagnostics/tracing.go | 12 +- pkg/discovery/mdns.go | 2 +- pkg/http/api.go | 5 +- pkg/injector/pod_patch.go | 3 +- pkg/placement/consistent_hash.go | 5 +- pkg/placement/placement.go | 3 +- pkg/runtime/runtime.go | 7 +- 15 files changed, 284 insertions(+), 36 deletions(-) create mode 100644 .golangci.yml diff --git a/.gitignore b/.gitignore index 3e22129247a..56839acf544 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ -/dist \ No newline at end of file +/dist +.idea \ No newline at end of file diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000000..c66e70aa52a --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,244 @@ +# options for analysis running +run: + # default concurrency is a available CPU number + concurrency: 4 + + # timeout for analysis, e.g. 30s, 5m, default is 1m + deadline: 5m + + # exit code when at least one issue was found, default is 1 + issues-exit-code: 1 + + # include test files or not, default is true + tests: false + + # list of build tags, all linters use it. Default is empty list. + #build-tags: + # - mytag + + # which dirs to skip: they won't be analyzed; + # can use regexp here: generated.*, regexp is applied on full path; + # default value is empty list, but next dirs are always skipped independently + # from this option's value: + # vendor$, third_party$, testdata$, examples$, Godeps$, builtin$ + skip-dirs: + - ^vendor$ + - ^pkg.*client.*clientset.*versioned.* + - ^pkg.*client.*informers.*externalversions.* + + # which files to skip: they will be analyzed, but issues from them + # won't be reported. Default value is empty list, but there is + # no need to include all autogenerated files, we confidently recognize + # autogenerated files. If it's not please let us know. + skip-files: + # - ".*\\.my\\.go$" + # - lib/bad.go + - pkg.*version.*version.go + - pkg.*models.*models.go + - pkg.*config.*modes.*kubernetes_config.go + +# output configuration options +output: + # colored-line-number|line-number|json|tab|checkstyle, default is "colored-line-number" + format: tab + + # print lines of code with issue, default is true + print-issued-lines: true + + # print linter name in the end of issue text, default is true + print-linter-name: true + + +# all available settings of specific linters +linters-settings: + errcheck: + # report about not checking of errors in type assetions: `a := b.(MyStruct)`; + # default is false: such cases aren't reported by default. + check-type-assertions: false + + # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; + # default is false: such cases aren't reported by default. + check-blank: false + + # [deprecated] comma-separated list of pairs of the form pkg:regex + # the regex is used to ignore names within pkg. (default "fmt:.*"). + # see https://github.com/kisielk/errcheck#the-deprecated-method for details + ignore: fmt:.*,io/ioutil:^Read.* + + # path to a file containing a list of functions to exclude from checking + # see https://github.com/kisielk/errcheck#excluding-functions for details + exclude: + + funlen: + lines: 60 + statements: 40 + + govet: + # report about shadowed variables + check-shadowing: true + + # settings per analyzer + settings: + printf: # analyzer name, run `go tool vet help` to see all analyzers + funcs: # run `go tool vet help printf` to see available settings for `printf` analyzer + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Infof + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf + - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf + + # enable or disable analyzers by name + enable: + - atomicalign + enable-all: false + disable: + - shadow + disable-all: false + golint: + # minimal confidence for issues, default is 0.8 + min-confidence: 0.8 + gofmt: + # simplify code: gofmt with `-s` option, true by default + simplify: true + goimports: + # put imports beginning with prefix after 3rd-party packages; + # it's a comma-separated list of prefixes + local-prefixes: github.com/dapr/dapr + gocyclo: + # minimal code complexity to report, 30 by default (but we recommend 10-20) + min-complexity: 10 + gocognit: + # minimal code complexity to report, 30 by default (but we recommend 10-20) + min-complexity: 10 + maligned: + # print struct with more effective memory layout or not, false by default + suggest-new: true + dupl: + # tokens count to trigger issue, 150 by default + threshold: 100 + goconst: + # minimal length of string constant, 3 by default + min-len: 3 + # minimal occurrences count to trigger, 3 by default + min-occurrences: 3 + depguard: + list-type: blacklist + include-go-root: false + packages: + - github.com/sirupsen/logrus + packages-with-error-messages: + # specify an error message to output when a blacklisted package is used + github.com/sirupsen/logrus: "logging is allowed only by logutils.Log" + misspell: + # Correct spellings using locale preferences for US or UK. + # Default is to use a neutral variety of English. + # Setting locale to US will correct the British spelling of 'colour' to 'color'. + locale: default + ignore-words: + - someword + lll: + # max line length, lines longer will be reported. Default is 120. + # '\t' is counted as 1 character by default, and can be changed with the tab-width option + line-length: 120 + # tab width in spaces. Default to 1. + tab-width: 1 + unused: + # treat code as a program (not a library) and report unused exported identifiers; default is false. + # XXX: if you enable this setting, unused will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find funcs usages. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + unparam: + # Inspect exported functions, default is false. Set to true if no external program/library imports your code. + # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find external interfaces. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + nakedret: + # make an issue if func has more lines of code than this setting and it has naked returns; default is 30 + max-func-lines: 30 + prealloc: + # XXX: we don't recommend using this linter before doing performance profiling. + # For most programs usage of prealloc will be a premature optimization. + + # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them. + # True by default. + simple: true + range-loops: true # Report preallocation suggestions on range loops, true by default + for-loops: false # Report preallocation suggestions on for loops, false by default + gocritic: + # Which checks should be enabled; can't be combined with 'disabled-checks'; + # See https://go-critic.github.io/overview#checks-overview + # To check which checks are enabled run `GL_DEBUG=gocritic golangci-lint run` + # By default list of stable checks is used. + enabled-checks: + + # Which checks should be disabled; can't be combined with 'enabled-checks'; default is empty + disabled-checks: + - regexpMust + - rangeValCopy + - hugeParam + - ifElseChain + - singleCaseSwitch + + # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint run` to see all tags and checks. + # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". + enabled-tags: + - performance + + settings: # settings passed to gocritic + captLocal: # must be valid enabled check name + paramsOnly: true + godox: + # report any comments starting with keywords, this is useful for TODO or FIXME comments that + # might be left in the code accidentally and should be resolved before merging + keywords: # default keywords are TODO, BUG, and FIXME, these can be overwritten by this setting + - NOTE + - OPTIMIZE # marks code that should be optimized before merging + - HACK # marks hack-arounds that should be removed before merging + dogsled: + # checks assignments with too many blank identifiers; default is 2 + max-blank-identifiers: 2 + + whitespace: + multi-if: false # Enforces newlines (or comments) after every multi-line if statement + multi-func: false # Enforces newlines (or comments) after every multi-line function signature + + wsl: + # If true append is only allowed to be cuddled if appending value is + # matching variables, fields or types on line above. Default is true. + strict-append: true + # Allow calls and assignments to be cuddled as long as the lines have any + # matching variables, fields or types. Default is true. + allow-assign-and-call: true + # Allow multiline assignments to be cuddled. Default is true. + allow-multiline-assign: true + # Allow case blocks to end with a whitespace. + allow-case-traling-whitespace: true + # Allow declarations (var) to be cuddled. + allow-cuddle-declarations: false + +linters: + fast: false + enable-all: true + disable: + # TODO Enforce the below linters later + - deadcode + - dupl + - errcheck + - funlen + - gochecknoglobals + - gochecknoinits + - gocyclo + - gocognit + - godox + - golint + # TODO enable gofmt and goimports again once github action is done. Now both failed in windows only. + - gofmt + - goimports + - interfacer + - lll + - maligned + - scopelint + - unparam + - unused + - wsl \ No newline at end of file diff --git a/Makefile b/Makefile index 31d3fdb5920..eab37d821e2 100644 --- a/Makefile +++ b/Makefile @@ -56,9 +56,11 @@ export GOOS ?= $(TARGET_OS_LOCAL) ifeq ($(GOOS),windows) BINARY_EXT_LOCAL:=.exe +GOLANGCI_LINT:=golangci-lint.exe export ARCHIVE_EXT = .zip else BINARY_EXT_LOCAL:= +GOLANGCI_LINT:=golangci-lint export ARCHIVE_EXT = .tar.gz endif @@ -239,3 +241,10 @@ release: build archive .PHONY: test test: go test ./pkg/... -mod=vendor + +################################################################################ +# Target: lint # +################################################################################ +.PHONY: lint +lint: + $(GOLANGCI_LINT) run diff --git a/build-binary-template.yml b/build-binary-template.yml index e7f045a462a..4ff7154c587 100644 --- a/build-binary-template.yml +++ b/build-binary-template.yml @@ -31,6 +31,14 @@ jobs: shopt -s extglob mv !(gopath) '$(modulePath)' echo '##vso[task.prependpath]$(GOBIN)' + curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b '$(GOPATH)/bin' v1.21.0 + - task: CmdLine@2 + displayName: 'Run make lint' + condition: ne('${{ parameters.targetArch }}', 'arm') + inputs: + script: 'make lint' + workingDirectory: '$(modulePath)' + failOnStderr: false - task: CmdLine@2 displayName: 'Run make test' condition: ne('${{ parameters.targetArch }}', 'arm') diff --git a/pkg/channel/http/http_channel.go b/pkg/channel/http/http_channel.go index dcee4f52c28..71d07144d3b 100644 --- a/pkg/channel/http/http_channel.go +++ b/pkg/channel/http/http_channel.go @@ -117,6 +117,7 @@ func (h *Channel) InvokeMethod(invokeRequest *channel.InvokeRequest) (*channel.I } // CreateLocalChannel creates an HTTP AppChannel +// nolint:gosec func CreateLocalChannel(port, maxConcurrency int) (channel.AppChannel, error) { c := &Channel{ client: &fasthttp.Client{MaxConnsPerHost: 1000000, TLSConfig: &tls.Config{InsecureSkipVerify: true}, ReadTimeout: time.Second * 60, MaxIdemponentCallAttempts: 0}, diff --git a/pkg/components/bindings/loader.go b/pkg/components/bindings/loader.go index a998b7442ec..250dec12b33 100644 --- a/pkg/components/bindings/loader.go +++ b/pkg/components/bindings/loader.go @@ -93,7 +93,5 @@ func Load() { RegisterOutputBinding("gcp.pubsub", func() bindings.OutputBinding { return pubsub.NewGCPPubSub() }) - RegisterInputBinding("kubernetes", func() bindings.InputBinding { - return kubernetes.NewKubernetes() - }) + RegisterInputBinding("kubernetes", kubernetes.NewKubernetes) } diff --git a/pkg/components/pubsub/loader.go b/pkg/components/pubsub/loader.go index c7169683cbb..8b4aeaf29da 100644 --- a/pkg/components/pubsub/loader.go +++ b/pkg/components/pubsub/loader.go @@ -6,13 +6,10 @@ package pubsub import ( - "github.com/dapr/components-contrib/pubsub" "github.com/dapr/components-contrib/pubsub/redis" ) // Load message buses func Load() { - RegisterMessageBus("redis", func() pubsub.PubSub { - return redis.NewRedisStreams() - }) + RegisterMessageBus("redis", redis.NewRedisStreams) } diff --git a/pkg/components/secretstores/loader.go b/pkg/components/secretstores/loader.go index 832e426e1d5..9ea080a7d42 100644 --- a/pkg/components/secretstores/loader.go +++ b/pkg/components/secretstores/loader.go @@ -6,17 +6,12 @@ package secretstores import ( - "github.com/dapr/components-contrib/secretstores" "github.com/dapr/components-contrib/secretstores/keyvault" "github.com/dapr/components-contrib/secretstores/kubernetes" ) // Load secret stores func Load() { - RegisterSecretStore("kubernetes", func() secretstores.SecretStore { - return kubernetes.NewKubernetesSecretStore() - }) - RegisterSecretStore("azure.keyvault", func() secretstores.SecretStore { - return keyvault.NewAzureKeyvaultSecretStore() - }) + RegisterSecretStore("kubernetes", kubernetes.NewKubernetesSecretStore) + RegisterSecretStore("azure.keyvault", keyvault.NewAzureKeyvaultSecretStore) } diff --git a/pkg/diagnostics/tracing.go b/pkg/diagnostics/tracing.go index e7bcfb8a896..28a7528a119 100644 --- a/pkg/diagnostics/tracing.go +++ b/pkg/diagnostics/tracing.go @@ -49,8 +49,8 @@ func DeserializeSpanContext(ctx string) trace.SpanContext { traceID, _ := hex.DecodeString(parts[1]) traceOptions, _ := strconv.ParseUint(parts[2], 10, 32) ret := trace.SpanContext{} - copy(ret.SpanID[:], spanID[:]) - copy(ret.TraceID[:], traceID[:]) + copy(ret.SpanID[:], spanID) + copy(ret.TraceID[:], traceID) ret.TraceOptions = trace.TraceOptions(traceOptions) return ret } @@ -199,13 +199,13 @@ func addAnnotationsFromMD(md metautils.NiceMD, span *trace.Span, expandParams bo if expandParams { for k, vv := range md { for _, v := range vv { - span.AddAttributes(trace.StringAttribute(string(k), v)) + span.AddAttributes(trace.StringAttribute(k, v)) } } } - if includeBody { - //TODO: get request body? - } + //TODO: get request body? + //if includeBody { + //} } func projectStatusCode(code int) int32 { diff --git a/pkg/discovery/mdns.go b/pkg/discovery/mdns.go index 94f9f69e6d3..084e4abf53d 100644 --- a/pkg/discovery/mdns.go +++ b/pkg/discovery/mdns.go @@ -63,7 +63,7 @@ func LookupPortMDNS(id string) (int, error) { err = resolver.Browse(ctx, id, "local.", entries) if err != nil { - return -1, fmt.Errorf("Failed to browse: %s", err.Error()) + return -1, fmt.Errorf("failed to browse: %s", err.Error()) } <-ctx.Done() diff --git a/pkg/http/api.go b/pkg/http/api.go index df84dfb22be..a2423649dc2 100644 --- a/pkg/http/api.go +++ b/pkg/http/api.go @@ -110,7 +110,7 @@ func (a *api) constructStateEndpoints() []Endpoint { func (a *api) constructPubSubEndpoints() []Endpoint { return []Endpoint{ - Endpoint{ + { Methods: []string{http.Post, http.Put}, Route: "publish/", Version: apiVersionV1, @@ -121,7 +121,7 @@ func (a *api) constructPubSubEndpoints() []Endpoint { func (a *api) constructBindingsEndpoints() []Endpoint { return []Endpoint{ - Endpoint{ + { Methods: []string{http.Post, http.Put}, Route: "bindings/", Version: apiVersionV1, @@ -234,7 +234,6 @@ func (a *api) onOutputBindingMessage(c *routing.Context) error { msg := NewErrorResponse("ERR_INVOKE_OUTPUT_BINDING", fmt.Sprintf("can't deserialize request data field: %s", err)) respondWithError(c.RequestCtx, 500, msg) return nil - } err = a.sendToOutputBindingFn(name, &bindings.WriteRequest{ Metadata: req.Metadata, diff --git a/pkg/injector/pod_patch.go b/pkg/injector/pod_patch.go index 83955a1a469..a854b0cf20e 100644 --- a/pkg/injector/pod_patch.go +++ b/pkg/injector/pod_patch.go @@ -36,7 +36,8 @@ const ( kubernetesMountPath = "/var/run/secrets/kubernetes.io/serviceaccount" ) -func (i *injector) getPodPatchOperations(ar *v1beta1.AdmissionReview, namespace, image string) ([]PatchOperation, error) { +func (i *injector) getPodPatchOperations(ar *v1beta1.AdmissionReview, + namespace, image string) ([]PatchOperation, error) { req := ar.Request var pod corev1.Pod if err := json.Unmarshal(req.Object.Raw, &pod); err != nil { diff --git a/pkg/placement/consistent_hash.go b/pkg/placement/consistent_hash.go index 1523d55e117..3684fc6f960 100644 --- a/pkg/placement/consistent_hash.go +++ b/pkg/placement/consistent_hash.go @@ -111,10 +111,7 @@ func (c *Consistent) Add(host string, port int64) bool { } // sort hashes ascendingly sort.Slice(c.sortedSet, func(i int, j int) bool { - if c.sortedSet[i] < c.sortedSet[j] { - return true - } - return false + return c.sortedSet[i] < c.sortedSet[j] }) return false diff --git a/pkg/placement/placement.go b/pkg/placement/placement.go index 057b775e3dc..862b06a08be 100644 --- a/pkg/placement/placement.go +++ b/pkg/placement/placement.go @@ -96,7 +96,8 @@ func (p *PlacementService) RemoveHost(srv daprinternal_pb.PlacementService_Repor // PerformTablesUpdate updates the connected dapr runtimes using a 3 stage commit. first it locks so no further dapr can be taken // it then proceeds to update and then unlock once all runtimes have been updated -func (p *PlacementService) PerformTablesUpdate(hosts []daprinternal_pb.PlacementService_ReportDaprStatusServer, options placementOptions) { +func (p *PlacementService) PerformTablesUpdate(hosts []daprinternal_pb.PlacementService_ReportDaprStatusServer, + options placementOptions) { p.updateLock.Lock() defer p.updateLock.Unlock() diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 5d877e77416..fd8893f56a3 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -335,7 +335,7 @@ func (a *DaprRuntime) sendBindingEventToApp(bindingName string, data []byte, met return fmt.Errorf("error invoking app: %s", err) } if resp != nil { - response := &bindings.AppResponse{} + response = &bindings.AppResponse{} response.Concurrency = resp.Concurrency response.To = resp.To @@ -357,7 +357,6 @@ func (a *DaprRuntime) sendBindingEventToApp(bindingName string, data []byte, met }) } } - } else if a.runtimeConfig.ApplicationProtocol == HTTPProtocol { req := channel.InvokeRequest{ Metadata: metadata, @@ -839,7 +838,6 @@ func (a *DaprRuntime) loadAppConfiguration() { a.appConfig = *appConfig log.Info("application configuration loaded") } - return } func (a *DaprRuntime) getConfigurationHTTP() (*config.ApplicationConfig, error) { @@ -901,7 +899,6 @@ func (a *DaprRuntime) announceSelf() error { } log.Info("local service entry announced") } - return nil } @@ -923,7 +920,7 @@ func (a *DaprRuntime) initSecretStores() error { // Initialize all secretstore components for _, c := range a.components { - if strings.Index(c.Spec.Type, "secretstores") < 0 { + if !strings.Contains(c.Spec.Type, "secretstores") { continue }