Chore: Implement revive (grafana#16200)

Since we do not like some of the default golint rules,
this commit proposes to use https://github.com/mgechev/revive.

And potential revive speed-up should't hurt :).

Right now, presented config (./conf/revive.toml) is permissive,
we might improve it over time however. Fixes for found revive
issues in the code are very limited so it wouldn't be large to review.

Also in this commit:

* Add annotations for makefile commands and declare phony targets

* Rename "gometalinter" script and CI command to "lint"
  since we are doing there a bit more then using gometalinter package

* Add Makefile rules to .editorconfig

* Documentation which mentioned "golint" replaced with revive

Fixes grafana#16109
Ref grafana#16160

Author: markelog

.circleci/config.yml

@@ -86,7 +86,7 @@ jobs:
           name: check documentation spelling errors
           command: 'codespell -I ./words_to_ignore.txt docs/'
 
-  gometalinter:
+  backend-lint:
     docker:
       - image: circleci/golang:1.11.5
         environment:
@@ -96,8 +96,8 @@ jobs:
     steps:
       - checkout
       - run:
-          name: Gometalinter tests
-          command: './scripts/gometalinter.sh'
+          name: backend lint
+          command: './scripts/backend-lint.sh'
 
   test-frontend:
     docker:
@@ -460,7 +460,7 @@ workflows:
           filters: *filter-only-master
       - codespell:
           filters: *filter-only-master
-      - gometalinter:
+      - backend-lint:
           filters: *filter-only-master
       - test-frontend:
           filters: *filter-only-master
@@ -476,7 +476,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-master
@@ -487,7 +487,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-master
@@ -497,7 +497,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
             - build-all-enterprise
@@ -511,7 +511,7 @@ workflows:
           filters: *filter-only-release
       - codespell:
           filters: *filter-only-release
-      - gometalinter:
+      - backend-lint:
           filters: *filter-only-release
       - test-frontend:
           filters: *filter-only-release
@@ -527,7 +527,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -538,7 +538,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -549,7 +549,7 @@ workflows:
             - test-backend
             - test-frontend
             - codespell
-            - gometalinter
+            - backend-lint
             - mysql-integration-test
             - postgres-integration-test
           filters: *filter-only-release
@@ -560,7 +560,7 @@ workflows:
             filters: *filter-not-release-or-master
         - codespell:
             filters: *filter-not-release-or-master
-        - gometalinter:
+        - backend-lint:
             filters: *filter-not-release-or-master
         - test-frontend:
             filters: *filter-not-release-or-master
@@ -578,7 +578,7 @@ workflows:
               - test-backend
               - test-frontend
               - codespell
-              - gometalinter
+              - backend-lint
               - mysql-integration-test
               - postgres-integration-test
               - cache-server-test
@@ -589,7 +589,7 @@ workflows:
               - test-backend
               - test-frontend
               - codespell
-              - gometalinter
+              - backend-lint
               - mysql-integration-test
               - postgres-integration-test
               - cache-server-test

.editorconfig

@@ -18,3 +18,7 @@ insert_final_newline = true
 
 [*.md]
 trim_trailing_whitespace = false
+
+[Makefile]
+indent_style = tab
+indent_size = 2

Makefile

@@ -1,5 +1,7 @@
 -include local/Makefile
 
+.PHONY: all deps-go deps-js deps build-go build-server build-cli build-js build build-docker-dev build-docker-full lint-go test-go test-js test run clean
+
 all: deps build
 
 deps-go:
@@ -10,42 +12,57 @@ deps-js: node_modules
 deps: deps-js
 
 build-go:
+	@echo "build go files"
 	go run build.go build
 
 build-server:
+	@echo "build server"
 	go run build.go build-server
 
 build-cli:
+	@echo "build in CI environment"
 	go run build.go build-cli
 
 build-js:
+	@echo "build frontend"
 	yarn run build
 
 build: build-go build-js
 
 build-docker-dev:
+	@echo "build development container"
 	@echo "\033[92mInfo:\033[0m the frontend code is expected to be built already."
 	go run build.go -goos linux -pkg-arch amd64 ${OPT} build pkg-archive latest
 	cp dist/grafana-latest.linux-x64.tar.gz packaging/docker
 	cd packaging/docker && docker build --tag grafana/grafana:dev .
 
 build-docker-full:
+	@echo "build docker container"
 	docker build --tag grafana/grafana:dev .
 
+lint-go:
+	@echo "lint go source"
+	scripts/backend-lint.sh
+
 test-go:
+	@echo "test backend"
 	go test -v ./pkg/...
 
 test-js:
+	@echo "test frontend"
 	yarn test
 
 test: test-go test-js
 
 run:
+	@echo "start a server"
 	./bin/grafana-server
 
 clean:
+	@echo "cleaning"
 	rm -rf node_modules
 	rm -rf public/build
 
 node_modules: package.json yarn.lock
+	@echo "install frontend dependencies"
 	yarn install --pure-lockfile --no-progress

conf/revive.toml

@@ -0,0 +1,27 @@
+ignoreGeneratedHeader = false
+severity = "error"
+confidence = 0.8
+errorCode = 0
+
+[rule.context-as-argument]
+[rule.error-return]
+[rule.package-comments]
+[rule.range]
+[rule.superfluous-else]
+[rule.modifies-parameter]
+
+# This can be checked by other tools like megacheck
+[rule.unreachable-code]
+
+# Those are probably should be enabled at some point
+# [rule.unexported-return]
+# [rule.exported]
+# [rule.var-naming]
+# [error-naming]
+# [rule.dot-imports]
+# [blank-imports]
+# [rule.receiver-naming]
+# [error-strings]
+# [rule.if-return]
+# [rule.indent-error-flow]
+# [rule.blank-imports]

grafana.deb

@@ -0,0 +1,36 @@
+name: "grafana"
+arch: "${ARCH}"
+platform: "linux"
+version: "${VERSION}"
+section: "default"
+priority: "extra"
+replaces:
+- grafana
+provides:
+- grafana-server
+- grafana-cli
+depends:
+- adduser
+- libfontconfig
+maintainer: "<[email protected]>"
+description: |
+  Grafana
+vendor: "Grafana"
+homepage: "https://grafana.com"
+license: "Apache 2"
+bindir: "/usr/sbin"
+files:
+  "./bin/grafana-server": "/usr/sbin/grafana-server"
+  "./bin/grafana-cli": "/usr/sbin/grafana-cli"
+config_files:
+  ./packaging/deb/init.d/grafana-server: "/etc/init.d/grafana-server"
+  ./packaging/deb/default/grafana-server: "/etc/default/grafana-server"
+  ./packaging/deb/systemd/grafana-server.service: "/usr/lib/systemd/system/grafana-server.service"
+overrides:
+  rpm:
+    scripts:
+      preinstall: ./scripts/preinstall.sh
+      postremove: ./scripts/postremove.sh
+  deb:
+    scripts:
+      postinstall: ./packaging/deb/control/postinst

packaging/conf/nfpm.yaml

@@ -34,9 +34,9 @@ func runDbCommand(command func(commandLine CommandLine) error) func(context *cli
 
 			cmd.ShowHelp()
 			os.Exit(1)
-		} else {
-			logger.Info("\n\n")
 		}
+
+		logger.Info("\n\n")
 	}
 }
 
