Add function comments to pkg/actions, tweaks to Makefile and linting

Assuming this is accepted, this is the first in a likely long series of PRs from me refactoring, cleaning up code, adding function comments, gradually adding linter rules to match what we've got in Pipeline along with addressing issues those linter rules find, etc.

Signed-off-by: Andrew Bayer <[email protected]>

.gitignore

@@ -45,3 +45,5 @@ tags
 
 # Chocolatey
 *.nupkg
+
+.bin

.golangci.yml

@@ -1,4 +1,5 @@
 run:
+  issues-exit-code: 1
   build-tags:
     - e2e
   skip-dirs:
@@ -13,6 +14,6 @@ linters:
   - goimports
   - gosec
   - gocritic
-  - golint
+  - revive
   - misspell
   - deadcode

Makefile

@@ -1,3 +1,18 @@
+GO   = go
+PKGS = $(or $(PKG),$(shell env GO111MODULE=on $(GO) list ./... | grep -v 'github\.com\/tektoncd\/cli\/third_party\/'))
+BIN  = $(CURDIR)/.bin
+
+export GO111MODULE=on
+
+V = 0
+Q = $(if $(filter 1,$V),,@)
+M = $(shell printf "\033[34;1m🐱\033[0m")
+
+TIMEOUT_UNIT = 5m
+TIMEOUT_E2E  = 20m
+
+GOLANGCI_VERSION = v1.42.0
+
 YAML_FILES := $(shell find . -type f -regex ".*y[a]ml" -print)
 
 ifneq ($(NAMESPACE),)
@@ -35,7 +50,7 @@ FORCE:
 
 .PHONY: vendor
 vendor:
-	@go mod vendor
+	@$(GO) mod vendor
 
 .PHONY: cross
 cross: amd64 386 arm arm64 s390x ppc64le ## build cross platform binaries
@@ -69,7 +84,7 @@ ppc64le:
 	GOOS=linux GOARCH=ppc64le go build -mod=vendor $(LDFLAGS) -o bin/tkn-linux-ppc64le ./cmd/tkn
 
 bin/%: cmd/% FORCE
-	go build -mod=vendor $(LDFLAGS) -v -o $@ ./$<
+	$(Q) $(GO) build -mod=vendor $(LDFLAGS) -v -o $@ ./$<
 
 check: lint test
 
@@ -79,37 +94,45 @@ test: test-unit ## run all tests
 .PHONY: lint
 lint: lint-go lint-yaml ## run all linters
 
+GOLANGCILINT = $(BIN)/golangci-lint
+$(BIN)/golangci-lint: ; $(info $(M) getting golangci-lint $(GOLANGCI_VERSION))
+	cd tools; GOBIN=$(BIN) $(GO) install -mod=mod github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_VERSION)
+
 .PHONY: lint-go
-lint-go: ## runs go linter on all go files
-	@echo "Linting go files..."
-	@golangci-lint run ./... --modules-download-mode=vendor \
-							--max-issues-per-linter=0 \
-							--max-same-issues=0 \
-							--deadline 5m
+lint-go: | $(GOLANGCILINT) ; $(info $(M) running golangci-lint…) @ ## Run golangci-lint
+	$Q $(GOLANGCILINT) run --modules-download-mode=vendor --max-issues-per-linter=0 --max-same-issues=0 --deadline 5m
+
+GOIMPORTS = $(BIN)/goimports
+$(BIN)/goimports: PACKAGE=golang.org/x/tools/cmd/goimports
+
+.PHONY: goimports
+goimports: | $(GOIMPORTS) ; $(info $(M) running goimports…) ## Run goimports
+	$Q $(GOIMPORTS) -l -e -w pkg cmd test
+
 
 .PHONY: lint-yaml
-lint-yaml: ${YAML_FILES} ## runs yamllint on all yaml files
-	@echo "Linting yaml files..."
+lint-yaml: ${YAML_FILES}  ; $(info $(M) running yamllint…) ## runs yamllint on all yaml files
 	@yamllint -c .yamllint $(YAML_FILES)
 
