Minimize BuildRun Update calls

Logic was updating two times the BuildRun, introduce a variable to only update once
shipwright-io · Sep 25, 2020 · 17150eb · 17150eb
1 parent a628f5c
commit 17150eb
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 12 deletions.
diff --git a/pkg/controller/buildrun/buildrun_controller.go b/pkg/controller/buildrun/buildrun_controller.go
@@ -240,6 +240,8 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 	var buildRun *buildv1alpha1.BuildRun
 	var build *buildv1alpha1.Build
 
+	updateBuildRun := false
+
 	// Set the ctx to be Background, as the top-level context for incoming requests.
 	ctx, cancel := context.WithTimeout(r.ctx, r.config.CtxTimeOut)
 	defer cancel()
@@ -278,35 +280,32 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 				buildRun.Labels = make(map[string]string)
 			}
 
-			// TODOS:
-			// add documentation on Build STATUS reflecting ownership failures -> NOT DONE
-			// consolidate client updates for labels and ownerreferences
-
 			// Set OwnerReference for Build and BuildRun only when build.build.dev/build-run-deletion is set "true"
-			if build.GetAnnotations()[buildv1alpha1.AnnotationBuildRunDeletion] == "true" {
+			if build.GetAnnotations()[buildv1alpha1.AnnotationBuildRunDeletion] == "true" && len(buildRun.OwnerReferences) == 0 {
 				if err := r.setOwnerReferenceFunc(build, buildRun, r.scheme); err != nil {
 					build.Status.Reason = err.Error()
 					err := r.client.Status().Update(ctx, build)
 					if err != nil {
 						return reconcile.Result{}, err
 					}
-
-					// We dont wanna reconcile again because of https://github.com/shipwright-io/build/pull/371#discussion_r484109591
-					// return reconcile.Result{}, handleError("Failed to set OwnerReference for Build and BuildRun", err, updateErr)
-				}
-				if err = r.client.Update(ctx, buildRun); err != nil {
-					return reconcile.Result{}, err
 				}
+				ctxlog.Info(ctx, fmt.Sprintf("updating BuildRun %s OwnerReferences, owner is Build %s", buildRun.Name, build.Name), namespace, request.Namespace, name, request.Name)
+				updateBuildRun = true
 			}
 
 			buildGeneration := strconv.FormatInt(build.Generation, 10)
 			if buildRun.GetLabels()[buildv1alpha1.LabelBuild] != build.Name || buildRun.GetLabels()[buildv1alpha1.LabelBuildGeneration] != buildGeneration {
 				buildRun.Labels[buildv1alpha1.LabelBuild] = build.Name
 				buildRun.Labels[buildv1alpha1.LabelBuildGeneration] = buildGeneration
 				ctxlog.Info(ctx, "updating BuildRun labels", namespace, request.Namespace, name, request.Name)
+				updateBuildRun = true
+			}
+
+			if updateBuildRun {
 				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)
 			}
 
 			// Set the Build spec in the BuildRun status

diff --git a/pkg/controller/buildrun/buildrun_controller_test.go b/pkg/controller/buildrun/buildrun_controller_test.go
@@ -649,7 +649,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 
 				_, err := reconciler.Reconcile(buildRunRequest)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal(fmt.Sprintf("The Build is not yet validated, build: %s", buildName)))
+				Expect(err.Error()).To(Equal(fmt.Sprintf("the Build is not yet validated, build: %s", buildName)))
 				Expect(client.StatusCallCount()).To(Equal(0))
 			})