Refactor code using gofmt and go lint.

codenio · codenio · commit 0dbaf7e84406 · 2019-08-25T10:19:55.000+05:30
This Commit,
- Refactors code using `gofmt` and `go lint` to improve code quality. 
- Relocates `diff` and `manifests` into `pkg/` dir.
- Adds scripts to `gofmt` code and to update required changes
- Adds scripts to `go lint` and `go vet` code
- Adds `lint` stage before `build` to retain code quality
diff --git a/Makefile b/Makefile
@@ -20,8 +20,15 @@ install: build
 	cp bin/diff $(HELM_HOME)/plugins/helm-diff/bin
 	cp plugin.yaml $(HELM_HOME)/plugins/helm-diff/
 
+.PHONY: lint
+lint:
+	scripts/update-gofmt.sh
+	scripts/verify-gofmt.sh
+	scripts/verify-golint.sh
+	scripts/verify-govet.sh
+
 .PHONY: build
-build:
+build: lint
 	mkdir -p bin/
 	go build -i -v -o bin/diff -ldflags="$(LDFLAGS)"
 
@@ -61,7 +68,7 @@ dist:
 	tar -C build/ -zcvf $(CURDIR)/release/helm-diff-windows.tgz diff/
 
 .PHONY: release
-release: dist
+release: lint dist 
 ifndef GITHUB_TOKEN
 	$(error GITHUB_TOKEN is undefined)
 endif
diff --git a/cmd/error.go b/cmd/error.go
@@ -1,5 +1,6 @@
 package cmd
 