-.PHONY: test-unit
-test-unit: ./vendor ## run unit tests
-	@echo "Running unit tests..."
-	@go test ./pkg/... ./cmd/... -failfast -v -cover
+## Tests
+TEST_UNIT_TARGETS := test-unit-verbose test-unit-race
+test-unit-verbose: ARGS=-v
+test-unit-race:    ARGS=-race
+$(TEST_UNIT_TARGETS): test-unit
+.PHONY: $(TEST_UNIT_TARGETS) test-unit
+test-unit: ; $(info $(M) running unit tests…) ## Run unit tests
+	$(GO) test -timeout $(TIMEOUT_UNIT) $(ARGS) $(shell go list ./... | grep -v third_party/)
 
 .PHONY: update-golden
-update-golden: ./vendor ## run unit tests (updating golden files)
-	@echo "Running unit tests to update golden files..."
+update-golden: ./vendor ; $(info $(M) Running unit tests to update golden files…) ## run unit tests (updating golden files)
 	@./hack/update-golden.sh
 
 .PHONY: test-e2e
-test-e2e: bin/tkn ## run e2e tests
-	@echo "Running e2e tests..."
+test-e2e: bin/tkn ; $(info $(M) Running e2e tests…) ## run e2e tests
 	@LOCAL_CI_RUN=true bash ./test/e2e-tests.sh
 
 .PHONY: docs
-docs: bin/docs ## update docs
-	@echo "Generating docs"
+docs: bin/docs ; $(info $(M) Generating docs…) ## update docs
 	@mkdir -p ./docs/cmd ./docs/man/man1
 	@./bin/docs --target=./docs/cmd
 	@./bin/docs --target=./docs/man/man1 --kind=man
@@ -123,8 +146,8 @@ clean: ## clean build artifacts
 	rm -fR bin VERSION
 
 .PHONY: fmt ## formats the GO code(excludes vendors dir)
-fmt:
-	@go fmt `go list ./... | grep -v /vendor/`
+fmt: ; $(info $(M) running gofmt…) @ ## Run gofmt on all source files
+	$Q $(GO) fmt $(PKGS)
 
 .PHONY: help
 help: ## print this help

