code review comments

go.mod

@@ -216,6 +216,7 @@ require (
 	github.com/prometheus/procfs v0.8.0 // indirect
 	github.com/robfig/cron v1.2.0 // indirect
 	github.com/russross/blackfriday v1.5.2 // indirect
+	github.com/samber/lo v1.38.1 // indirect
 	github.com/sergi/go-diff v1.1.0 // indirect
 	github.com/shopspring/decimal v1.2.0 // indirect
 	github.com/sirupsen/logrus v1.9.0 // indirect

go.sum

@@ -1038,6 +1038,8 @@ github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR
 github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
 github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
+github.com/samber/lo v1.38.1 h1:j2XEAqXKb09Am4ebOg31SpvzUTTs6EN3VfgeLUhPdXM=
+github.com/samber/lo v1.38.1/go.mod h1:+m/ZKRl6ClXCE2Lgf3MsQlWfh4bn1bz6CXEOxnEXnEA=
 github.com/samuel/go-zookeeper v0.0.0-20190923202752-2cc03de413da/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E=
 github.com/sanity-io/litter v1.2.0/go.mod h1:JF6pZUFgu2Q0sBZ+HSV35P8TVPI1TTzEwyu9FXAw2W4=
 github.com/satori/go.uuid v1.2.0 h1:0uYX9dsZ2yD7q2RtLRtPSdGDWzjeM3TbMJP9utgA0ww=

internal/sql/repository/CiArtifactRepository.go

@@ -241,12 +241,14 @@ func (impl CiArtifactRepositoryImpl) GetArtifactsByCDPipeline(cdPipelineId, limi
 }
 
 func (impl CiArtifactRepositoryImpl) GetArtifactsByCDPipelineV3(listingFilterOpts *bean.ArtifactsListFilterOptions) ([]*CiArtifact, error) {
+	//TODO Gireesh: listingFilterOpts.SearchString should be conditional,
 	artifacts := make([]*CiArtifact, 0, listingFilterOpts.Limit)
 	commonPaginationQueryPart := " cia.image ILIKE %?%" +
 		" ORDER BY cia.id DESC" +
 		" LIMIT ?" +
 		" OFFSET ?;"
 	if listingFilterOpts.ParentStageType == bean.CI_WORKFLOW_TYPE {
+		//TODO Gireesh: listingFilterOpts.PipelineId is ciPipelineId in this case why are we taking join
 		query := " SELECT cia.* " +
 			" FROM ci_artifact cia" +
 			" INNER JOIN ci_pipeline cp ON cp.id=cia.pipeline_id" +
@@ -291,6 +293,7 @@ func (impl CiArtifactRepositoryImpl) GetArtifactsByCDPipelineV3(listingFilterOpt
 
 	//(this will fetch all the artifacts that were deployed on the given pipeline atleast once in new->old deployed order)
 	artifactsDeployed := make([]*CiArtifact, 0, len(artifactsIds))
+	//TODO Gireesh: compare this query plan with cd_workflow & cd_workflow_runner join query Plan, since pco is heavy
 	query := " SELECT cia.id,pco.created_on AS created_on " +
 		" FROM ci_artifact cia" +
 		" INNER JOIN pipeline_config_override pco ON pco.ci_artifact_id=cia.id" +
@@ -312,6 +315,8 @@ func (impl CiArtifactRepositoryImpl) GetArtifactsByCDPipelineV3(listingFilterOpt
 		}
 	}
 
+	//TODO Gireesh: create separate meaningful functions of these queries
+
 	return artifacts, nil
 
 }

internal/sql/repository/pipelineConfig/CdWorfkflowRepository.go

@@ -380,11 +380,13 @@ func (impl *CdWorkflowRepositoryImpl) FindCdWorkflowMetaByPipelineId(pipelineId
 func (impl *CdWorkflowRepositoryImpl) FindArtifactByListFilter(listingFilterOptions *bean.ArtifactsListFilterOptions) ([]CdWorkflowRunner, error) {
 	var wfrList []CdWorkflowRunner
 	var wfIds []int
+	//TODO Gireesh: why are we extracting artifacts which belongs to current pipeline as it will impact page size of response ??
 	query := impl.dbConnection.Model(&wfIds).
 		Column("MAX(cd_workflow_runner.id) AS id").
 		Join("INNER JOIN cd_workflow ON cd_workflow.id=cd_workflow_runner.cd_workflow_id").
 		Join("INNER JOIN ci_artifact cia ON cia.id = cd_workflow.ci_artifact_id").
-		Where("(cd_workflow.pipeline_id = ? AND cd_workflow_runner.workflow_type = ?) OR (cd_workflow.pipeline_id = ? AND cd_workflow_runner.workflow_type = ? AND cd_workflow_runner.status IN (?))",
+		Where("(cd_workflow.pipeline_id = ? AND cd_workflow_runner.workflow_type = ?) "+
+			"OR (cd_workflow.pipeline_id = ? AND cd_workflow_runner.workflow_type = ? AND cd_workflow_runner.status IN (?))",
 			listingFilterOptions.PipelineId,
 			listingFilterOptions.StageType,
 			listingFilterOptions.ParentId,

pkg/pipeline/AppArtifactManager.go

@@ -26,6 +26,7 @@ import (
 	repository2 "github.com/devtron-labs/devtron/pkg/pipeline/repository"
 	"github.com/devtron-labs/devtron/pkg/user"
 	"github.com/go-pg/pg"
+	lo "github.com/samber/lo"
 	"go.uber.org/zap"
 	"sort"
 )
@@ -393,12 +394,10 @@ func (impl *AppArtifactManagerImpl) extractParentMetaDataByPipeline(pipeline *pi
 	// retrieve parent details
 	parentId, parentType, err = impl.cdPipelineConfigService.RetrieveParentDetails(pipeline.Id)
 	if err != nil {
-		impl.logger.Errorw("failed to retrieve parent details",
-			"cdPipelineId", pipeline.Id,
-			"err", err)
 		return parentId, parentType, parentCdId, err
 	}
 
+	//TODO Gireesh: why this(stage != bean.CD_WORKFLOW_TYPE_POST) check is added, explain that in comment ??
 	if parentType == bean.CD_WORKFLOW_TYPE_POST || (parentType == bean.CD_WORKFLOW_TYPE_DEPLOY && stage != bean.CD_WORKFLOW_TYPE_POST) {
 		// parentCdId is being set to store the artifact currently deployed on parent cd (if applicable).
 		// Parent component is CD only if parent type is POST/DEPLOY
@@ -460,7 +459,9 @@ func (impl *AppArtifactManagerImpl) RetrieveArtifactsByCDPipelineV2(pipeline *pi
 		})
 	}
 
-	artifactIds := make([]int, 0, len(ciArtifacts))
+	//TODO Gireesh: need to check this behaviour, can we use this instead of below loop ??
+	artifactIds := lo.FlatMap(ciArtifacts, func(artifact bean2.CiArtifactBean, _ int) []int { return []int{artifact.Id} })
+	//artifactIds := make([]int, 0, len(ciArtifacts))
 	for _, artifact := range ciArtifacts {
 		artifactIds = append(artifactIds, artifact.Id)
 	}
@@ -477,6 +478,7 @@ func (impl *AppArtifactManagerImpl) RetrieveArtifactsByCDPipelineV2(pipeline *pi
 		return ciArtifactsResponse, err
 	}
 
+	//TODO Gireesh: Create a meaningful func
 	for i, artifact := range ciArtifacts {
 		if imageTaggingResp := imageTagsDataMap[ciArtifacts[i].Id]; imageTaggingResp != nil {
 			ciArtifacts[i].ImageReleaseTags = imageTaggingResp
@@ -507,7 +509,6 @@ func (impl *AppArtifactManagerImpl) RetrieveArtifactsByCDPipelineV2(pipeline *pi
 			}
 		}
 		ciArtifacts[i].CiConfigureSourceType = ciWorkflow.GitTriggers[ciWorkflow.CiPipelineId].CiConfigureSourceType
-		ciArtifacts[i].CiConfigureSourceValue = ciWorkflow.GitTriggers[ciWorkflow.CiPipelineId].CiConfigureSourceValue
 	}
 
 	ciArtifactsResponse.CdPipelineId = pipeline.Id
@@ -542,6 +543,7 @@ func (impl *AppArtifactManagerImpl) BuildArtifactsList(listingFilterOpts *bean.A
 		listingFilterOpts.ExcludeArtifactIds = []int{currentRunningArtifact.Id}
 		currentRunningArtifactId = currentRunningArtifact.Id
 		currentRunningWorkflowStatus = latestWf[0].Status
+		// TODO Gireesh: move below logic to proper func belong to CiArtifactBean
 		//current deployed artifact should always be computed, as we have to show it every time
 		mInfo, err := parseMaterialInfo([]byte(currentRunningArtifact.MaterialInfo), currentRunningArtifact.DataSource)
 		if err != nil {
@@ -588,6 +590,7 @@ func (impl *AppArtifactManagerImpl) BuildArtifactsForCdStageV2(listingFilterOpts
 		impl.logger.Errorw("error in fetching cd workflow runners using filter", "filterOptions", listingFilterOpts, "err", err)
 		return nil, err
 	}
+	//TODO Gireesh: initialized array with size but are using append, not optimized solution
 	ciArtifacts := make([]*bean2.CiArtifactBean, 0, len(cdWfrList))
 
 	//get artifact running on parent cd
@@ -603,6 +606,7 @@ func (impl *AppArtifactManagerImpl) BuildArtifactsForCdStageV2(listingFilterOpts
 	}
 
 	for _, wfr := range cdWfrList {
+		//TODO Gireesh: Refactoring needed
 		mInfo, err := parseMaterialInfo([]byte(wfr.CdWorkflow.CiArtifact.MaterialInfo), wfr.CdWorkflow.CiArtifact.DataSource)
 		if err != nil {
 			mInfo = []byte("[]")
@@ -637,6 +641,7 @@ func (impl *AppArtifactManagerImpl) BuildArtifactsForCIParentV2(listingFilterOpt
 		return nil, err
 	}
 
+	//TODO Gireesh: if initialized then no need of using append, put value directly to index
 	ciArtifacts := make([]*bean2.CiArtifactBean, 0, len(artifacts))
 	for _, artifact := range artifacts {
 		mInfo, err := parseMaterialInfo([]byte(artifact.MaterialInfo), artifact.DataSource)