You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/10/12 14:45:27 UTC

[GitHub] [incubator-devlake] warren830 opened a new pull request, #3403: Fix makepipelineplan

warren830 opened a new pull request, #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403

   # Summary
   
   modify github blueprint.go and blueprint_test.go to be more elegant
   
   ### Does this close any open issues?
   relate to #3127 
   
   ### Screenshots
   <img width="676" alt="image" src="https://user-images.githubusercontent.com/39366025/195374456-f45ff310-f5e6-4298-aa1a-099116c61b86.png">
   
   
   ### Other Information
   Any other information that is important to this PR.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-devlake] warren830 commented on a diff in pull request #3403: Fix makepipelineplan

Posted by GitBox <gi...@apache.org>.
warren830 commented on code in PR #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403#discussion_r994095371


##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,178 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	getApiRepoIfNil := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {
+		if repo == nil {
+			repo, err = getApiRepo(op, apiClient)
+		}
+		return repo, err
 	}
-	stage = append(stage, &core.PipelineTask{
-		Plugin:   "github",
-		Subtasks: subtasks,
-		Options:  options,
-	})
-	// collect git data by gitextractor if CODE was requested
-	if utils.StringsContains(scopeElem.Entities, core.DOMAIN_TYPE_CODE) {
-		// here is the tricky part, we have to obtain the repo id beforehand
-		if connection == nil {
-			connection = new(models.GithubConnection)
-			err = connectionHelper.FirstById(connection, connectionId)
+	plan := make(core.PipelinePlan, len(scope))
+	for i, scopeElem := range scope {
+		// handle taskOptions and transformationRules, by dumping them to taskOptions
+		transformationRules := make(map[string]interface{})
+		if len(scopeElem.Transformation) > 0 {
+			err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
 			if err != nil {
 				return nil, err
 			}
 		}
-		token := strings.Split(connection.Token, ",")[0]
-		if apiRepo == nil {
-			apiRepo = new(tasks.GithubApiRepo)
-			err = getApiRepo(connection, token, op, apiRepo)
-			if err != nil {
-				return nil, err
+		// refdiff
+		if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
+			// add a new task to next stage
+			j := i + 1
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			plan[j] = core.PipelineStage{
+				{
+					Plugin:  "refdiff",
+					Options: refdiffRules.(map[string]interface{}),
+				},
 			}
+			// remove it from github transformationRules
+			delete(transformationRules, "refdiff")
 		}
-		cloneUrl, err := errors.Convert01(url.Parse(apiRepo.CloneUrl))
+		// construct task options for github
+		options := make(map[string]interface{})
+		err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
 		if err != nil {
 			return nil, err
 		}
-		cloneUrl.User = url.UserPassword("git", token)
-		stage = append(stage, &core.PipelineTask{
-			Plugin: "gitextractor",
-			Options: map[string]interface{}{
-				"url":    cloneUrl.String(),
-				"repoId": didgen.NewDomainIdGenerator(&models.GithubRepo{}).Generate(connectionId, apiRepo.GithubId),
-				"proxy":  connection.Proxy,
-			},
-		})
-	}
-	// dora
-	if productionPattern, ok := transformationRules["productionPattern"]; ok && productionPattern != nil {
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		// add a new task to next stage
-		if plan[j] != nil {
-			j++
-		}
-		if j == len(plan) {
-			plan = append(plan, nil)
+		options["connectionId"] = connection.ID
+		options["transformationRules"] = transformationRules
+		// make sure task options is valid
+		op, err := tasks.DecodeAndValidateTaskOptions(options)
+		if err != nil {
+			return nil, err
 		}
+		// construct subtasks
+		subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
 		if err != nil {
 			return nil, err
 		}
-		if apiRepo == nil {
-			if connection == nil {
-				connection = new(models.GithubConnection)
-				err = connectionHelper.FirstById(connection, connectionId)
-				if err != nil {
-					return nil, err
-				}
-			}
+		stage := plan[i]
+		if stage == nil {
+			stage = core.PipelineStage{}
+		}
+		stage = append(stage, &core.PipelineTask{
+			Plugin:   "github",
+			Subtasks: subtasks,
+			Options:  options,
+		})
+		// collect git data by gitextractor if CODE was requested
+		if utils.StringsContains(scopeElem.Entities, core.DOMAIN_TYPE_CODE) {
+			// here is the tricky part, we have to obtain the repo id beforehand
 			token := strings.Split(connection.Token, ",")[0]
-			apiRepo = new(tasks.GithubApiRepo)
-			err = getApiRepo(connection, token, op, apiRepo)
+			repo, err = getApiRepoIfNil(op)
+			if err != nil {
+				return nil, err
+			}
+			cloneUrl, err := errors.Convert01(url.Parse(repo.CloneUrl))
+			if err != nil {
+				return nil, err
+			}
+			cloneUrl.User = url.UserPassword("git", token)
+			stage = append(stage, &core.PipelineTask{
+				Plugin: "gitextractor",
+				Options: map[string]interface{}{
+					"url":    cloneUrl.String(),
+					"repoId": didgen.NewDomainIdGenerator(&models.GithubRepo{}).Generate(connection.ID, repo.GithubId),
+					"proxy":  connection.Proxy,
+				},
+			})
+		}
+		// dora
+		if productionPattern, ok := transformationRules["productionPattern"]; ok && productionPattern != nil {
+			j := i + 1
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			// add a new task to next stage
+			if plan[j] != nil {
+				j++
+			}
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			if err != nil {
+				return nil, err
+			}
+			repo, err = getApiRepoIfNil(op)
 			if err != nil {
 				return nil, err
 			}
+
+			doraOption := make(map[string]interface{})

Review Comment:
   fixed



##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,178 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	getApiRepoIfNil := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {

Review Comment:
   fixed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3403: Fix makepipelineplan

Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403#discussion_r994211883


##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,180 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
-	}
-	stage = append(stage, &core.PipelineTask{
-		Plugin:   "github",
-		Subtasks: subtasks,
-		Options:  options,
-	})
-	// collect git data by gitextractor if CODE was requested
-	if utils.StringsContains(scopeElem.Entities, core.DOMAIN_TYPE_CODE) {
-		// here is the tricky part, we have to obtain the repo id beforehand
-		if connection == nil {
-			connection = new(models.GithubConnection)
-			err = connectionHelper.FirstById(connection, connectionId)
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	plan := make(core.PipelinePlan, len(scope))
+	for i, scopeElem := range scope {
+		// handle taskOptions and transformationRules, by dumping them to taskOptions
+		transformationRules := make(map[string]interface{})
+		if len(scopeElem.Transformation) > 0 {
+			err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
 			if err != nil {
 				return nil, err
 			}
 		}
-		token := strings.Split(connection.Token, ",")[0]
-		if apiRepo == nil {
-			apiRepo = new(tasks.GithubApiRepo)
-			err = getApiRepo(connection, token, op, apiRepo)
-			if err != nil {
-				return nil, err
+		// refdiff
+		if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
+			// add a new task to next stage
+			j := i + 1
+			if j == len(plan) {
+				plan = append(plan, nil)
 			}
+			plan[j] = core.PipelineStage{
+				{
+					Plugin:  "refdiff",
+					Options: refdiffRules.(map[string]interface{}),
+				},
+			}
+			// remove it from github transformationRules
+			delete(transformationRules, "refdiff")
 		}
-		cloneUrl, err := errors.Convert01(url.Parse(apiRepo.CloneUrl))
+		// construct task options for github
+		options := make(map[string]interface{})
+		err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
 		if err != nil {
 			return nil, err
 		}
-		cloneUrl.User = url.UserPassword("git", token)
-		stage = append(stage, &core.PipelineTask{
-			Plugin: "gitextractor",
-			Options: map[string]interface{}{
-				"url":    cloneUrl.String(),
-				"repoId": didgen.NewDomainIdGenerator(&models.GithubRepo{}).Generate(connectionId, apiRepo.GithubId),
-				"proxy":  connection.Proxy,
-			},
-		})
-	}
-	// dora
-	if productionPattern, ok := transformationRules["productionPattern"]; ok && productionPattern != nil {
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		// add a new task to next stage
-		if plan[j] != nil {
-			j++
+		options["connectionId"] = connection.ID
+		options["transformationRules"] = transformationRules
+		// make sure task options is valid
+		op, err := tasks.DecodeAndValidateTaskOptions(options)
+		if err != nil {
+			return nil, err
 		}
-		if j == len(plan) {
-			plan = append(plan, nil)
+		memorizedGetApiRepo := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {

Review Comment:
   Please remove `op *tasks.GithubOptions`, let function uses the `op` variable from 106 instead.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-devlake] abeizn merged pull request #3403: Fix makepipelineplan

Posted by GitBox <gi...@apache.org>.
abeizn merged PR #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3403: Fix makepipelineplan

Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403#discussion_r994084332


##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,178 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	getApiRepoIfNil := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {

Review Comment:
   How about `memorizedGetApiRepo`?



##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,178 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	getApiRepoIfNil := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {

Review Comment:
   This memorized function should be moved after line 112 and take no parameters.
   It makes no sense if the caller passed a different `op`



##########
plugins/github/api/blueprint.go:
##########
@@ -38,179 +38,178 @@ import (
 
 func MakePipelinePlan(subtaskMetas []core.SubTaskMeta, connectionId uint64, scope []*core.BlueprintScopeV100) (core.PipelinePlan, errors.Error) {
 	var err errors.Error
-	plan := make(core.PipelinePlan, len(scope))
-	for i, scopeElem := range scope {
-		plan, err = processScope(subtaskMetas, connectionId, scopeElem, i, plan, nil, nil)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return plan, nil
-}
-func processScope(subtaskMetas []core.SubTaskMeta, connectionId uint64, scopeElem *core.BlueprintScopeV100, i int, plan core.PipelinePlan, apiRepo *tasks.GithubApiRepo, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
-	var err errors.Error
-	// handle taskOptions and transformationRules, by dumping them to taskOptions
-	transformationRules := make(map[string]interface{})
-	if len(scopeElem.Transformation) > 0 {
-		err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
-		if err != nil {
-			return nil, err
-		}
-	}
-	// refdiff
-	if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
-		// add a new task to next stage
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		plan[j] = core.PipelineStage{
-			{
-				Plugin:  "refdiff",
-				Options: refdiffRules.(map[string]interface{}),
-			},
-		}
-		// remove it from github transformationRules
-		delete(transformationRules, "refdiff")
-	}
-	// construct task options for github
-	options := make(map[string]interface{})
-	err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
+	connection := new(models.GithubConnection)
+	err = connectionHelper.FirstById(connection, connectionId)
 	if err != nil {
 		return nil, err
 	}
-	options["connectionId"] = connectionId
-	options["transformationRules"] = transformationRules
-	// make sure task options is valid
-	op, err := tasks.DecodeAndValidateTaskOptions(options)
+	token := strings.Split(connection.Token, ",")[0]
+
+	apiClient, err := helper.NewApiClient(
+		context.TODO(),
+		connection.Endpoint,
+		map[string]string{
+			"Authorization": fmt.Sprintf("Bearer %s", token),
+		},
+		10*time.Second,
+		connection.Proxy,
+		basicRes,
+	)
 	if err != nil {
 		return nil, err
 	}
-	// construct subtasks
-	subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
+	plan, err := makePipelinePlan(subtaskMetas, scope, apiClient, connection)
 	if err != nil {
 		return nil, err
 	}
-	stage := plan[i]
-	if stage == nil {
-		stage = core.PipelineStage{}
+	return plan, nil
+}
+
+func makePipelinePlan(subtaskMetas []core.SubTaskMeta, scope []*core.BlueprintScopeV100, apiClient helper.ApiClientGetter, connection *models.GithubConnection) (core.PipelinePlan, errors.Error) {
+	var err errors.Error
+	var repo *tasks.GithubApiRepo
+	getApiRepoIfNil := func(op *tasks.GithubOptions) (*tasks.GithubApiRepo, errors.Error) {
+		if repo == nil {
+			repo, err = getApiRepo(op, apiClient)
+		}
+		return repo, err
 	}
-	stage = append(stage, &core.PipelineTask{
-		Plugin:   "github",
-		Subtasks: subtasks,
-		Options:  options,
-	})
-	// collect git data by gitextractor if CODE was requested
-	if utils.StringsContains(scopeElem.Entities, core.DOMAIN_TYPE_CODE) {
-		// here is the tricky part, we have to obtain the repo id beforehand
-		if connection == nil {
-			connection = new(models.GithubConnection)
-			err = connectionHelper.FirstById(connection, connectionId)
+	plan := make(core.PipelinePlan, len(scope))
+	for i, scopeElem := range scope {
+		// handle taskOptions and transformationRules, by dumping them to taskOptions
+		transformationRules := make(map[string]interface{})
+		if len(scopeElem.Transformation) > 0 {
+			err = errors.Convert(json.Unmarshal(scopeElem.Transformation, &transformationRules))
 			if err != nil {
 				return nil, err
 			}
 		}
-		token := strings.Split(connection.Token, ",")[0]
-		if apiRepo == nil {
-			apiRepo = new(tasks.GithubApiRepo)
-			err = getApiRepo(connection, token, op, apiRepo)
-			if err != nil {
-				return nil, err
+		// refdiff
+		if refdiffRules, ok := transformationRules["refdiff"]; ok && refdiffRules != nil {
+			// add a new task to next stage
+			j := i + 1
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			plan[j] = core.PipelineStage{
+				{
+					Plugin:  "refdiff",
+					Options: refdiffRules.(map[string]interface{}),
+				},
 			}
+			// remove it from github transformationRules
+			delete(transformationRules, "refdiff")
 		}
-		cloneUrl, err := errors.Convert01(url.Parse(apiRepo.CloneUrl))
+		// construct task options for github
+		options := make(map[string]interface{})
+		err = errors.Convert(json.Unmarshal(scopeElem.Options, &options))
 		if err != nil {
 			return nil, err
 		}
-		cloneUrl.User = url.UserPassword("git", token)
-		stage = append(stage, &core.PipelineTask{
-			Plugin: "gitextractor",
-			Options: map[string]interface{}{
-				"url":    cloneUrl.String(),
-				"repoId": didgen.NewDomainIdGenerator(&models.GithubRepo{}).Generate(connectionId, apiRepo.GithubId),
-				"proxy":  connection.Proxy,
-			},
-		})
-	}
-	// dora
-	if productionPattern, ok := transformationRules["productionPattern"]; ok && productionPattern != nil {
-		j := i + 1
-		if j == len(plan) {
-			plan = append(plan, nil)
-		}
-		// add a new task to next stage
-		if plan[j] != nil {
-			j++
-		}
-		if j == len(plan) {
-			plan = append(plan, nil)
+		options["connectionId"] = connection.ID
+		options["transformationRules"] = transformationRules
+		// make sure task options is valid
+		op, err := tasks.DecodeAndValidateTaskOptions(options)
+		if err != nil {
+			return nil, err
 		}
+		// construct subtasks
+		subtasks, err := helper.MakePipelinePlanSubtasks(subtaskMetas, scopeElem.Entities)
 		if err != nil {
 			return nil, err
 		}
-		if apiRepo == nil {
-			if connection == nil {
-				connection = new(models.GithubConnection)
-				err = connectionHelper.FirstById(connection, connectionId)
-				if err != nil {
-					return nil, err
-				}
-			}
+		stage := plan[i]
+		if stage == nil {
+			stage = core.PipelineStage{}
+		}
+		stage = append(stage, &core.PipelineTask{
+			Plugin:   "github",
+			Subtasks: subtasks,
+			Options:  options,
+		})
+		// collect git data by gitextractor if CODE was requested
+		if utils.StringsContains(scopeElem.Entities, core.DOMAIN_TYPE_CODE) {
+			// here is the tricky part, we have to obtain the repo id beforehand
 			token := strings.Split(connection.Token, ",")[0]
-			apiRepo = new(tasks.GithubApiRepo)
-			err = getApiRepo(connection, token, op, apiRepo)
+			repo, err = getApiRepoIfNil(op)
+			if err != nil {
+				return nil, err
+			}
+			cloneUrl, err := errors.Convert01(url.Parse(repo.CloneUrl))
+			if err != nil {
+				return nil, err
+			}
+			cloneUrl.User = url.UserPassword("git", token)
+			stage = append(stage, &core.PipelineTask{
+				Plugin: "gitextractor",
+				Options: map[string]interface{}{
+					"url":    cloneUrl.String(),
+					"repoId": didgen.NewDomainIdGenerator(&models.GithubRepo{}).Generate(connection.ID, repo.GithubId),
+					"proxy":  connection.Proxy,
+				},
+			})
+		}
+		// dora
+		if productionPattern, ok := transformationRules["productionPattern"]; ok && productionPattern != nil {
+			j := i + 1
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			// add a new task to next stage
+			if plan[j] != nil {
+				j++
+			}
+			if j == len(plan) {
+				plan = append(plan, nil)
+			}
+			if err != nil {
+				return nil, err
+			}
+			repo, err = getApiRepoIfNil(op)
 			if err != nil {
 				return nil, err
 			}
+
+			doraOption := make(map[string]interface{})

Review Comment:
   Can we move 173 ~ 177 to 182 as part of the stage in initialize style declaration to make it more readable?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [incubator-devlake] klesh commented on a diff in pull request #3403: Fix makepipelineplan

Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #3403:
URL: https://github.com/apache/incubator-devlake/pull/3403#discussion_r994212751


##########
plugins/github/api/blueprint_test.go:
##########
@@ -66,19 +81,58 @@ func TestProcessScope(t *testing.T) {
               "productionPattern": "xxxx"
             }`),
 	}
-	apiRepo := &tasks.GithubApiRepo{
-		GithubId: 123,
-		CloneUrl: "HttpUrlToRepo",
-	}
 	scopes := make([]*core.BlueprintScopeV100, 0)
 	scopes = append(scopes, bs)
-	plan := make(core.PipelinePlan, len(scopes))
-	for i, scopeElem := range scopes {
-		plan, err = processScope(nil, 1, scopeElem, i, plan, apiRepo, connection)
-		assert.Nil(t, err)
+	plan, err := makePipelinePlan(nil, scopes, mockApiCLient, connection)
+	assert.Nil(t, err)
+
+	//planJson, err1 := json.Marshal(plan)

Review Comment:
   Plese remove commented code



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org