pkg/actions/create.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
+// Create is used to take a partial resource and an unstructured object and create it in the cluster using the dynamic client.
 func Create(gr schema.GroupVersionResource, clients *cli.Clients, object *unstructured.Unstructured, ns string, op metav1.CreateOptions) (*unstructured.Unstructured, error) {
 	gvr, err := GetGroupVersionResource(gr, clients.Tekton.Discovery())
 	if err != nil {

pkg/actions/delete.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/client-go/dynamic"
 )
 
+// Delete is used to take a partial resource and the name of an object in the cluster and delete it using the dynamic client.
 func Delete(gr schema.GroupVersionResource, dynamic dynamic.Interface, discovery discovery.DiscoveryInterface, objname, ns string, op metav1.DeleteOptions) error {
 	gvr, err := GetGroupVersionResource(gr, discovery)
 	if err != nil {

pkg/actions/get.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/client-go/dynamic"
 )
 
+// PrintObject is used to take a partial resource and the name of an object in the cluster, fetch it using the dynamic client, and print out the object.
 func PrintObject(groupResource schema.GroupVersionResource, obj string, w io.Writer, dynamic dynamic.Interface, discovery discovery.DiscoveryInterface, f *cliopts.PrintFlags, ns string) error {
 	res, err := Get(groupResource, dynamic, discovery, obj, ns, metav1.GetOptions{})
 	if err != nil {
@@ -36,6 +37,7 @@ func PrintObject(groupResource schema.GroupVersionResource, obj string, w io.Wri
 	return printer.PrintObject(w, res, f)
 }
 
+// Get is used to take a partial resource and the name of an object in the cluster and fetch it from the cluster using the dynamic client.
 func Get(gr schema.GroupVersionResource, dynamic dynamic.Interface, discovery discovery.DiscoveryInterface, objname, ns string, op metav1.GetOptions) (*unstructured.Unstructured, error) {
 	gvr, err := GetGroupVersionResource(gr, discovery)
 	if err != nil {

pkg/actions/gvr.go

@@ -27,6 +27,7 @@ var (
 	apiGroupRes []*restmapper.APIGroupResources
 )
 
+// GetGroupVersionResource takes a partial resource, and returns the full resource matching the partial resource, if there's only one match.
 func GetGroupVersionResource(gr schema.GroupVersionResource, discovery discovery.DiscoveryInterface) (*schema.GroupVersionResource, error) {
 	var err error
 	doOnce.Do(func() {
@@ -45,6 +46,7 @@ func GetGroupVersionResource(gr schema.GroupVersionResource, discovery discovery
 	return &gvr, nil
 }
 
+// InitializeAPIGroupRes initializes and populates the discovery client.
 func InitializeAPIGroupRes(discovery discovery.DiscoveryInterface) error {
 	var err error
 	apiGroupRes, err = restmapper.GetAPIGroupResources(discovery)

pkg/actions/list.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/client-go/dynamic"
 )
 
+// PrintObjects takes a partial resource, fetches a list of that resource's objects in the cluster using the dynamic client, and prints out the objects.
 func PrintObjects(groupResource schema.GroupVersionResource, w io.Writer, dynamic dynamic.Interface, discovery discovery.DiscoveryInterface, f *cliopts.PrintFlags, ns string) error {
 	allres, err := List(groupResource, dynamic, discovery, ns, metav1.ListOptions{})
 	if err != nil {
@@ -36,6 +37,7 @@ func PrintObjects(groupResource schema.GroupVersionResource, w io.Writer, dynami
 	return printer.PrintObject(w, allres, f)
 }
 
+// List takes a partial resource and fetches a list of that resource's objects in the cluster using the dynamic client.
 func List(gr schema.GroupVersionResource, dynamic dynamic.Interface, discovery discovery.DiscoveryInterface, ns string, op metav1.ListOptions) (*unstructured.UnstructuredList, error) {
 	gvr, err := GetGroupVersionResource(gr, discovery)
 	if err != nil {

pkg/actions/patch.go

@@ -24,6 +24,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 )
 
+// Patch takes a partial resource, an object name in the cluster, and patch data to be applied to that object, and patches the object using the dynamic client.
 func Patch(gr schema.GroupVersionResource, clients *cli.Clients, objName string, data []byte, opt metav1.PatchOptions, ns string) (*unstructured.Unstructured, error) {
 	gvr, err := GetGroupVersionResource(gr, clients.Tekton.Discovery())
 	if err != nil {

pkg/actions/watch.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/watch"
 )
 
+// Watch takes a partial resource, and returns a watcher interface for that resource using the dynamic client
 func Watch(gr schema.GroupVersionResource, clients *cli.Clients, ns string, op metav1.ListOptions) (watch.Interface, error) {
 	gvr, err := GetGroupVersionResource(gr, clients.Tekton.Discovery())
 	if err != nil {

pkg/flags/flags.go

@@ -74,12 +74,12 @@ func AddTektonOptions(cmd *cobra.Command) {
 // GetTektonOptions get the global tekton Options that are not passed to a subcommands
 func GetTektonOptions(cmd *cobra.Command) TektonOptions {
 	kcPath, _ := cmd.Flags().GetString(kubeConfig)
-	kContext, _ := cmd.Flags().GetString(context)
+	kubeContext, _ := cmd.Flags().GetString(context)
 	ns, _ := cmd.Flags().GetString(namespace)
 	nocolourFlag, _ := cmd.Flags().GetBool(nocolor)
 	return TektonOptions{
 		KubeConfig: kcPath,
-		Context:    kContext,
+		Context:    kubeContext,
 		Namespace:  ns,
 		Nocolour:   nocolourFlag,
 	}
@@ -97,11 +97,11 @@ func InitParams(p cli.Params, cmd *cobra.Command) error {
 	}
 	p.SetKubeConfigPath(kcPath)
 
-	kContext, err := cmd.Flags().GetString(context)
+	kubeContext, err := cmd.Flags().GetString(context)
 	if err != nil {
 		return err
 	}
-	p.SetKubeContext(kContext)
+	p.SetKubeContext(kubeContext)
 
 	// ensure that the config is valid by creating a client
 	if _, err := p.Clients(); err != nil {

pkg/log/task_reader.go

@@ -20,7 +20,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/pkg/errors"
 	"github.com/tektoncd/cli/pkg/pods"
 	tr "github.com/tektoncd/cli/pkg/taskrun"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
@@ -198,7 +197,7 @@ func (r *Reader) readPodLogs(podC <-chan string, podErrC <-chan error, follow bo
 				pod, err = p.Get()
 			}
 			if err != nil {
-				errC <- errors.New(fmt.Sprintf("task %s failed: %s. Run tkn tr desc %s for more details.", r.task, strings.TrimSpace(err.Error()), r.run))
+				errC <- fmt.Errorf("task %s failed: %s. Run tkn tr desc %s for more details", r.task, strings.TrimSpace(err.Error()), r.run)
 			}
 			steps := filterSteps(pod, r.allSteps, r.steps)
 			r.readStepsLogs(logC, errC, steps, p, follow)

pkg/test/params.go

@@ -27,14 +27,14 @@ import (
 )
 
 type Params struct {
-	ns, kConfig, kContext string
-	Tekton                versioned.Interface
-	Resource              versionedResource.Interface
-	Triggers              versionedTriggers.Interface
-	Kube                  k8s.Interface
-	Clock                 clockwork.Clock
-	Cls                   *cli.Clients
-	Dynamic               dynamic.Interface
+	ns, kubeCfg, kubeCtx string
+	Tekton               versioned.Interface
+	Resource             versionedResource.Interface
+	Triggers             versionedTriggers.Interface
+	Kube                 k8s.Interface
+	Clock                clockwork.Clock
+	Cls                  *cli.Clients
+	Dynamic              dynamic.Interface
 }
 
 var _ cli.Params = &Params{}
@@ -50,15 +50,15 @@ func (p *Params) SetNoColour(b bool) {
 }
 
 func (p *Params) SetKubeConfigPath(path string) {
-	p.kConfig = path
+	p.kubeCfg = path
 }
 
 func (p *Params) SetKubeContext(context string) {
-	p.kContext = context
+	p.kubeCtx = context
 }
 
 func (p *Params) KubeConfigPath() string {
-	return p.kConfig
+	return p.kubeCfg
 }
 
 func (p *Params) tektonClient() (versioned.Interface, error) {

test/README.md

@@ -4,7 +4,9 @@ To run tests:
 
 ```shell
 # Unit tests
-go test ./...
+go test $(go list ./... | grep -v third_party/)
+# or
+make test-unit
 
 # Integration tests (against your current kube cluster)
 go test -v -count=1 -tags=e2e ./test/...
@@ -15,7 +17,9 @@ go test -v -count=1 -tags=e2e ./test/...
 Unit tests live side by side with the code they are testing and can be run with:
 
 ```shell
-go test ./...
+go test $(go list ./... | grep -v third_party/)
+# or
+make test-unit
 ```
 
 By default `go test` will not run [the end to end tests](#end-to-end-tests),

test/presubmit-tests.sh

@@ -114,7 +114,7 @@ function build_tests() {
   # For documentation PRs, just check the md files
   (( IS_DOCUMENTATION_PR )) && return ${failed}
   # Skip build test if there is no go code
-  local go_pkg_dirs="$(go list ./...)"
+  local go_pkg_dirs="$(go list ./... |grep -v third_party/)"
   [[ -z "${go_pkg_dirs}" ]] && return ${failed}
   # Ensure all the code builds
   subheader "Checking that go code builds"