Improve error messaging for invalid arguments (flyteorg#5658)

* Improve error messaging for invalid arguments Signed-off-by: Kevin Su <[email protected]> * update tests Signed-off-by: Kevin Su <[email protected]> * fix tests Signed-off-by: Kevin Su <[email protected]> * Define a separate identifier for the launchplan test Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Kevin Su <[email protected]> Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
robert-ulbrich-mercedes-benz · Aug 14, 2024 · bed761c · bed761c
1 parent 3d38729
commit bed761c
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 13 deletions.
diff --git a/flyteadmin/pkg/errors/errors.go b/flyteadmin/pkg/errors/errors.go
@@ -112,31 +112,30 @@ func NewIncompatibleClusterError(ctx context.Context, errorMsg, curCluster strin
 }
 
 func compareJsons(jsonArray1 jsondiff.Patch, jsonArray2 jsondiff.Patch) []string {
-	results := []string{}
+	var results []string
 	map1 := make(map[string]jsondiff.Operation)
 	for _, obj := range jsonArray1 {
 		map1[obj.Path] = obj
 	}
 
 	for _, obj := range jsonArray2 {
 		if val, ok := map1[obj.Path]; ok {
-			result := fmt.Sprintf("\t\t- %v: %v -> %v", obj.Path, obj.Value, val.Value)
+			result := fmt.Sprintf("%v:\n\t- %v \n\t+ %v", obj.Path, obj.Value, val.Value)
 			results = append(results, result)
 		}
 	}
 	return results
 }
 
 func NewTaskExistsDifferentStructureError(ctx context.Context, request *admin.TaskCreateRequest, oldSpec *core.CompiledTask, newSpec *core.CompiledTask) FlyteAdminError {
-	errorMsg := fmt.Sprintf("%v task with different structure already exists:\n", request.Id.Name)
+	errorMsg := fmt.Sprintf("%v task with different structure already exists. (Please register a new version of the task):\n", request.Id.Name)
 	diff, _ := jsondiff.Compare(oldSpec, newSpec)
 	rdiff, _ := jsondiff.Compare(newSpec, oldSpec)
 	rs := compareJsons(diff, rdiff)
 
 	errorMsg += strings.Join(rs, "\n")
 
 	return NewFlyteAdminErrorf(codes.InvalidArgument, errorMsg)
-
 }
 
 func NewTaskExistsIdenticalStructureError(ctx context.Context, request *admin.TaskCreateRequest) FlyteAdminError {
@@ -145,7 +144,7 @@ func NewTaskExistsIdenticalStructureError(ctx context.Context, request *admin.Ta
 }
 
 func NewWorkflowExistsDifferentStructureError(ctx context.Context, request *admin.WorkflowCreateRequest, oldSpec *core.CompiledWorkflowClosure, newSpec *core.CompiledWorkflowClosure) FlyteAdminError {
-	errorMsg := fmt.Sprintf("%v workflow with different structure already exists:\n", request.Id.Name)
+	errorMsg := fmt.Sprintf("%v workflow with different structure already exists. (Please register a new version of the workflow):\n", request.Id.Name)
 	diff, _ := jsondiff.Compare(oldSpec, newSpec)
 	rdiff, _ := jsondiff.Compare(newSpec, oldSpec)
 	rs := compareJsons(diff, rdiff)
@@ -183,7 +182,7 @@ func NewWorkflowExistsIdenticalStructureError(ctx context.Context, request *admi
 }
 
 func NewLaunchPlanExistsDifferentStructureError(ctx context.Context, request *admin.LaunchPlanCreateRequest, oldSpec *admin.LaunchPlanSpec, newSpec *admin.LaunchPlanSpec) FlyteAdminError {
-	errorMsg := "launch plan with different structure already exists:\n"
+	errorMsg := fmt.Sprintf("%v launch plan with different structure already exists. (Please register a new version of the launch plan):\n", request.Id.Name)
 	diff, _ := jsondiff.Compare(oldSpec, newSpec)
 	rdiff, _ := jsondiff.Compare(newSpec, oldSpec)
 	rs := compareJsons(diff, rdiff)

diff --git a/flyteadmin/pkg/errors/errors_test.go b/flyteadmin/pkg/errors/errors_test.go
@@ -74,7 +74,7 @@ func TestJsonDifferHasDiffError(t *testing.T) {
 	diff, _ := jsondiff.Compare(oldSpec, newSpec)
 	rdiff, _ := jsondiff.Compare(newSpec, oldSpec)
 	rs := compareJsons(diff, rdiff)
-	assert.Equal(t, "\t\t- /four: 4 -> 0", strings.Join(rs, "\n"))
+	assert.Equal(t, "/four:\n\t- 4 \n\t+ 0", strings.Join(rs, "\n"))
 }
 
 func TestJsonDifferNoDiffError(t *testing.T) {
@@ -144,7 +144,7 @@ func TestNewTaskExistsDifferentStructureError(t *testing.T) {
 	s, ok := status.FromError(statusErr)
 	assert.True(t, ok)
 	assert.Equal(t, codes.InvalidArgument, s.Code())
-	assert.Equal(t, "t1 task with different structure already exists:\n\t\t- /template/Target/Container/resources/requests/0/value: 150m -> 250m", s.Message())
+	assert.Equal(t, "t1 task with different structure already exists. (Please register a new version of the task):\n/template/Target/Container/resources/requests/0/value:\n\t- 150m \n\t+ 250m", s.Message())
 }
 
 func TestNewTaskExistsIdenticalStructureError(t *testing.T) {
@@ -160,7 +160,7 @@ func TestNewTaskExistsIdenticalStructureError(t *testing.T) {
 }
 
 func TestNewWorkflowExistsDifferentStructureError(t *testing.T) {
-	identifier = core.Identifier{
+	identifier := core.Identifier{
 		ResourceType: core.ResourceType_WORKFLOW,
 		Project:      "testProj",
 		Domain:       "domain",
@@ -225,7 +225,7 @@ func TestNewWorkflowExistsDifferentStructureError(t *testing.T) {
 	s, ok := status.FromError(statusErr)
 	assert.True(t, ok)
 	assert.Equal(t, codes.InvalidArgument, s.Code())
-	assert.Equal(t, "hello workflow with different structure already exists:\n\t\t- /primary/connections/upstream/bar: <nil> -> map[ids:[start-node]]\n\t\t- /primary/connections/upstream/end-node/ids/0: foo -> bar\n\t\t- /primary/connections/upstream/foo: map[ids:[start-node]] -> <nil>\n\t\t- /primary/template/nodes/0/id: foo -> bar", s.Message())
+	assert.Equal(t, "hello workflow with different structure already exists. (Please register a new version of the workflow):\n/primary/connections/upstream/bar:\n\t- <nil> \n\t+ map[ids:[start-node]]\n/primary/connections/upstream/end-node/ids/0:\n\t- foo \n\t+ bar\n/primary/connections/upstream/foo:\n\t- map[ids:[start-node]] \n\t+ <nil>\n/primary/template/nodes/0/id:\n\t- foo \n\t+ bar", s.Message())
 
 	details, ok := s.Details()[0].(*admin.CreateWorkflowFailureReason)
 	assert.True(t, ok)
@@ -251,6 +251,11 @@ func TestNewWorkflowExistsIdenticalStructureError(t *testing.T) {
 }
 
 func TestNewLaunchPlanExistsDifferentStructureError(t *testing.T) {
+	identifier := core.Identifier{
+		ResourceType: core.ResourceType_LAUNCH_PLAN,
+		Name:         "lp_name",
+	}
+
 	req := &admin.LaunchPlanCreateRequest{
 		Id: &identifier,
 	}
@@ -284,7 +289,7 @@ func TestNewLaunchPlanExistsDifferentStructureError(t *testing.T) {
 	s, ok := status.FromError(statusErr)
 	assert.True(t, ok)
 	assert.Equal(t, codes.InvalidArgument, s.Code())
-	assert.Equal(t, "launch plan with different structure already exists:\n\t\t- /workflow_id/version: ver1 -> ver2", s.Message())
+	assert.Equal(t, "lp_name launch plan with different structure already exists. (Please register a new version of the launch plan):\n/workflow_id/version:\n\t- ver1 \n\t+ ver2", s.Message())
 }
 
 func TestNewLaunchPlanExistsIdenticalStructureError(t *testing.T) {

diff --git a/flyteadmin/pkg/manager/impl/workflow_manager_test.go b/flyteadmin/pkg/manager/impl/workflow_manager_test.go
@@ -187,7 +187,7 @@ func TestCreateWorkflow_ExistingWorkflow(t *testing.T) {
 		getMockWorkflowConfigProvider(), getMockWorkflowCompiler(), mockStorageClient, storagePrefix, mockScope.NewTestScope())
 	request := testutils.GetWorkflowRequest()
 	response, err := workflowManager.CreateWorkflow(context.Background(), request)
-	assert.EqualError(t, err, "name workflow with different structure already exists:\n\t\t- /primary/template/id: <nil> -> map[domain:domain name:name project:project resource_type:2 version:version]\n\t\t- /primary/template/interface/inputs: <nil> -> map[variables:map[foo:map[type:map[Type:map[Simple:3]]]]]\n\t\t- /primary/template/interface/outputs: <nil> -> map[variables:map[bar:map[type:map[Type:map[Simple:3]]]]]\n\t\t- /primary/template/nodes: <nil> -> [map[Target:<nil> id:node 1] map[Target:<nil> id:node 2]]")
+	assert.EqualError(t, err, "name workflow with different structure already exists. (Please register a new version of the workflow):\n/primary/template/id:\n\t- <nil> \n\t+ map[domain:domain name:name project:project resource_type:2 version:version]\n/primary/template/interface/inputs:\n\t- <nil> \n\t+ map[variables:map[foo:map[type:map[Type:map[Simple:3]]]]]\n/primary/template/interface/outputs:\n\t- <nil> \n\t+ map[variables:map[bar:map[type:map[Type:map[Simple:3]]]]]\n/primary/template/nodes:\n\t- <nil> \n\t+ [map[Target:<nil> id:node 1] map[Target:<nil> id:node 2]]")
 	assert.Nil(t, response)
 }
 
@@ -205,7 +205,7 @@ func TestCreateWorkflow_ExistingWorkflow_Different(t *testing.T) {
 
 	request := testutils.GetWorkflowRequest()
 	response, err := workflowManager.CreateWorkflow(context.Background(), request)
-	assert.EqualError(t, err, "name workflow with different structure already exists:\n\t\t- /primary/template/id: <nil> -> map[domain:domain name:name project:project resource_type:2 version:version]\n\t\t- /primary/template/interface/inputs: <nil> -> map[variables:map[foo:map[type:map[Type:map[Simple:3]]]]]\n\t\t- /primary/template/interface/outputs: <nil> -> map[variables:map[bar:map[type:map[Type:map[Simple:3]]]]]\n\t\t- /primary/template/nodes: <nil> -> [map[Target:<nil> id:node 1] map[Target:<nil> id:node 2]]")
+	assert.EqualError(t, err, "name workflow with different structure already exists. (Please register a new version of the workflow):\n/primary/template/id:\n\t- <nil> \n\t+ map[domain:domain name:name project:project resource_type:2 version:version]\n/primary/template/interface/inputs:\n\t- <nil> \n\t+ map[variables:map[foo:map[type:map[Type:map[Simple:3]]]]]\n/primary/template/interface/outputs:\n\t- <nil> \n\t+ map[variables:map[bar:map[type:map[Type:map[Simple:3]]]]]\n/primary/template/nodes:\n\t- <nil> \n\t+ [map[Target:<nil> id:node 1] map[Target:<nil> id:node 2]]")
 	flyteErr := err.(flyteErrors.FlyteAdminError)
 	assert.Equal(t, codes.InvalidArgument, flyteErr.Code())
 	assert.Nil(t, response)