From 04b3afcd1519e36214d5cfd4ffb5d1f60d488bd5 Mon Sep 17 00:00:00 2001 From: Oleg Gaidarenko Date: Wed, 27 Mar 2019 17:53:49 +0100 Subject: [PATCH] Chore: Implement revive (#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 #16109 Ref #16160 --- .circleci/config.yml | 28 +++++++-------- .editorconfig | 4 +++ Makefile | 17 +++++++++ conf/revive.toml | 27 +++++++++++++++ grafana.deb | 0 packaging/conf/nfpm.yaml | 36 ++++++++++++++++++++ pkg/cmd/grafana-cli/commands/commands.go | 8 ++--- pkg/infra/metrics/graphitebridge/graphite.go | 11 +++--- scripts/{gometalinter.sh => backend-lint.sh} | 2 ++ style_guides/backend.md | 4 +-- 10 files changed, 110 insertions(+), 27 deletions(-) create mode 100644 conf/revive.toml create mode 100644 grafana.deb create mode 100644 packaging/conf/nfpm.yaml rename scripts/{gometalinter.sh => backend-lint.sh} (89%) diff --git a/.circleci/config.yml b/.circleci/config.yml index e5bf6cdad6e..cb72d9434c2 100644 --- a/.circleci/config.yml +++ b/.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 diff --git a/.editorconfig b/.editorconfig index cbde126ca6c..18e6162dd6d 100644 --- a/.editorconfig +++ b/.editorconfig @@ -18,3 +18,7 @@ insert_final_newline = true [*.md] trim_trailing_whitespace = false + +[Makefile] +indent_style = tab +indent_size = 2 diff --git a/Makefile b/Makefile index 6410714d4fc..365a925d61c 100644 --- a/Makefile +++ b/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 diff --git a/conf/revive.toml b/conf/revive.toml new file mode 100644 index 00000000000..1c7e3b92a65 --- /dev/null +++ b/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] diff --git a/grafana.deb b/grafana.deb new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packaging/conf/nfpm.yaml b/packaging/conf/nfpm.yaml new file mode 100644 index 00000000000..6eafe22e250 --- /dev/null +++ b/packaging/conf/nfpm.yaml @@ -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: "" +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 diff --git a/pkg/cmd/grafana-cli/commands/commands.go b/pkg/cmd/grafana-cli/commands/commands.go index 902fd415977..d5add2b7168 100644 --- a/pkg/cmd/grafana-cli/commands/commands.go +++ b/pkg/cmd/grafana-cli/commands/commands.go @@ -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 . \n\n") } + + logger.Info("\nRestart grafana after installing plugins . \n\n") } } diff --git a/pkg/infra/metrics/graphitebridge/graphite.go b/pkg/infra/metrics/graphitebridge/graphite.go index 5b61f078e6c..ba687fe8921 100644 --- a/pkg/infra/metrics/graphitebridge/graphite.go +++ b/pkg/infra/metrics/graphitebridge/graphite.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 ( diff --git a/scripts/gometalinter.sh b/scripts/backend-lint.sh similarity index 89% rename from scripts/gometalinter.sh rename to scripts/backend-lint.sh index b360b7f1222..98519d59a00 100755 --- a/scripts/gometalinter.sh +++ b/scripts/backend-lint.sh @@ -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 diff --git a/style_guides/backend.md b/style_guides/backend.md index 1c6c86efc0b..b428303dbaa 100644 --- a/style_guides/backend.md +++ b/style_guides/backend.md @@ -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