@@ -50,9 +50,9 @@ func runPluginCommand(command func(commandLine CommandLine) error) func(context
 
 			cmd.ShowHelp()
 			os.Exit(1)
-		} else {
-			logger.Info("\nRestart grafana after installing plugins . <service grafana-server restart>\n\n")
 		}
+
+		logger.Info("\nRestart grafana after installing plugins . <service grafana-server restart>\n\n")
 	}
 }
 

pkg/cmd/grafana-cli/commands/commands.go

@@ -11,12 +11,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package graphite provides a bridge to push Prometheus metrics to a Graphite
+// Package graphitebridge provides a bridge to push Prometheus metrics to a Graphite
 // server.
 package graphitebridge
 
 import (
 	"bufio"
+	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -26,14 +27,10 @@ import (
 	"strings"
 	"time"
 
-	"context"
-
+	"github.com/prometheus/client_golang/prometheus"
+	dto "github.com/prometheus/client_model/go"
 	"github.com/prometheus/common/expfmt"
 	"github.com/prometheus/common/model"
-
-	dto "github.com/prometheus/client_model/go"
-
-	"github.com/prometheus/client_golang/prometheus"
 )
 
 const (

pkg/infra/metrics/graphitebridge/graphite.go

@@ -19,6 +19,7 @@ go get -u github.com/opennota/check/cmd/structcheck
 go get -u github.com/mdempsky/unconvert
 go get -u github.com/opennota/check/cmd/varcheck
 go get -u honnef.co/go/tools/cmd/staticcheck
+go get -u github.com/mgechev/revive
 
 exit_if_fail gometalinter --enable-gc --vendor --deadline 10m --disable-all \
   --enable=deadcode \
@@ -31,3 +32,4 @@ exit_if_fail gometalinter --enable-gc --vendor --deadline 10m --disable-all \
   --enable=staticcheck
 
 exit_if_fail go vet ./pkg/...
+exit_if_fail revive -formatter stylish -config ./conf/revive.toml

scripts/gometalinter.sh renamed to scripts/backend-lint.sh

@@ -20,9 +20,9 @@ In the `setting` packages there are many global variables which Grafana sets at
 away from and move as much configuration as possible to the `setting.Cfg` struct and pass it around, just like the bus.
 
 ## Linting and formatting
-We enforce strict `gofmt` formating and use some linters on our codebase. You can find the current list of linters at https://github.com/grafana/grafana/blob/master/scripts/gometalinter.sh#L23 
+We enforce strict `gofmt` formating and use some linters on our codebase. You can find the current list of linters at https://github.com/grafana/grafana/blob/master/scripts/backend-lint.sh 
 
-We don't enforce `golint` but we encourage it and we will test so the number of linting errors does not increase over time.
+We use [revive](https://github.com/mgechev/revive) as a go linter, and do enforce our [custom config](https://github.com/grafana/grafana/blob/master/conf/revive.toml) for it.
 
 ## Testing
 We use GoConvey for BDD/scenario based testing. Which we think is useful for testing certain chain or interactions. Ex https://github.com/grafana/grafana/blob/master/pkg/services/auth/auth_token_test.go