Adds golangci lint check and fix corresponding issues

Author: roobalimsab

.circleci/config.yml

@@ -1,5 +1,38 @@
 version: 2
 jobs:
+  build:
+    working_directory: /go/src/github.com/gojekfarm/kat
+    docker:
+    - image: circleci/golang:1.11
+    steps:
+    - checkout
+    - setup_remote_docker:
+        docker_layer_caching: false
+    - restore_cache:
+        keys:
+        - vendor-pkg-{{ checksum "go.sum" }}
+        - vendor-pkg-
+    - run:
+        name: Install Dependencies
+        command: |
+          make setup
+    - save_cache:
+        paths:
+        - ./vendor
+        - "/go/pkg"
+        key: vendor-pkg-{{ checksum "go.sum" }}
+    - run:
+        name: Check Quality
+        command: |
+          env GO111MODULE=on go mod verify
+          make clean check-quality golangci
+    - run:
+        name: Build
+        command: |
+          env GO111MODULE=on go mod verify
+          make clean build
+
+
   test:
     working_directory: /go/src/github.com/gojekfarm/kat
     docker:
@@ -25,7 +58,7 @@ jobs:
         name: Run tests
         command: |
           env GO111MODULE=on go mod verify
-          env GO111MODULE=on CGO_ENABLED=0 make
+          make test
 
 
   release:
@@ -52,14 +85,17 @@ jobs:
       - run:
           name: Release go binary
           command: |
-            env GO111MODULE=on CGO_ENABLED=0 make
+            make
             curl -sL https://git.io/goreleaser | bash
 
 workflows:
   version: 2
-  build_and_test:
+  build-test-release:
     jobs:
-    - test
+    - build
+    - test:
+        requires:
+          - build
     - release:
         requires:
           - test

.golangci.yml

