Merge pull request #538 from HeavyWombat/fix/go-card-warnings

Fix some of the Go Report Card findings

Makefile

@@ -152,6 +152,39 @@ install-counterfeiter:
 install-operator-sdk:
 	hack/install-operator-sdk.sh
 
+.PHONY: govet
+govet:
+	@echo "Checking go vet"
+	@go vet ./...
+
+# Install it via: go get -u github.com/gordonklaus/ineffassign
+.PHONY: ineffassign
+ineffassign:
+	@echo "Checking ineffassign"
+	@ineffassign ./...
+
+# Install it via: go get -u golang.org/x/lint/golint
+# See https://github.com/golang/lint/issues/320 for details regarding the grep
+.PHONY: golint
+golint:
+	@echo "Checking golint"
+	@go list ./... | grep -v -e /vendor -e /test | xargs -L1 golint -set_exit_status
+
+# Install it via: go get -u github.com/client9/misspell/cmd/misspell
+.PHONY: misspell
+misspell:
+	@echo "Checking misspell"
+	@find . -type f | grep -v /vendor | xargs misspell -source=text -error
+
+# Install it via: go get -u honnef.co/go/tools/cmd/staticcheck
+.PHONY: staticcheck
+staticcheck:
+	@echo "Checking staticcheck"
+	@go list ./... | grep -v /test | xargs staticcheck
+
+.PHONY: sanity-check
+sanity-check: ineffassign golint govet misspell staticcheck
+
 # https://github.com/shipwright-io/build/issues/123
 test: test-unit
 

docs/build.md

@@ -71,7 +71,7 @@ A `Build` resource can specify a Git source, together with other parameters like
 
 By default, the Build controller will validate that the Git repository exists. If the validation is not desired, users can define the `build.build.dev/verify.repository` annotation with `false`. For example:
 
-Example of a `Build` with the **build.build.dev/verify.repository** annotation, in order to disbale the `spec.source.url` validation.
+Example of a `Build` with the **build.build.dev/verify.repository** annotation, in order to disable the `spec.source.url` validation.
 
 ```yaml
 apiVersion: build.dev/v1alpha1

docs/development/testing.md

@@ -45,6 +45,16 @@ This allow us to use test doubles in the unit tests, from where we can instantia
 
 Counterfeiter is required by the code generator scripts. Run `make install-counterfeiter` to add counterfeiter to your `GOPATH`.
 
+### Static code analysis and linting
+
+Run `make sanity-check` to run a collection of static code analyser and linters to check the code for issues, for example ineffective assignments, unused variables, missing comments, misspellings and so on. Each check also has an individual Make target to check:
+
+- `make govet` examines Go source code and reports suspicious constructs
+- `make ineffassign` checks Go source for variable assignments that are not used (i.e. overridden)
+- `make golint` runs a linter against the Go source
+- `make misspell` checks for TYPOs
+- `make staticcheck` performs more complex static code analysis to find unused code and other issues
+
 ## Unit Tests
 
 We use unit tests to provide coverage and ensure that our functions are behaving as expected, but also to assert the behaviour of the controllers during Reconciliations.
@@ -95,7 +105,7 @@ make test-integration
 
 ## E2E Tests
 
-We use e2e tests as the last signal to ensure the controllers behaviour in the cluster matches the developer specifications( _based on [e2e-tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/e2e-tests.md)_ ). During e2e tests execution, we don´t want to test any interaction between components but rather we want to simulate a normal user operation and ensure that images are succesfully build. E2E tests should only cover:
+We use e2e tests as the last signal to ensure the controllers behaviour in the cluster matches the developer specifications( _based on [e2e-tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/e2e-tests.md)_ ). During e2e tests execution, we don´t want to test any interaction between components but rather we want to simulate a normal user operation and ensure that images are successfully build. E2E tests should only cover:
 
 - As the way to validate if the image was successfully build, only assert for a Succeeded Status on TaskRuns.
 - Testing should be around building images with different supported strategies, and different runtimes inside the strategies.
@@ -208,4 +218,3 @@ make test-e2e \
   TEST_PRIVATE_GITLAB="[email protected]:<youruser>/<your-repo>.git" \
   TEST_SOURCE_SECRET="<secret-name>"
 ```