+// Error to report errors
 type Error struct {
 	error
 	Code int
diff --git a/cmd/helm.go b/cmd/helm.go
@@ -41,9 +41,9 @@ func (v *valueFiles) Valid() error {
 
 	if errStr == "" {
 		return nil
-	} else {
-		return errors.New(errStr)
 	}
+
+	return errors.New(errStr)
 }
 
 func (v *valueFiles) Type() string {
@@ -148,12 +148,12 @@ func (d *diffCmd) vals() ([]byte, error) {
 		currentMap := map[string]interface{}{}
 
 		var bytes []byte
- 		var err error
- 		if strings.TrimSpace(filePath) == "-" {
- 			bytes, err = ioutil.ReadAll(os.Stdin)
- 		} else {
- 			bytes, err = ioutil.ReadFile(filePath)
- 		}
+		var err error
+		if strings.TrimSpace(filePath) == "-" {
+			bytes, err = ioutil.ReadAll(os.Stdin)
+		} else {
+			bytes, err = ioutil.ReadFile(filePath)
+		}
 		if err != nil {
 			return []byte{}, err
 		}
diff --git a/cmd/helpers.go b/cmd/helpers.go
@@ -19,7 +19,8 @@ const (
 )
 
 var (
-	settings        helm_env.EnvSettings
+	settings helm_env.EnvSettings
+	// DefaultHelmHome to hold default home path of .helm dir
 	DefaultHelmHome = filepath.Join(homedir.HomeDir(), ".helm")
 )
 
@@ -59,7 +60,7 @@ func createHelmClient() helm.Interface {
 }
 
 func expandTLSPaths() {
-       settings.TLSCaCertFile = os.ExpandEnv(settings.TLSCaCertFile)
-       settings.TLSCertFile = os.ExpandEnv(settings.TLSCertFile)
-       settings.TLSKeyFile = os.ExpandEnv(settings.TLSKeyFile)
+	settings.TLSCaCertFile = os.ExpandEnv(settings.TLSCaCertFile)
+	settings.TLSCertFile = os.ExpandEnv(settings.TLSCertFile)
+	settings.TLSKeyFile = os.ExpandEnv(settings.TLSKeyFile)
 }
diff --git a/cmd/release.go b/cmd/release.go
@@ -8,8 +8,8 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/helm/pkg/helm"
 
-	"github.com/databus23/helm-diff/diff"
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/diff"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
 type release struct {
@@ -91,7 +91,7 @@ func (d *release) differentiate() error {
 	}
 
 	if releaseResponse1.Release.Chart.Metadata.Name == releaseResponse2.Release.Chart.Metadata.Name {
-		seenAnyChanges := diff.DiffReleases(
+		seenAnyChanges := diff.DifferenceReleases(
 			manifest.ParseRelease(releaseResponse1.Release, d.includeTests),
 			manifest.ParseRelease(releaseResponse2.Release, d.includeTests),
 			d.suppressedKinds,
diff --git a/cmd/revision.go b/cmd/revision.go
@@ -9,8 +9,8 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/helm/pkg/helm"
 
-	"github.com/databus23/helm-diff/diff"
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/diff"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
 type revision struct {
@@ -104,7 +104,7 @@ func (d *revision) differentiate() error {
 			return prettyError(err)
 		}
 
-		diff.DiffManifests(
+		diff.DifferenceManifests(
 			manifest.ParseRelease(revisionResponse.Release, d.includeTests),
 			manifest.ParseRelease(releaseResponse.Release, d.includeTests),
 			d.suppressedKinds,
@@ -128,7 +128,7 @@ func (d *revision) differentiate() error {
 			return prettyError(err)
 		}
 
-		seenAnyChanges := diff.DiffManifests(
+		seenAnyChanges := diff.DifferenceManifests(
 			manifest.ParseRelease(revisionResponse1.Release, d.includeTests),
 			manifest.ParseRelease(revisionResponse2.Release, d.includeTests),
 			d.suppressedKinds,
diff --git a/cmd/rollback.go b/cmd/rollback.go
@@ -9,8 +9,8 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/helm/pkg/helm"
 
-	"github.com/databus23/helm-diff/diff"
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/diff"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
 type rollback struct {
@@ -96,7 +96,7 @@ func (d *rollback) backcast() error {
 	}
 
 	// create a diff between the current manifest and the version of the manifest that a user is intended to rollback
-	seenAnyChanges := diff.DiffManifests(
+	seenAnyChanges := diff.DifferenceManifests(
 		manifest.ParseRelease(releaseResponse.Release, d.includeTests),
 		manifest.ParseRelease(revisionResponse.Release, d.includeTests),
 		d.suppressedKinds,
diff --git a/cmd/root.go b/cmd/root.go
@@ -25,6 +25,7 @@ The Helm Diff Plugin
   helm rollback will perform.
 `
 
+// New creates a new cobra client
 func New() *cobra.Command {
 
 	chartCommand := newChartCommand()
diff --git a/cmd/upgrade.go b/cmd/upgrade.go
@@ -9,8 +9,8 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/helm/pkg/helm"
 
-	"github.com/databus23/helm-diff/diff"
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/diff"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
 type diffCmd struct {
@@ -171,7 +171,7 @@ func (d *diffCmd) run() error {
 		}
 	}
 
-	seenAnyChanges := diff.DiffManifests(currentSpecs, newSpecs, d.suppressedKinds, d.outputContext, os.Stdout)
+	seenAnyChanges := diff.DifferenceManifests(currentSpecs, newSpecs, d.suppressedKinds, d.outputContext, os.Stdout)
 
 	if d.detailedExitCode && seenAnyChanges {
 		return Error{
diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go
@@ -10,10 +10,11 @@ import (
 	"github.com/aryann/difflib"
 	"github.com/mgutz/ansi"
 
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
-func DiffManifests(oldIndex, newIndex map[string]*manifest.MappingResult, suppressedKinds []string, context int, to io.Writer) bool {
+// DifferenceManifests diff on manifests
+func DifferenceManifests(oldIndex, newIndex map[string]*manifest.MappingResult, suppressedKinds []string, context int, to io.Writer) bool {
 	seenAnyChanges := false
 	emptyMapping := &manifest.MappingResult{}
 	for key, oldContent := range oldIndex {
@@ -52,11 +53,11 @@ func DiffManifests(oldIndex, newIndex map[string]*manifest.MappingResult, suppre
 	return seenAnyChanges
 }
 
-// DiffReleases reindex the content  based on the template names and pass it to DiffManifests
-func DiffReleases(oldIndex, newIndex map[string]*manifest.MappingResult, suppressedKinds []string, context int, to io.Writer) bool {
+// DifferenceReleases reindex the content  based on the template names and pass it to DifferenceManifests
+func DifferenceReleases(oldIndex, newIndex map[string]*manifest.MappingResult, suppressedKinds []string, context int, to io.Writer) bool {
 	oldIndex = reIndexForRelease(oldIndex)
 	newIndex = reIndexForRelease(newIndex)
-	return DiffManifests(oldIndex, newIndex, suppressedKinds, context, to)
+	return DifferenceManifests(oldIndex, newIndex, suppressedKinds, context, to)
 }
 
 func diffMappingResults(oldContent *manifest.MappingResult, newContent *manifest.MappingResult) []difflib.DiffRecord {
diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go
@@ -7,7 +7,7 @@ import (
 	"github.com/mgutz/ansi"
 	"github.com/stretchr/testify/require"
 
-	"github.com/databus23/helm-diff/manifest"
+	"github.com/databus23/helm-diff/pkg/manifest"
 )
 
 var text1 = "" +
@@ -127,9 +127,9 @@ func assertDiff(t *testing.T, before, after string, context int, expected string
 	}
 }
 
-func TestDiffManifests(t *testing.T) {
+func TestDifferenceManifests(t *testing.T) {
 	specBeta := map[string]*manifest.MappingResult{
-		"default, nginx, Deployment (apps)": &manifest.MappingResult{
+		"default, nginx, Deployment (apps)": {
 
 			Name: "default, nginx, Deployment (apps)",
 			Kind: "Deployment",
@@ -142,7 +142,7 @@ metadata:
 		}}
 
 	specRelease := map[string]*manifest.MappingResult{
-		"default, nginx, Deployment (apps)": &manifest.MappingResult{
+		"default, nginx, Deployment (apps)": {
 
 			Name: "default, nginx, Deployment (apps)",
 			Kind: "Deployment",
@@ -158,8 +158,8 @@ metadata:
 
 		var buf1 bytes.Buffer
 
-		if changesSeen := DiffManifests(specBeta, specRelease, []string{}, 10, &buf1); !changesSeen {
-			t.Error("Unexpected return value from DiffManifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
+		if changesSeen := DifferenceManifests(specBeta, specRelease, []string{}, 10, &buf1); !changesSeen {
+			t.Error("Unexpected return value from DifferenceManifests: Expected the return value to be `true` to indicate that it has seen any change(s), but was `false`")
 		}
 
 		require.Equal(t, `default, nginx, Deployment (apps) has changed:
@@ -176,8 +176,8 @@ metadata:
 	t.Run("OnNoChange", func(t *testing.T) {
 		var buf2 bytes.Buffer
 
-		if changesSeen := DiffManifests(specRelease, specRelease, []string{}, 10, &buf2); changesSeen {
-			t.Error("Unexpected return value from DiffManifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")
+		if changesSeen := DifferenceManifests(specRelease, specRelease, []string{}, 10, &buf2); changesSeen {
+			t.Error("Unexpected return value from DifferenceManifests: Expected the return value to be `false` to indicate that it has NOT seen any change(s), but was `true`")
 		}
 
 		require.Equal(t, ``, buf2.String())
diff --git a/pkg/manifest/parse.go b/pkg/manifest/parse.go
@@ -13,14 +13,15 @@ import (
 
 var yamlSeperator = []byte("\n---\n")
 
+// MappingResult to store result of diff
 type MappingResult struct {
 	Name    string
 	Kind    string
 	Content string
 }
 
 type metadata struct {
-	ApiVersion string `yaml:"apiVersion"`
+	APIVersion string `yaml:"apiVersion"`
 	Kind       string
 	Metadata   struct {
 		Namespace string
@@ -29,7 +30,7 @@ type metadata struct {
 }
 
 func (m metadata) String() string {
-	apiBase := m.ApiVersion
+	apiBase := m.APIVersion
 	sp := strings.Split(apiBase, "/")
 	if len(sp) > 1 {
 		apiBase = strings.Join(sp[:len(sp)-1], "/")
@@ -61,6 +62,7 @@ func splitSpec(token string) (string, string) {
 	return "", ""
 }
 
+// ParseRelease parses release objects into MappingResult
 func ParseRelease(release *release.Release, includeTests bool) map[string]*MappingResult {
 	manifest := release.Manifest
 	for _, hook := range release.Hooks {
@@ -75,12 +77,13 @@ func ParseRelease(release *release.Release, includeTests bool) map[string]*Mappi
 	return Parse(manifest, release.Namespace)
 }
 
+// Parse parses manifest strings into MappingResult
 func Parse(manifest string, defaultNamespace string) map[string]*MappingResult {
 	scanner := bufio.NewScanner(strings.NewReader(manifest))
 	scanner.Split(scanYamlSpecs)
 	//Allow for tokens (specs) up to 1M in size
 	scanner.Buffer(make([]byte, bufio.MaxScanTokenSize), 1048576)
-	//Discard the first result, we only care about everything after the first seperator
+	//Discard the first result, we only care about everything after the first separator
 	scanner.Scan()
 
 	result := make(map[string]*MappingResult)
diff --git a/pkg/manifest/parse_test.go b/pkg/manifest/parse_test.go
@@ -7,12 +7,12 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	. "github.com/databus23/helm-diff/manifest"
+	. "github.com/databus23/helm-diff/pkg/manifest"
 )
 
 func foundObjects(result map[string]*MappingResult) []string {
 	objs := make([]string, 0, len(result))
-	for k, _ := range result {
+	for k := range result {
 		objs = append(objs, k)
 	}
 	sort.Strings(objs)
diff --git a/pkg/manifest/testdata/deploy_v1.yaml b/pkg/manifest/testdata/deploy_v1.yaml
diff --git a/pkg/manifest/testdata/deploy_v1beta1.yaml b/pkg/manifest/testdata/deploy_v1beta1.yaml
diff --git a/pkg/manifest/testdata/empty.yaml b/pkg/manifest/testdata/empty.yaml
diff --git a/pkg/manifest/testdata/pod.yaml b/pkg/manifest/testdata/pod.yaml
diff --git a/pkg/manifest/testdata/pod_namespace.yaml b/pkg/manifest/testdata/pod_namespace.yaml
diff --git a/scripts/update-gofmt.sh b/scripts/update-gofmt.sh
@@ -0,0 +1,16 @@
+#!/bin/bash
+# script credits : https://github.com/infracloudio/botkube
+
+set -o errexit
+set -o nounset
+set -o pipefail
+
+find_files() {
+  find . -not \( \
+      \( \
+        -wholename '*/vendor/*' \
+      \) -prune \
+    \) -name '*.go'
+}
+
+find_files | xargs gofmt -w -s
diff --git a/scripts/verify-gofmt.sh b/scripts/verify-gofmt.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# script credits : https://github.com/infracloudio/botkube
+
+set -o errexit
+set -o nounset
+set -o pipefail
+
+find_files() {
+  find . -not \( \
+      \( \
+        -wholename '*/vendor/*' \
+      \) -prune \
+    \) -name '*.go'
+}
+
+bad_files=$(find_files | xargs gofmt -d -s 2>&1)
+if [[ -n "${bad_files}" ]]; then
+  echo "${bad_files}" >&2
+  echo >&2
+  echo "Run ./hack/update-gofmt.sh" >&2
+  exit 1
+fi
diff --git a/scripts/verify-golint.sh b/scripts/verify-golint.sh
@@ -0,0 +1,20 @@
+#!/bin/bash
+# script credits : https://github.com/infracloudio/botkube
+
+set -o errexit
+set -o nounset
+set -o pipefail
+
+find_files() {
+  find . -not \( \
+      \( \
+        -wholename '*/vendor/*' \
+      \) -prune \
+    \) -name '*.go'
+}
+
+bad_files=$(find_files | xargs -I@ bash -c "golint @")
+if [[ -n "${bad_files}" ]]; then
+  echo "${bad_files}"
+  exit 1
+fi
diff --git a/scripts/verify-govet.sh b/scripts/verify-govet.sh
@@ -0,0 +1,7 @@
+#!/bin/bash
+# script credits : https://github.com/infracloudio/botkube
+
+set -x
+
+go vet ./pkg/...
+go vet ./cmd/...

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,8 @@ const (`
`19`	`19`	`)`
`20`	`20`
`21`	`21`	`var (`
`22`		`- settings helm_env.EnvSettings`
	`22`	`+ settings helm_env.EnvSettings`
	`23`	`+ // DefaultHelmHome to hold default home path of .helm dir`
`23`	`24`	`DefaultHelmHome = filepath.Join(homedir.HomeDir(), ".helm")`
`24`	`25`	`)`
`25`	`26`
`@@ -59,7 +60,7 @@ func createHelmClient() helm.Interface {`
`59`	`60`	`}`
`60`	`61`
`61`	`62`	`func expandTLSPaths() {`
`62`		`- settings.TLSCaCertFile = os.ExpandEnv(settings.TLSCaCertFile)`
`63`		`- settings.TLSCertFile = os.ExpandEnv(settings.TLSCertFile)`
`64`		`- settings.TLSKeyFile = os.ExpandEnv(settings.TLSKeyFile)`
	`63`	`+ settings.TLSCaCertFile = os.ExpandEnv(settings.TLSCaCertFile)`
	`64`	`+ settings.TLSCertFile = os.ExpandEnv(settings.TLSCertFile)`
	`65`	`+ settings.TLSKeyFile = os.ExpandEnv(settings.TLSKeyFile)`
`65`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ The Helm Diff Plugin`
`25`	`25`	`helm rollback will perform.`
`26`	`26`	`
`27`	`27`
	`28`	`+// New creates a new cobra client`
`28`	`29`	`func New() *cobra.Command {`
`29`	`30`
`30`	`31`	`chartCommand := newChartCommand()`
Original file line number	Diff line number	Diff line change
`@@ -9,8 +9,8 @@ import (`
`9`	`9`	`"github.com/spf13/cobra"`
`10`	`10`	`"k8s.io/helm/pkg/helm"`
`11`	`11`
`12`		`- "github.com/databus23/helm-diff/diff"`
`13`		`- "github.com/databus23/helm-diff/manifest"`
	`12`	`+ "github.com/databus23/helm-diff/pkg/diff"`
	`13`	`+ "github.com/databus23/helm-diff/pkg/manifest"`
`14`	`14`	`)`
`15`	`15`
`16`	`16`	`type diffCmd struct {`
`@@ -171,7 +171,7 @@ func (d *diffCmd) run() error {`
`171`	`171`	`}`
`172`	`172`	`}`
`173`	`173`
`174`		`- seenAnyChanges := diff.DiffManifests(currentSpecs, newSpecs, d.suppressedKinds, d.outputContext, os.Stdout)`
	`174`	`+ seenAnyChanges := diff.DifferenceManifests(currentSpecs, newSpecs, d.suppressedKinds, d.outputContext, os.Stdout)`
`175`	`175`
`176`	`176`	`if d.detailedExitCode && seenAnyChanges {`
`177`	`177`	`return Error{`