@@ -0,0 +1,92 @@
+linters-settings:
+  govet:
+    check-shadowing: true
+    settings:
+      printf:
+        funcs:
+          - (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
+  golint:
+    min-confidence: 0
+  gocyclo:
+    min-complexity: 8
+  maligned:
+    suggest-new: true
+  dupl:
+    threshold: 100
+  goconst:
+    min-len: 2
+    min-occurrences: 3
+  misspell:
+    locale: US
+  lll:
+    line-length: 160
+  goimports:
+    local-prefixes: github.com/golangci/golangci-lint
+  gocritic:
+    enabled-tags:
+      - performance
+      - style
+      - experimental
+    disabled-checks:
+      - wrapperFunc
+      - hugeParam
+      - octalLiteral
+
+    settings: # settings passed to gocritic
+      captLocal: # must be valid enabled check name
+        paramsOnly: true
+      rangeValCopy:
+        sizeThreshold: 200
+
+linters:
+  disable-all: true
+  enable:
+  - deadcode
+  - depguard
+  - dupl
+  - errcheck
+  - goconst
+  - gocritic
+  - gocyclo
+  - gofmt
+  - goimports
+  - golint
+  - gosec
+  - gosimple
+  - govet
+  - ineffassign
+  - interfacer
+  - lll
+  - misspell
+  - nakedret
+  - scopelint
+  - staticcheck
+  - structcheck
+  - stylecheck
+  - typecheck
+  - unconvert
+  - unparam
+  - unused
+  - varcheck
+
+run:
+  skip-dirs:
+    - test/testdata_etc
+    - pkg/golinters/goanalysis/(checker|passes)
+  tests: false
+
+issues:
+  exclude-rules:
+    - text: "weak cryptographic primitive"
+      linters:
+        - gosec
+
+# golangci.com configuration
+# https://github.com/golangci/golangci/wiki/Configuration
+service:
+  golangci-lint-version: 1.15.x # use the fixed version to not introduce new linters unexpectedly
+  prepare:
+    - echo "here I can run custom commands, but no preparation needed for this repo"

Makefile

@@ -1,4 +1,4 @@
-all: clean check-quality test build
+all: clean check-quality golangci test build
 APP=kat
 ALL_PACKAGES=$(shell go list ./...)
 SOURCE_DIRS=$(shell go list ./... | cut -d "/" -f4 | uniq)
@@ -15,6 +15,9 @@ setup:
 test:
 	go test ./...
 
+testcodecov:
+	go test -coverprofile=coverage.txt -covermode=atomic ./...
+
 build:
 	@echo "Building './kat'..."
 	go mod download
@@ -40,6 +43,10 @@ fix_imports:
 vet:
 	go vet ./...
 
+golangci:
+	GO111MODULE=off go get -v github.com/golangci/golangci-lint/cmd/golangci-lint
+	golangci-lint run -v --deadline 5m0s
+
 test-coverage:
 	mkdir -p ./out
 	@echo "mode: count" > coverage-all.out

README.md

@@ -1,4 +1,5 @@
 # KAT
+[![Build Status](https://circleci.com/gh/gojekfarm/kat.svg?branch=master)](https://circleci.com/gh/gojekfarm/kat)
 
 Kafka Admin Tool provides an interface to perform many admin operations on kafka in a straight-forward manner.
 
@@ -10,12 +11,14 @@ brew install kat
 ```
 
 ### Others
-1. Download the project -- ```go get -u github.com/gojekfarm/kat```
-2. Build -- ```make```
+```
+go install github.com/gojekfarm/kat
+```
 
 ### Local Dev/Testing
-* Unit tests - ```make test```
-* Manual - Two kafka clusters can be created in local environment using docker-compose - ```docker-compose up -d```. Commands can be run against these clusters for testing
+* Clone the repo
+* Run ```make all``` to run lint checks, unit tests and build the project
+* Manual testing: Running ```docker-compose up -d``` will create 2 local kafka clusters. Commands can be run against these clusters for testing
 
 ## Admin operations available
 - [List Topics](#list-topics)

cmd/alterconfig.go

@@ -28,7 +28,9 @@ var alterConfigCmd = &cobra.Command{
 
 func init() {
 	alterConfigCmd.PersistentFlags().StringP("config", "c", "", "Comma separated list of configs, eg: key1=val1,key2=val2")
-	alterConfigCmd.MarkPersistentFlagRequired("config")
+	if err := alterConfigCmd.MarkPersistentFlagRequired("config"); err != nil {
+		logger.Fatal(err)
+	}
 }
 
 func (a *alterConfig) alterConfig() {

cmd/base.go

@@ -43,7 +43,6 @@ func WithAddr(addrConfig string) Opts {
 }
 
 func (b *BaseCmd) setTopicCli() {
-	var err error
 	addr := strings.Split(b.cobraUtil.GetStringArg(b.addrConfig), ",")
 	var opts []pkg.TopicOpts
 	if b.enableSSH {

cmd/config.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"github.com/gojekfarm/kat/logger"
 	"github.com/spf13/cobra"
 )
 
@@ -11,8 +12,9 @@ var configCmd = &cobra.Command{
 
 func init() {
 	configCmd.PersistentFlags().StringP("topics", "t", "", "Comma separated list of topic names for which retention needs to be changed")
-	configCmd.MarkPersistentFlagRequired("topics")
-
+	if err := configCmd.MarkPersistentFlagRequired("topics"); err != nil {
+		logger.Fatal(err)
+	}
 	configCmd.AddCommand(showConfigCmd)
 	configCmd.AddCommand(alterConfigCmd)
 }

cmd/deletetopic.go

@@ -60,21 +60,15 @@ func init() {
 }
 
 func (d *deleteTopic) deleteTopic() {
-	var regex string
-	var include bool
-	if ((d.topicWhitelist == "") && (d.topicBlacklist == "")) || ((d.topicWhitelist != "") && (d.topicBlacklist != "")) {
-		logger.Fatalf("Any one of blacklist or whitelist should be passed.")
-		return
-	}
-	if d.topicWhitelist != "" {
-		include = true
-		regex = d.topicWhitelist
-	} else if d.topicBlacklist != "" {
-		include = false
-		regex = d.topicBlacklist
+	regex, include, err := d.filterCriteria()
+	if err != nil {
+		logger.Fatal(err)
 	}
 	topics, err := d.getTopics(regex, include)
-	if err != nil || len(topics) == 0 {
+	if err != nil {
+		logger.Fatal(err)
+	}
+	if len(topics) == 0 {
 		return
 	}
 	fmt.Println("------------------------------------------------------------")
@@ -91,6 +85,20 @@ func (d *deleteTopic) deleteTopic() {
 	}
 }
 
+func (d *deleteTopic) filterCriteria() (regex string, include bool, err error) {
+	if ((d.topicWhitelist == "") && (d.topicBlacklist == "")) || ((d.topicWhitelist != "") && (d.topicBlacklist != "")) {
+		return regex, include, fmt.Errorf("any one of blacklist or whitelist should be passed")
+	}
+	if d.topicWhitelist != "" {
+		include = true
+		regex = d.topicWhitelist
+	} else if d.topicBlacklist != "" {
+		include = false
+		regex = d.topicBlacklist
+	}
+	return regex, include, nil
+}
+
 func (d *deleteTopic) getLastWrittenTopics() ([]string, error) {
 	topics, err := d.TopicCli.ListLastWrittenTopics(d.lastWrite, d.dataDir)
 	if err != nil {
@@ -101,17 +109,20 @@ func (d *deleteTopic) getLastWrittenTopics() ([]string, error) {
 }
 
 func (d *deleteTopic) getTopics(regex string, include bool) ([]string, error) {
-	var topics []string
-	var err error
 	if d.lastWrite != 0 {
 		lastWrittenTopics, err := d.getLastWrittenTopics()
 		if err != nil {
 			return nil, err
 		}
-		topics, err = util.Filter(lastWrittenTopics, regex, include)
-	} else {
-		topics, err = d.TopicCli.ListOnly(regex, include)
+		topics, err := util.Filter(lastWrittenTopics, regex, include)
+		if err != nil {
+			logger.Errorf("Error while fetching topic list - %v\n", err)
+			return nil, err
+		}
+		return topics, nil
 	}
+
+	topics, err := d.TopicCli.ListOnly(regex, include)
 	if err != nil {
 		logger.Errorf("Error while fetching topic list - %v\n", err)
 		return nil, err

cmd/deletetopic_test.go

@@ -174,8 +174,13 @@ func TestDelete_WhenLastWriteIsPassed_DeletesBlackListedTopicsOnError(t *testing
 
 	d := deleteTopic{BaseCmd: BaseCmd{TopicCli: mockTopicCli}, topicBlacklist: "test-1|test-2", io: mockIo, lastWrite: 123}
 	mockTopicCli.On("ListLastWrittenTopics", d.lastWrite, d.dataDir).Return(topics, errors.New("test"))
+	fakeExit := func(int) {
+		panic("os.Exit called")
+	}
+	patch := monkey.Patch(os.Exit, fakeExit)
+	defer patch.Unpatch()
 
-	d.deleteTopic()
+	assert.PanicsWithValue(t, "os.Exit called", d.deleteTopic, "os.Exit was not called")
 	mockTopicCli.AssertNotCalled(t, "ListOnly", mock.Anything, mock.Anything)
 	mockTopicCli.AssertNotCalled(t, "Delete", mock.Anything)
 	mockIo.AssertNotCalled(t, "AskForConfirmation", mock.Anything)

cmd/describetopic.go

@@ -28,7 +28,9 @@ var describeTopicCmd = &cobra.Command{
 
 func init() {
 	describeTopicCmd.PersistentFlags().StringP("topics", "t", "", "Comma separated list of topic names to describe")
-	describeTopicCmd.MarkPersistentFlagRequired("topics")
+	if err := describeTopicCmd.MarkPersistentFlagRequired("topics"); err != nil {
+		logger.Fatal(err)
+	}
 }
 
 func (d *describeTopic) describeTopic() {
@@ -41,11 +43,13 @@ func (d *describeTopic) describeTopic() {
 
 func printConfigs(metadata []*pkg.TopicMetadata) {
 	for _, topicMetadata := range metadata {
-		fmt.Printf("Topic Name: %v,\nIsInternal: %v,\nPartitions:\n", (*topicMetadata).Name, (*topicMetadata).IsInternal)
+		fmt.Printf("Topic Name: %v,\nIsInternal: %v,\nPartitions:\n", topicMetadata.Name, topicMetadata.IsInternal)
 
-		partitions := (*topicMetadata).Partitions
+		partitions := topicMetadata.Partitions
 		for _, partitionMetadata := range partitions {
-			fmt.Printf("Id: %v, Leader: %v, Replicas: %v, ISR: %v, OfflineReplicas: %v\n", (*partitionMetadata).ID, (*partitionMetadata).Leader, (*partitionMetadata).Replicas, (*partitionMetadata).Isr, (*partitionMetadata).OfflineReplicas)
+			fmt.Printf("Id: %v, Leader: %v, Replicas: %v, ISR: %v, OfflineReplicas: %v\n",
+				partitionMetadata.ID, partitionMetadata.Leader, partitionMetadata.Replicas,
+				partitionMetadata.Isr, partitionMetadata.OfflineReplicas)
 		}
 		fmt.Println()
 	}