-

docs/proposals/build-execution-using-build-run.md

@@ -34,7 +34,7 @@ spec:
     name: kaniko-golang-build
 ```
 
-The `BuildRun` API also provides a way to override/specify  execution time information. 
+The `BuildRun` API also provides a way to override/specify execution time information.
 
 As an example, a `Build` configuration should not have to accurately specify the resource requirements - the information on resource requirements is valuable at build execution time which varies from environment to environment.
 
@@ -73,7 +73,7 @@ As an example, the source code [nodejs-rest-http-crud](https://github.com/nodesh
 Note:
 In some scenarios, the source code 'revision' may not necessarily be deterministic as well, especially in case of builds triggered from PRs/forks.
 
-### Deciding what should be overriden
+### Deciding what should be overridden
 
 - Could the value of attribute X be reasonably determined at the time of `Build` configuration specification ?
 - Could the modification of attribute X reasonably compromise the integrity of the build ?
@@ -83,7 +83,7 @@ The above is a non-exhaustive list of questions that should help us modify the B
 
 ### Next steps / Consequences
 
-Here's what it means to adopt this proposal: 
+Here's what it means to adopt this proposal:
 
 1. Make `spec.output` optional in `Build` API's `spec` .
 2. Introduce `spec.output` in the `BuildRun` API's `spec`.

docs/proposals/buildstrategy-steps-resources.md

@@ -54,8 +54,8 @@ there could be situations where containers under the same strategy required diff
 
 ## Motivation
 
-For strategies with multiple steps like Buildpacks, not all the steps(_containers_) will require the same set of resources(_e.g. memory, cpu, etc_) values. 
-In order to be able to manage situations where setting particular steps resources is required, we need a more flexible way to deal with N number of steps under the same strategy (_Build/ClusterBuildStrategy_). Also, we still need to help users to know that independently of the abstraction of the strategies, there are options for speeding up the builds. This options could be presented in the form of different `flavours` of the same strategy(_cluster or namespaced scope_), that differ between each other in terms of container resources values.
+For strategies with multiple steps like Buildpacks, not all the steps (_containers_) will require the same set of resources (_e.g. memory, cpu, etc_) values.
+In order to be able to manage situations where setting particular steps resources is required, we need a more flexible way to deal with N number of steps under the same strategy (_Build/ClusterBuildStrategy_). Also, we still need to help users to know that independently of the abstraction of the strategies, there are options for speeding up the builds. This options could be presented in the form of different `flavours` of the same strategy (_cluster or namespaced scope_), that differ between each other in terms of container resources values.
 
 ### Goals
 
@@ -256,7 +256,7 @@ spec:
 ### Risks and Mitigations
 
 Proper documentation needs to be made to communicate the existence of `flavours` for strategies of the same type to users. When it comes to UI, is the decision of the UI team to decide
-how they will expose(_interface_) this flavours to their end-users. Strategy admins have full responsability on the flavours they decide to define, but recommendations of flavours should be
+how they will expose(_interface_) this flavours to their end-users. Strategy admins have full responsibility on the flavours they decide to define, but recommendations of flavours should be
 made for this repository.
 
 ## Design Details

docs/proposals/remote-artifacts.md

@@ -103,7 +103,7 @@ can extend the API to support arbitrary configuration.
 For the initial implementation, we will only support one git repository that is specified in
 `.spec.source`. Remote artifacts will be specified n `.spec.sources` only.
 
-We will address supporting multipe Git repositories in a future enhancement proposal. This will
+We will address supporting multiple Git repositories in a future enhancement proposal. This will
 involve re-defining `/workspace/source` location and the `$workspace` placeholder.
 
 ## Steps and Helper

pkg/apis/core/v1alpha1/condition_types.go

@@ -127,6 +127,7 @@ type Status struct {
 	Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
 }
 
+// GetCondition returns the condition based on the type
 func (s *Status) GetCondition(t ConditionType) *Condition {
 	for _, cond := range s.Conditions {
 		if cond.Type == t {

pkg/controller/build/build_controller.go

@@ -101,9 +101,11 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error
 
 	// Watch for changes to primary resource Build
 	err = c.Watch(&source.Kind{Type: &build.Build{}}, &handler.EnqueueRequestForObject{}, pred)
+	if err != nil {
+		return err
+	}
 
 	preSecret := predicate.Funcs{
-
 		// Only filter events where the secret have the Build specific annotation
 		CreateFunc: func(e event.CreateEvent) bool {
 			objectAnnotations := e.Meta.GetAnnotations()
@@ -137,7 +139,7 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error
 		},
 	}
 
-	err = c.Watch(&source.Kind{Type: &corev1.Secret{}}, &handler.EnqueueRequestsFromMapFunc{
+	return c.Watch(&source.Kind{Type: &corev1.Secret{}}, &handler.EnqueueRequestsFromMapFunc{
 		ToRequests: handler.ToRequestsFunc(func(o handler.MapObject) []reconcile.Request {
 
 			secret := o.Object.(*corev1.Secret)
@@ -190,12 +192,6 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error
 			return reconcileList
 		}),
 	}, preSecret)
-
-	if err != nil {
-		return err
-	}
-
-	return nil
 }
 
 // blank assignment to verify that ReconcileBuild implements reconcile.Reconciler

pkg/controller/buildrun/buildrun_controller.go

@@ -306,7 +306,7 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 				if err = r.client.Update(ctx, buildRun); err != nil {
 					return reconcile.Result{}, err
 				}
-				ctxlog.Info(ctx, fmt.Sprintf("succesfully updated BuildRun %s", buildRun.Name), namespace, request.Namespace, name, request.Name)
+				ctxlog.Info(ctx, fmt.Sprintf("successfully updated BuildRun %s", buildRun.Name), namespace, request.Namespace, name, request.Name)
 			}
 
 			// Set the Build spec in the BuildRun status

pkg/controller/buildrun/generate_taskrun.go

@@ -51,6 +51,7 @@ func getStringTransformations(fullText string) string {
 	return fullText
 }
 
+// GenerateTaskSpec creates Tekton TaskRun spec to be used for a build run
 func GenerateTaskSpec(
 	cfg *config.Config,
 	build *buildv1alpha1.Build,
@@ -173,6 +174,7 @@ func GenerateTaskSpec(
 	return &generatedTaskSpec, nil
 }
 
+// GenerateTaskRun creates a Tekton TaskRun to be used for a build run
 func GenerateTaskRun(
 	cfg *config.Config,
 	build *buildv1alpha1.Build,

pkg/controller/controller.go

@@ -17,12 +17,6 @@ import (
 	"github.com/shipwright-io/build/pkg/ctxlog"
 )
 
-var (
-	metricsHost               = "0.0.0.0"
-	metricsPort         int32 = 8383
-	operatorMetricsPort int32 = 8686
-)
-
 // AddToManagerFuncs is a list of functions to add all Controllers to the Manager
 var AddToManagerFuncs []func(context.Context, *config.Config, manager.Manager) error
 

pkg/metrics/metrics.go

@@ -14,6 +14,7 @@ import (
 	"github.com/shipwright-io/build/pkg/config"
 )
 
+// Labels used in Prometheus metrics
 const (
 	BuildStrategyLabel string = "buildstrategy"
 	NamespaceLabel     string = "namespace"

test/catalog.go

@@ -297,7 +297,7 @@ func (c *Catalog) StubBuildUpdateOwnerReferences(ownerKind string, ownerName str
 	}
 }
 
-// StubBuildRun is used to simulate the existance of a BuildRun
+// StubBuildRun is used to simulate the existence of a BuildRun
 // only when there is a client GET on this object type
 func (c *Catalog) StubBuildRun(
 	b *build.BuildRun,

test/e2e/samples.go

@@ -125,6 +125,10 @@ func printTestFailureDebugInfo(namespace string, buildRunName string) {
 	f := framework.Global
 
 	buildRun, build, err := retrieveBuildAndBuildRun(namespace, buildRunName)
+	if err != nil {
+		Logf("Failed to retrieve build and buildrun logs: %w", err)
+	}
+
 	if build != nil {
 		Logf("The status of Build %s: registered=%s, reason=%s", build.Name, build.Status.Registered, build.Status.Reason)
 		if buildJSON, err := json.Marshal(build); err == nil {

test/integration/buildruns_to_taskruns_test.go

@@ -250,18 +250,21 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() {
 			_, err = tb.GetBRTillStartTime(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, "Pending")
-			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", "Pending", actualReason))
+			expectedReason := "Pending"
+			actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
-			err = tb.GetBRTillDesiredReason(buildRunObject.Name, "Pending")
-			Expect(err).To(BeNil())
-
-			actualReason, err = tb.GetTRTillDesiredReason(buildRunObject.Name, "Running")
-			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", "Running", actualReason))
+			expectedReason = "Pending"
+			actualReason, err = tb.GetBRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
-			err = tb.GetBRTillDesiredReason(buildRunObject.Name, "Running")
-			Expect(err).To(BeNil())
+			expectedReason = "Running"
+			actualReason, err = tb.GetTRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
+			expectedReason = "Running"
+			actualReason, err = tb.GetBRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 		})
 	})
 
@@ -281,19 +284,20 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() {
 			_, err = tb.GetBRTillCompletion(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, "TaskRunTimeout")
-			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", "TaskRunTimeout", actualReason))
+			expectedReason := "TaskRunTimeout"
+			actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
 			tr, err := tb.GetTaskRunFromBuildRun(buildRunObject.Name)
 			Expect(err).To(BeNil())
 
-			err = tb.GetBRTillDesiredReason(buildRunObject.Name, fmt.Sprintf("TaskRun \"%s\" failed to finish within \"5s\"", tr.Name))
-			Expect(err).To(BeNil())
+			expectedReason = fmt.Sprintf("TaskRun \"%s\" failed to finish within \"5s\"", tr.Name)
+			actualReason, err = tb.GetBRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
 			tr, err = tb.GetTaskRunFromBuildRun(buildRunObject.Name)
 			Expect(err).To(BeNil())
 			Expect(tr.Status.CompletionTime).ToNot(BeNil())
-
 		})
 	})
 
@@ -352,11 +356,13 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() {
 			tr, err = tb.UpdateTaskRun(tr)
 			Expect(err).To(BeNil())
 
-			err = tb.GetBRTillDesiredReason(buildRunObject.Name, fmt.Sprintf("TaskRun \"%s\" was cancelled", tr.Name))
-
-			actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, "TaskRunCancelled")
-			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", "TaskRunCancelled", actualReason))
+			expectedReason := fmt.Sprintf("TaskRun \"%s\" was cancelled", tr.Name)
+			actualReason, err := tb.GetBRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 
+			expectedReason = "TaskRunCancelled"
+			actualReason, err = tb.GetTRTillDesiredReason(buildRunObject.Name, expectedReason)
+			Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", expectedReason, actualReason))
 		})
 	})
 })

test/integration/utils/buildruns.go

@@ -123,30 +123,21 @@ func (t *TestBuild) GetBRTillStartTime(name string) (*v1alpha1.BuildRun, error)
 }
 
 // GetBRTillDesiredReason polls until a BuildRun gets a particular Reason
-// it exit if an error happens or the timeout is reach
-func (t *TestBuild) GetBRTillDesiredReason(buildRunname string, reason string) error {
-
-	var (
-		pollBRTillCompletion = func() (bool, error) {
-
-			currentReason, err := t.GetBRReason(buildRunname)
-			if err != nil {
-				return false, err
-			}
-			if currentReason == reason {
-				return true, nil
-			}
-
-			return false, nil
+// it exit if an error happens or the timeout is reached
+func (t *TestBuild) GetBRTillDesiredReason(buildRunname string, reason string) (currentReason string, err error) {
+	err = wait.PollImmediate(t.Interval, t.TimeOut, func() (bool, error) {
+		currentReason, err = t.GetBRReason(buildRunname)
+		if err != nil {
+			return false, err
+		}
+		if currentReason == reason {
+			return true, nil
 		}
-	)
 
-	err := wait.PollImmediate(t.Interval, t.TimeOut, pollBRTillCompletion)
-	if err != nil {
-		return err
-	}
+		return false, nil
+	})
 
-	return nil
+	return
 }
 
 // GetBRTillDeletion polls until a BuildRun is not found, it returns