From 18aad7458febb11c31a4f515874000daacb44a5b Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 20 Jan 2021 10:06:28 +0100 Subject: [PATCH 1/8] Introduce new Makefile targets for sanity checking Add Makefile targets for: - go vet - golint - ineffassign - misspell - staticcheck --- Makefile | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/Makefile b/Makefile index 68bf213732..71c9699843 100644 --- a/Makefile +++ b/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 From 965b0fd0481be78c5dc0ccfa87c060192367e02d Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Mon, 18 Jan 2021 16:26:20 +0100 Subject: [PATCH 2/8] Add missing comments Fix `golint` issues for missing comments of exported functions and constants. --- pkg/apis/core/v1alpha1/condition_types.go | 1 + pkg/controller/buildrun/generate_taskrun.go | 2 ++ pkg/metrics/metrics.go | 1 + version/version.go | 5 ++--- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/apis/core/v1alpha1/condition_types.go b/pkg/apis/core/v1alpha1/condition_types.go index f79f51d9b8..70af986f83 100644 --- a/pkg/apis/core/v1alpha1/condition_types.go +++ b/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 { diff --git a/pkg/controller/buildrun/generate_taskrun.go b/pkg/controller/buildrun/generate_taskrun.go index 17c218edec..7c3c84bed4 100644 --- a/pkg/controller/buildrun/generate_taskrun.go +++ b/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, diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index d19c98b6f0..6482124eef 100644 --- a/pkg/metrics/metrics.go +++ b/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" diff --git a/version/version.go b/version/version.go index 4a3bcc01ef..1a50dec679 100644 --- a/version/version.go +++ b/version/version.go @@ -4,6 +4,5 @@ package version -var ( - Version = "0.0.1" -) +// Version describes the version of build +var Version = "0.0.1" From 664fcf60382363968b6784f5f29f55c7f8ec0a7a Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Mon, 18 Jan 2021 16:30:48 +0100 Subject: [PATCH 3/8] Fix misspell findings Use suggested replacements for `misspell` findings. --- docs/build.md | 2 +- docs/development/testing.md | 2 +- docs/proposals/build-execution-using-build-run.md | 2 +- docs/proposals/buildstrategy-steps-resources.md | 2 +- docs/proposals/remote-artifacts.md | 2 +- pkg/controller/buildrun/buildrun_controller.go | 2 +- test/catalog.go | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/build.md b/docs/build.md index e0f2ceedc6..271385f19c 100644 --- a/docs/build.md +++ b/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 diff --git a/docs/development/testing.md b/docs/development/testing.md index 84e481ecf7..bdfe11bb65 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -95,7 +95,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. diff --git a/docs/proposals/build-execution-using-build-run.md b/docs/proposals/build-execution-using-build-run.md index db9326ac48..bab2da7535 100644 --- a/docs/proposals/build-execution-using-build-run.md +++ b/docs/proposals/build-execution-using-build-run.md @@ -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 ? diff --git a/docs/proposals/buildstrategy-steps-resources.md b/docs/proposals/buildstrategy-steps-resources.md index 66c6614502..2419b23de7 100644 --- a/docs/proposals/buildstrategy-steps-resources.md +++ b/docs/proposals/buildstrategy-steps-resources.md @@ -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 diff --git a/docs/proposals/remote-artifacts.md b/docs/proposals/remote-artifacts.md index 60dc8aaf3b..7cb9d6a521 100644 --- a/docs/proposals/remote-artifacts.md +++ b/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 diff --git a/pkg/controller/buildrun/buildrun_controller.go b/pkg/controller/buildrun/buildrun_controller.go index a009a80056..335ad64043 100644 --- a/pkg/controller/buildrun/buildrun_controller.go +++ b/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 diff --git a/test/catalog.go b/test/catalog.go index f298a14822..4de36cd023 100644 --- a/test/catalog.go +++ b/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, From 2dce71929c7c505d694d25327b2030c79ea8d1bd Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 20 Jan 2021 10:00:33 +0100 Subject: [PATCH 4/8] Fix trailing, duplicate, missing whitesapces Remove trailing whitespaces. Add missing whitespaces between words. Delete duplicate whitespaces. --- docs/proposals/build-execution-using-build-run.md | 4 ++-- docs/proposals/buildstrategy-steps-resources.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/proposals/build-execution-using-build-run.md b/docs/proposals/build-execution-using-build-run.md index bab2da7535..dbae766e8f 100644 --- a/docs/proposals/build-execution-using-build-run.md +++ b/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. @@ -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`. diff --git a/docs/proposals/buildstrategy-steps-resources.md b/docs/proposals/buildstrategy-steps-resources.md index 2419b23de7..e8cb3368a1 100644 --- a/docs/proposals/buildstrategy-steps-resources.md +++ b/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 From 401f68c3e0cce447e1a20a2ba85ebbe2a79f40ce Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Tue, 19 Jan 2021 17:32:00 +0100 Subject: [PATCH 5/8] Remove unused variables Remove unused variables reported by `staticcheck`. --- pkg/controller/controller.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 5d9c5a2af5..3b7ef4deb4 100644 --- a/pkg/controller/controller.go +++ b/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 From 1be85e3b25290632175bea654b63e9b1f2f72e3d Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 20 Jan 2021 11:00:00 +0100 Subject: [PATCH 6/8] Fix ineffassign findings Simplify return code on build controller watch call by combining the `return` statement with the watch call. Add `if err != nil` check in build controller `add` function for the `Watch` that checks primary resource builds. Add log statement in e2e test code where the err was assigned, but not checked. Add `Expect` in e2e test code where err was assigned, but not used. --- pkg/controller/build/build_controller.go | 12 ++++-------- test/e2e/samples.go | 4 ++++ test/integration/buildruns_to_taskruns_test.go | 1 + 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/controller/build/build_controller.go b/pkg/controller/build/build_controller.go index c6b2d18622..bec276eb81 100644 --- a/pkg/controller/build/build_controller.go +++ b/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 diff --git a/test/e2e/samples.go b/test/e2e/samples.go index 4cdd80d824..2ae2f07012 100644 --- a/test/e2e/samples.go +++ b/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 { diff --git a/test/integration/buildruns_to_taskruns_test.go b/test/integration/buildruns_to_taskruns_test.go index c9576e3589..1fbe39be0a 100644 --- a/test/integration/buildruns_to_taskruns_test.go +++ b/test/integration/buildruns_to_taskruns_test.go @@ -353,6 +353,7 @@ var _ = Describe("Integration tests BuildRuns and TaskRuns", func() { Expect(err).To(BeNil()) err = tb.GetBRTillDesiredReason(buildRunObject.Name, fmt.Sprintf("TaskRun \"%s\" was cancelled", tr.Name)) + Expect(err).To(BeNil()) actualReason, err := tb.GetTRTillDesiredReason(buildRunObject.Name, "TaskRunCancelled") Expect(err).To(BeNil(), fmt.Sprintf("failed to get desired reason; expected %s, got %s", "TaskRunCancelled", actualReason)) From 4bc04b2a07bae50f2acbc7cfa40adb0d9d4f0db4 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Wed, 20 Jan 2021 17:39:19 +0100 Subject: [PATCH 7/8] Harmonise function signatures in test code Based on feedback in #538, harmonise function signatures to better match. --- .../integration/buildruns_to_taskruns_test.go | 43 +++++++++++-------- test/integration/utils/buildruns.go | 33 ++++++-------- test/integration/utils/taskruns.go | 33 +++++--------- 3 files changed, 47 insertions(+), 62 deletions(-) diff --git a/test/integration/buildruns_to_taskruns_test.go b/test/integration/buildruns_to_taskruns_test.go index 1fbe39be0a..286d7cd211 100644 --- a/test/integration/buildruns_to_taskruns_test.go +++ b/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,12 +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)) - Expect(err).To(BeNil()) - - 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)) }) }) }) diff --git a/test/integration/utils/buildruns.go b/test/integration/utils/buildruns.go index a8ee1b1e68..df0595a199 100644 --- a/test/integration/utils/buildruns.go +++ b/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 diff --git a/test/integration/utils/taskruns.go b/test/integration/utils/taskruns.go index 67bb02f8e9..5d3bb79fb0 100644 --- a/test/integration/utils/taskruns.go +++ b/test/integration/utils/taskruns.go @@ -63,30 +63,19 @@ func (t *TestBuild) GetTRReason(buildRunName string) (string, error) { // GetTRTillDesiredReason polls until a TaskRun matches a desired Reason // or it exits if an error happen or a timeout is reach. -func (t *TestBuild) GetTRTillDesiredReason(buildRunName string, reason string) (string, error) { - var trReason string - var err error - - var ( - pollTRTillCompletion = func() (bool, error) { - - trReason, err = t.GetTRReason(buildRunName) - if err != nil { - return false, err - } - - if trReason == reason { - return true, nil - } +func (t *TestBuild) GetTRTillDesiredReason(buildRunName string, reason string) (trReason string, err error) { + err = wait.PollImmediate(t.Interval, t.TimeOut, func() (bool, error) { + trReason, err = t.GetTRReason(buildRunName) + if err != nil { + return false, err + } - return false, nil + if trReason == reason { + return true, nil } - ) - pollError := wait.PollImmediate(t.Interval, t.TimeOut, pollTRTillCompletion) - if pollError != nil { - return trReason, pollError - } + return false, nil + }) - return trReason, nil + return } From 23f57630dd3d34964c23c84b835f6690430a3404 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Thu, 21 Jan 2021 12:02:58 +0100 Subject: [PATCH 8/8] Add details about code check to testing doc Introduce section to explain the new Makefile targets. --- docs/development/testing.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/development/testing.md b/docs/development/testing.md index bdfe11bb65..ae0ba028a6 100644 --- a/docs/development/testing.md +++ b/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. @@ -208,4 +218,3 @@ make test-e2e \ TEST_PRIVATE_GITLAB="git@gitlab.com:/.git" \ TEST_SOURCE_SECRET="" ``` -