[RHTAPBUGS-442 Redux] Ensure gitops repository is cleaned up in error states

controllers/applicationsnapshotenvironmentbinding_controller.go

@@ -168,6 +168,7 @@
 		if err != nil {
 			log.Error(err, fmt.Sprintf("unable to get the Component %s %v", componentName, req.NamespacedName))
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			return ctrl.Result{}, err
 		}
 
@@ -179,6 +180,7 @@
 		if hasComponent.Spec.Application != applicationName {
 			err := fmt.Errorf("component %s does not belong to the application %s", componentName, applicationName)
 			log.Error(err, "")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
@@ -194,6 +196,7 @@
 		if isKubernetesCluster && clusterIngressDomain == "" {
 			err = fmt.Errorf("ingress domain cannot be empty on a Kubernetes cluster")
 			log.Error(err, "unable to create an ingress resource on a Kubernetes cluster")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
@@ -205,46 +208,50 @@
 		if err != nil {
 			errMsg := fmt.Sprintf("Unable to parse the devfile from Component status, exiting reconcile loop %v", req.NamespacedName)
 			log.Error(err, errMsg)
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, fmt.Errorf("%v: %v", errMsg, err))
 			return ctrl.Result{}, err
 		}
 
 		deployAssociatedComponents, err := devfileParser.GetDeployComponents(compDevfileData)
 		if err != nil {
 			log.Error(err, "unable to get deploy components")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
 
 		var hostname string
 		if isKubernetesCluster {
 			hostname, err = devfile.GetIngressHostName(hasComponent.Name, appSnapshotEnvBinding.Namespace, clusterIngressDomain)
 			if err != nil {
 				log.Error(err, fmt.Sprintf("unable to get generate a host name from an ingress domain for %s %v", hasComponent.Name, req.NamespacedName))
+				ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 				r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 				return ctrl.Result{}, err
 			}
 		}
 
 		// Generate a route name for the component
 
 		kubernetesResources, err := devfile.GetResourceFromDevfile(log, compDevfileData, deployAssociatedComponents, hasComponent.Name, hasComponent.Spec.Application, hasComponent.Spec.ContainerImage, hostname)
 		if err != nil {
 			log.Error(err, "unable to get kubernetes resources from the devfile outerloop components")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
 
 		// Create a random, generated name for the route
 		// ToDo: Ideally we wouldn't need to loop here, but since the Component status is a list, we can't avoid it
 		var routeName string
 		for _, compStatus := range appSnapshotEnvBinding.Status.Components {
 			if compStatus.Name == componentName {
 				if compStatus.GeneratedRouteName != "" {
 					routeName = compStatus.GeneratedRouteName
 					log.Info(fmt.Sprintf("route name for component is %s", routeName))
 				}
 				break
 			}
 		}
 		if routeName == "" {
@@ -269,13 +276,15 @@
 		if imageName == "" {
 			err := fmt.Errorf("application snapshot %s did not reference component %s", snapshotName, componentName)
 			log.Error(err, "")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
 
 		gitOpsRemoteURL, gitOpsBranch, gitOpsContext, err := util.ProcessGitOpsStatus(hasComponent.Status.GitOps, ghClient.Token)
 		if err != nil {
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			return ctrl.Result{}, err
 		}
 
@@ -347,7 +356,7 @@
 		err = r.Generator.GenerateOverlaysAndPush(tempDir, clone, gitOpsRemoteURL, genOptions, applicationName, environmentName, imageName, "", r.AppFS, gitOpsBranch, gitOpsContext, true, componentGeneratedResources)
 		if err != nil {
 			log.Error(err, fmt.Sprintf("unable to get generate gitops resources for %s %v", componentName, req.NamespacedName))
-			_ = r.AppFS.RemoveAll(tempDir) // not worried with an err, its a best case attempt to delete the temp clone dir
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) // not worried with an err, its a best case attempt to delete the temp clone dir
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
@@ -360,6 +369,7 @@
 		if commitID, err = r.Generator.GetCommitIDFromRepo(r.AppFS, repoPath); err != nil {
 			//gitops generator errors are sanitized
 			log.Error(err, "")
+			ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir)
 			r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err)
 			return ctrl.Result{}, err
 		}
@@ -444,7 +454,7 @@
 					return true
 				},
 				DeleteFunc: func(e event.DeleteEvent) bool {
 					log := log.WithValues("namespace", e.Object.GetNamespace())
 					logutil.LogAPIResourceChangeEvent(log, e.Object.GetName(), "Environment", logutil.ResourceDelete, nil)
 					return false
 				},

controllers/component_controller_finalizer.go

@@ -86,6 +86,7 @@
 	//Gitops functions return sanitized error messages
 	err = r.Generator.GitRemoveComponent(tempDir, gitOpsURL, component.Name, gitOpsBranch, gitOpsContext)
 	if err != nil {
+		ioutils.RemoveFolderAndLogError(r.Log, r.AppFS, tempDir)
 		return err
 	}
 

pkg/util/ioutils/ioutils.go

@@ -61,8 +61,10 @@ func CreateTempPath(prefix string, appFs afero.Afero) (string, error) {
 // RemoveFolderAndLogError removes the specified folder. If the delete fails, no error is returned, but an error is logged
 // Used in cases where we're cleaning up after encountering an error, but want to return the original error instead.
 func RemoveFolderAndLogError(log logr.Logger, fs afero.Fs, path string) {
-	err := fs.RemoveAll(path)
-	if err != nil {
-		log.Error(err, fmt.Sprintf("Unable to delete folder %s", path))
+	if path != "" {
+		err := fs.RemoveAll(path)
+		if err != nil {
+			log.Error(err, fmt.Sprintf("Unable to delete folder %s", path))
+		}
 	}
 }

pkg/util/ioutils/ioutils_test.go

@@ -164,6 +164,7 @@ func TestRemoveFolderAndLogError(t *testing.T) {
 	tests := []struct {
 		name string
 		fs   afero.Afero
+		path string
 	}{
 		{
 			name: "inmemory fs",
@@ -172,21 +173,29 @@ func TestRemoveFolderAndLogError(t *testing.T) {
 		{
 			name: "read only fs",
 			fs:   readOnlyFs,
+			path: "/somepath",
 		},
 		{
 			name: "local fs",
 			fs:   fs,
 		},
+		{
+			name: "empty path",
+			fs:   fs,
+			path: "",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			path, _ := CreateTempPath("TestCreateTempPath", tt.fs)
+			if tt.path == "" && tt.name != "empty path" {
+				tt.path, _ = CreateTempPath("TestCreateTempPath", tt.fs)
+			}
 			log := zap.New(zap.UseFlagOptions(&zap.Options{
 				Development: true,
 				TimeEncoder: zapcore.ISO8601TimeEncoder,
 			}))
-			RemoveFolderAndLogError(log, tt.fs, path)
+			RemoveFolderAndLogError(log, tt.fs, tt.path)
 		})
 	}
 }