refactor: cleanup minor part 3 (#555)

* refactor: remove panic when plugin conn fail * refactor: append err messages * refactor: safe cast on survey validator * refactor: add job names * refactor: return nil * refactor: return nil
raystack · Sep 15, 2022 · 03952e9 · 03952e9
1 parent 0595fdb
commit 03952e9
Show file tree

Hide file tree

Showing 10 changed files with 47 additions and 47 deletions.
diff --git a/api/handler/v1beta1/namespace_test.go b/api/handler/v1beta1/namespace_test.go
@@ -85,7 +85,7 @@ func TestNamespaceOnServer(t *testing.T) {
 			}
 			_, err := namespaceServiceServer.RegisterProjectNamespace(ctx, &namespaceRequest)
 			assert.NotNil(t, err)
-			assert.Equal(t, "rpc error: code = NotFound desc = project does not exist: not found for entity namespace: unable to store namespace", err.Error())
+			assert.Equal(t, "rpc error: code = NotFound desc = not found for entity namespace: project does not exist: unable to store namespace", err.Error())
 		})
 	})
 

diff --git a/job/service.go b/job/service.go
@@ -334,14 +334,13 @@ func (srv *Service) GetTaskDependencies(ctx context.Context, namespace models.Na
 
 // Delete deletes a job spec from all spec repos
 func (srv *Service) Delete(ctx context.Context, namespace models.NamespaceSpec, jobSpec models.JobSpec) error {
-	isDependency, err := srv.isDependency(ctx, jobSpec)
+	dependentJobNames, err := srv.getDependentJobNames(ctx, jobSpec)
 	if err != nil {
 		return err
 	}
 
-	if isDependency {
-		// TODO: Ideally should include list of jobs that are using the requested job in the error message
-		return fmt.Errorf("cannot delete job %s since it's dependency of other job", jobSpec.Name)
+	if len(dependentJobNames) > 0 {
+		return fmt.Errorf("cannot delete job %s since it's dependency of other jobs: %s", jobSpec.Name, strings.Join(dependentJobNames, ","))
 	}
 
 	// delete jobs from internal store
@@ -359,17 +358,16 @@ func (srv *Service) bulkDelete(ctx context.Context, namespace models.NamespaceSp
 	namespaceJobSpecRepo := srv.namespaceJobSpecRepoFactory.New(namespace)
 	success, failure := 0, 0
 	for _, jobSpec := range jobSpecsToDelete {
-		isDependency, err := srv.isDependency(ctx, jobSpec)
+		dependentJobNames, err := srv.getDependentJobNames(ctx, jobSpec)
 		if err != nil {
 			failure++
 			warnMsg := fmt.Sprintf("[%s] error '%s': failed to delete job, %s", namespace.Name, jobSpec.Name, err.Error())
 			logWriter.Write(writer.LogLevelWarning, warnMsg)
 			continue
 		}
-		if isDependency {
-			// TODO: Ideally should include list of jobs that are using the requested job in the error message
+		if len(dependentJobNames) > 0 {
 			failure++
-			err = fmt.Errorf("cannot delete job %s since it's dependency of other job", jobSpec.Name)
+			err = fmt.Errorf("cannot delete job %s since it's dependency of other jobs: %s", jobSpec.Name, strings.Join(dependentJobNames, ","))
 			warnMsg := fmt.Sprintf("[%s] error '%s': failed to delete job, %s", namespace.Name, jobSpec.Name, err.Error())
 			logWriter.Write(writer.LogLevelWarning, warnMsg)
 			continue
@@ -441,13 +439,17 @@ func (srv *Service) GetDependencyResolvedSpecs(ctx context.Context, proj models.
 }
 
 // do other jobs depend on this jobSpec
-func (srv *Service) isDependency(ctx context.Context, jobSpec models.JobSpec) (bool, error) {
+func (srv *Service) getDependentJobNames(ctx context.Context, jobSpec models.JobSpec) ([]string, error) {
 	// inferred and static dependents
 	dependentJobs, err := srv.jobSpecRepository.GetDependentJobs(ctx, &jobSpec)
 	if err != nil {
-		return false, fmt.Errorf("unable to check dependents of job %s", jobSpec.Name)
+		return nil, fmt.Errorf("unable to check dependents of job %s", jobSpec.Name)
 	}
-	return len(dependentJobs) > 0, nil
+	jobNames := make([]string, len(dependentJobs))
+	for i, job := range dependentJobs {
+		jobNames[i] = job.Name
+	}
+	return jobNames, nil
 }
 
 func (srv *Service) GetByDestination(ctx context.Context, projectSpec models.ProjectSpec, destination string) (models.JobSpec, error) {

diff --git a/job/service_test.go b/job/service_test.go
@@ -503,7 +503,7 @@ func TestService(t *testing.T) {
 				nil, nil, nil, nil, nil, nil, jobSpecRepo, nil)
 			err := svc.Delete(ctx, namespaceSpec, jobSpecsBase[0])
 			assert.NotNil(t, err)
-			assert.Equal(t, "cannot delete job test since it's dependency of other job", err.Error())
+			assert.Equal(t, "cannot delete job test since it's dependency of other jobs: downstream-test", err.Error())
 		})
 
 		t.Run("should return error if unable to delete job spec", func(t *testing.T) {

diff --git a/plugin/v1beta1/base/client.go b/plugin/v1beta1/base/client.go
@@ -94,8 +94,4 @@ func (m *GRPCClient) MakeFatalOnConnErr(err error) {
 	}
 	m.Logger.Error(fmt.Sprintf("Core communication failed with plugin: \n%+v", err))
 	m.Logger.Error(fmt.Sprintf("Exiting application, plugin crashed %s", m.Name))
-
-	// TODO(kush.sharma): once plugins are more stable and we have strict health
-	// checks we can remove this fail
-	panic(err)
 }
diff --git a/service/errors.go b/service/errors.go
@@ -53,22 +53,21 @@ func FromError(err error, entity, msg string) *DomainError {
 		msgStr = err.Error()
 	}
 
-	// TODO: Improve the msg, append to existing msg
-	if msg == "" {
-		msg = msgStr
+	if msg != "" {
+		msgStr = fmt.Sprintf("%s, %s", msg, msgStr)
 	}
 
 	return &DomainError{
 		Err:       err,
-		Message:   msg,
+		Message:   msgStr,
 		Entity:    entity,
 		ErrorType: errType,
 	}
 }
 
 func (e *DomainError) Error() string {
-	return fmt.Sprintf("%v: %v for entity %v",
-		e.Message, e.ErrorType.String(), e.Entity)
+	return fmt.Sprintf("%v for entity %v: %v",
+		e.ErrorType.String(), e.Entity, e.Message)
 }
 
 func (e *DomainError) Unwrap() error {
@@ -84,6 +83,6 @@ func (e *DomainError) DebugString() string {
 		wrappedError = e.Err.Error()
 	}
 
-	return fmt.Sprintf("%v: %v for %v: %s",
-		e.Message, e.ErrorType.String(), e.Entity, wrappedError)
+	return fmt.Sprintf("%v for %v: %v (%s)",
+		e.ErrorType.String(), e.Entity, e.Message, wrappedError)
 }
diff --git a/service/errors_test.go b/service/errors_test.go
@@ -15,29 +15,29 @@ func TestErrors(t *testing.T) {
 		de := service.NewError("project", service.ErrNotFound, "project t-optimus not found")
 
 		assert.Error(t, de)
-		assert.Equal(t, "project t-optimus not found: not found for entity project", de.Error())
+		assert.Equal(t, "not found for entity project: project t-optimus not found", de.Error())
 	})
 	t.Run("creates domain error from store error", func(t *testing.T) {
 		de := service.FromError(store.ErrResourceNotFound, "namespace", "invalid name")
 
 		assert.Error(t, de)
-		assert.Equal(t, "invalid name: not found for entity namespace", de.Error())
+		assert.Equal(t, "not found for entity namespace: invalid name, resource not found", de.Error())
 	})
 	t.Run("creates domain error and returns error message", func(t *testing.T) {
 		err1 := errors.New("random database error")
 		de1 := service.FromError(err1, "project", "not able to get project")
 		de2 := service.FromError(de1, "namespace", "error while fetching namespace")
 
 		assert.Error(t, de2)
-		expectedErrorString := "error while fetching namespace: internal error for namespace: not able to get project: internal error for project: random database error"
+		expectedErrorString := "internal error for namespace: error while fetching namespace, internal error (internal error for project: not able to get project, internal error (random database error))"
 		assert.Equal(t, expectedErrorString, de2.DebugString())
 	})
 	t.Run("creates debug string when err inside is nil", func(t *testing.T) {
 		de1 := service.NewError("project", service.ErrInternalError, "some error with project")
 		de2 := service.FromError(de1, "namespace", "error while fetching namespace")
 
 		assert.Error(t, de2)
-		expectedErrorString := "error while fetching namespace: internal error for namespace: some error with project: internal error for project: "
+		expectedErrorString := "internal error for namespace: error while fetching namespace, internal error (internal error for project: some error with project ())"
 		assert.Equal(t, expectedErrorString, de2.DebugString())
 	})
 	t.Run("allows unwrapping of error in error chain", func(t *testing.T) {
@@ -47,7 +47,7 @@ func TestErrors(t *testing.T) {
 		assert.Error(t, de2)
 		assert.ErrorIs(t, de2, store.ErrResourceNotFound)
 
-		expectedErrorString := "error while fetching namespace: not found for entity namespace"
+		expectedErrorString := "not found for entity namespace: error while fetching namespace, resource not found"
 		assert.Equal(t, expectedErrorString, de2.Error())
 	})
 }
diff --git a/service/namespace_service_test.go b/service/namespace_service_test.go
@@ -37,14 +37,14 @@ func TestNamespaceService(t *testing.T) {
 
 			_, err := svc.Get(ctx, "", "namespace")
 			assert.NotNil(t, err)
-			assert.Equal(t, "project name cannot be empty: invalid argument for entity project", err.Error())
+			assert.Equal(t, "invalid argument for entity project: project name cannot be empty", err.Error())
 		})
 		t.Run("return error when namespace name is empty", func(t *testing.T) {
 			svc := service.NewNamespaceService(nil, nil)
 
 			_, err := svc.Get(ctx, "project", "")
 			assert.NotNil(t, err)
-			assert.Equal(t, "namespace name cannot be empty: invalid argument for entity namespace", err.Error())
+			assert.Equal(t, "invalid argument for entity namespace: namespace name cannot be empty", err.Error())
 		})
 		t.Run("return error when repo returns error", func(t *testing.T) {
 			namespaceRepository := new(mock.NamespaceRepository)
@@ -56,7 +56,7 @@ func TestNamespaceService(t *testing.T) {
 
 			_, err := svc.Get(ctx, project.Name, "nonexistent")
 			assert.NotNil(t, err)
-			assert.Equal(t, "resource not found: not found for entity namespace", err.Error())
+			assert.Equal(t, "not found for entity namespace: resource not found", err.Error())
 		})
 		t.Run("return project and namespace successfully", func(t *testing.T) {
 			namespaceRepository := new(mock.NamespaceRepository)
@@ -80,7 +80,7 @@ func TestNamespaceService(t *testing.T) {
 
 			_, err := svc.GetByName(ctx, project, "")
 			assert.NotNil(t, err)
-			assert.Equal(t, "namespace name cannot be empty: invalid argument for entity namespace", err.Error())
+			assert.Equal(t, "invalid argument for entity namespace: namespace name cannot be empty", err.Error())
 		})
 		t.Run("returns error when repo returns error", func(t *testing.T) {
 			namespaceRepository := new(mock.NamespaceRepository)
@@ -92,7 +92,7 @@ func TestNamespaceService(t *testing.T) {
 
 			_, err := svc.GetByName(ctx, project, "nonexistent")
 			assert.NotNil(t, err)
-			assert.Equal(t, "resource not found: not found for entity namespace", err.Error())
+			assert.Equal(t, "not found for entity namespace: resource not found", err.Error())
 		})
 		t.Run("returns namespace successfully", func(t *testing.T) {
 			namespaceRepository := new(mock.NamespaceRepository)
@@ -135,7 +135,7 @@ func TestNamespaceService(t *testing.T) {
 
 			_, _, err := svc.GetNamespaceOptionally(ctx, project.Name, "nonexistent")
 			assert.NotNil(t, err)
-			assert.Equal(t, "resource not found: not found for entity namespace", err.Error())
+			assert.Equal(t, "not found for entity namespace: resource not found", err.Error())
 		})
 		t.Run("return project when namespace name is empty", func(t *testing.T) {
 			defer projService.AssertExpectations(t)
@@ -177,7 +177,7 @@ func TestNamespaceService(t *testing.T) {
 
 			err := svc.Save(ctx, project.Name, ns)
 			assert.NotNil(t, err)
-			assert.Equal(t, "namespace name cannot be empty: invalid argument for entity namespace", err.Error())
+			assert.Equal(t, "invalid argument for entity namespace: namespace name cannot be empty", err.Error())
 		})
 		t.Run("returns error when fetching project fails", func(t *testing.T) {
 			defer projService.AssertExpectations(t)
@@ -191,7 +191,7 @@ func TestNamespaceService(t *testing.T) {
 
 			err := svc.Save(ctx, project.Name, namespace)
 			assert.NotNil(t, err)
-			assert.Equal(t, "resource not found: not found for entity namespace", err.Error())
+			assert.Equal(t, "not found for entity namespace: resource not found", err.Error())
 		})
 		t.Run("calls repo to store project successfully", func(t *testing.T) {
 			defer projService.AssertExpectations(t)
@@ -231,7 +231,7 @@ func TestNamespaceService(t *testing.T) {
 
 			_, err := svc.GetAll(ctx, project.Name)
 			assert.NotNil(t, err)
-			assert.Equal(t, "internal error: internal error for entity namespace", err.Error())
+			assert.Equal(t, "internal error for entity namespace: internal error", err.Error())
 		})
 		t.Run("return namespaces successfully", func(t *testing.T) {
 			defer projService.AssertExpectations(t)

diff --git a/service/project_service_test.go b/service/project_service_test.go
@@ -27,7 +27,7 @@ func TestProjectService(t *testing.T) {
 
 			_, err := svc.Get(ctx, "")
 			assert.NotNil(t, err)
-			assert.Equal(t, "project name cannot be empty: invalid argument for entity project", err.Error())
+			assert.Equal(t, "invalid argument for entity project: project name cannot be empty", err.Error())
 		})
 		t.Run("return error when project name is invalid", func(t *testing.T) {
 			projectRepository := new(mock.ProjectRepository)
@@ -39,7 +39,7 @@ func TestProjectService(t *testing.T) {
 
 			_, err := svc.Get(ctx, "nonexistent")
 			assert.NotNil(t, err)
-			assert.Equal(t, "resource not found: not found for entity project", err.Error())
+			assert.Equal(t, "not found for entity project: resource not found", err.Error())
 		})
 		t.Run("return project successfully", func(t *testing.T) {
 			projectRepository := new(mock.ProjectRepository)
@@ -64,7 +64,7 @@ func TestProjectService(t *testing.T) {
 
 			err := svc.Save(ctx, proj)
 			assert.NotNil(t, err)
-			assert.Equal(t, "project name cannot be empty: invalid argument for entity project", err.Error())
+			assert.Equal(t, "invalid argument for entity project: project name cannot be empty", err.Error())
 		})
 		t.Run("calls repo to store project successfully", func(t *testing.T) {
 			project2 := models.ProjectSpec{
@@ -96,7 +96,7 @@ func TestProjectService(t *testing.T) {
 
 			_, err := svc.GetAll(ctx)
 			assert.NotNil(t, err)
-			assert.Equal(t, "internal error: internal error for entity project", err.Error())
+			assert.Equal(t, "internal error for entity project: internal error", err.Error())
 		})
 		t.Run("return projects successfully", func(t *testing.T) {
 			projectRepository := new(mock.ProjectRepository)

diff --git a/service/secret_service_test.go b/service/secret_service_test.go
@@ -46,7 +46,7 @@ func TestSecretService(t *testing.T) {
 
 			err := svc.Save(ctx, "local", "first", emptySecret)
 			assert.NotNil(t, err)
-			assert.Equal(t, "secret name cannot be empty: invalid argument for entity secret", err.Error())
+			assert.Equal(t, "invalid argument for entity secret: secret name cannot be empty", err.Error())
 		})
 		t.Run("returns error when namespace service has error", func(t *testing.T) {
 			nsService := new(mock.NamespaceService)
@@ -97,7 +97,7 @@ func TestSecretService(t *testing.T) {
 
 			err := svc.Update(ctx, "local", "first", emptySecret)
 			assert.NotNil(t, err)
-			assert.Equal(t, "secret name cannot be empty: invalid argument for entity secret", err.Error())
+			assert.Equal(t, "invalid argument for entity secret: secret name cannot be empty", err.Error())
 		})
 		t.Run("returns error when namespace service has error", func(t *testing.T) {
 			nsService := new(mock.NamespaceService)
@@ -207,7 +207,7 @@ func TestSecretService(t *testing.T) {
 			assert.Len(t, list, 0)
 
 			assert.NotNil(t, err)
-			assert.Equal(t, "error while getting secrets: internal error for entity secret", err.Error())
+			assert.Equal(t, "internal error for entity secret: error while getting secrets, internal error", err.Error())
 		})
 	})
 	t.Run("Delete", func(t *testing.T) {

diff --git a/utils/validator.go b/utils/validator.go
@@ -38,7 +38,10 @@ func (*VFactory) NewFromRegex(re, message string) survey.Validator {
 		if k != reflect.String {
 			return fmt.Errorf("was expecting a string, got %s", k.String())
 		}
-		val := v.(string)
+		val, ok := v.(string)
+		if !ok {
+			return fmt.Errorf("error to cast [%+v] to string type", v)
+		}
 		matched := regex.MatchString(val)
 		if matched == false {
 			return errors.New(message)