Merge pull request #6637 from devtron-labs/deployment-tempalte-history-fix

fix: duplicate entries in deployment history without override

Author: kartik-579

internal/sql/repository/pipelineConfig/PipelineRepository.go

@@ -118,7 +118,7 @@ type PipelineRepository interface {
 	FindActiveByInFilter(envId int, appIdIncludes []int) (pipelines []*Pipeline, err error)
 	FindActivePipelineAppIdsByInFilter(envId int, appIdIncludes []int) ([]int, error)
 	FindActiveByNotFilter(envId int, appIdExcludes []int) (pipelines []*Pipeline, err error)
-	FindAllPipelinesByChartsOverrideAndAppIdAndChartId(chartOverridden bool, appId int, chartId int) (pipelines []*Pipeline, err error)
+	FindAllPipelinesWithoutOverriddenCharts(appId int) (pipelineIds []int, err error)
 	FindActiveByAppIdAndPipelineId(appId int, pipelineId int) ([]*Pipeline, error)
 	FindActiveByAppIdAndEnvId(appId int, envId int) (*Pipeline, error)
 	SetDeploymentAppCreatedInPipeline(deploymentAppCreated bool, pipelineId int, userId int32) error
@@ -577,19 +577,16 @@ func (impl *PipelineRepositoryImpl) FindActiveByNotFilter(envId int, appIdExclud
 	return pipelines, err
 }
 
-func (impl *PipelineRepositoryImpl) FindAllPipelinesByChartsOverrideAndAppIdAndChartId(hasConfigOverridden bool, appId int, chartId int) (pipelines []*Pipeline, err error) {
-	err = impl.dbConnection.Model(&pipelines).
-		Column("pipeline.*").
-		Join("inner join charts on pipeline.app_id = charts.app_id").
-		Join("inner join chart_env_config_override ceco on charts.id = ceco.chart_id").
-		Where("pipeline.app_id = ?", appId).
-		Where("charts.id = ?", chartId).
-		Where("ceco.is_override = ?", hasConfigOverridden).
-		Where("pipeline.deleted = ?", false).
-		Where("ceco.active = ?", true).
-		Where("charts.active = ?", true).
-		Select()
-	return pipelines, err
+func (impl *PipelineRepositoryImpl) FindAllPipelinesWithoutOverriddenCharts(appId int) (pipelineIds []int, err error) {
+	err = impl.dbConnection.Model().Table("pipeline").Column("pipeline.id").
+		Where("pipeline.deleted = ?", false).Where("pipeline.app_id = ?", appId).
+		Where(`pipeline.environment_id NOT IN (
+				SELECT ceco.target_environment FROM chart_env_config_override ceco 
+				INNER JOIN charts ON charts.id = ceco.chart_id
+				WHERE charts.app_id = ? AND charts.active = ? AND ceco.is_override = ?
+				AND ceco.active = ? AND ceco.latest = ?)`, appId, true, true, true, true).
+		Select(&pipelineIds)
+	return pipelineIds, err
 }
 
 func (impl *PipelineRepositoryImpl) FindActiveByAppIdAndPipelineId(appId int, pipelineId int) ([]*Pipeline, error) {

pkg/deployment/manifest/deploymentTemplate/DeploymentTemplateHistoryService.go

@@ -70,11 +70,16 @@ func NewDeploymentTemplateHistoryServiceImpl(logger *zap.SugaredLogger, deployme
 
 func (impl DeploymentTemplateHistoryServiceImpl) CreateDeploymentTemplateHistoryFromGlobalTemplate(chart *chartRepoRepository.Chart, tx *pg.Tx, IsAppMetricsEnabled bool) (err error) {
 	//getting all pipelines without overridden charts
-	pipelines, err := impl.pipelineRepository.FindAllPipelinesByChartsOverrideAndAppIdAndChartId(false, chart.AppId, chart.Id)
+	pipelineIds, err := impl.pipelineRepository.FindAllPipelinesWithoutOverriddenCharts(chart.AppId)
 	if err != nil && err != pg.ErrNoRows {
 		impl.logger.Errorw("err in getting pipelines, CreateDeploymentTemplateHistoryFromGlobalTemplate", "err", err, "chart", chart)
 		return err
 	}
+	/*
+		When creating base template entry, we also create entries for all pipeline whose env template is not overridden;
+		as the change in base template will also impact them. Earlier we used to create individual entries for all pipelines
+		but due to performance impact we are now saving the impacted pipelineIds in the base template entry itself (column pipeline_ids).
+	*/
 	chartRefDto, err := impl.chartRefService.FindById(chart.ChartRefId)
 	if err != nil {
 		impl.logger.Errorw("err in getting chartRef, CreateDeploymentTemplateHistoryFromGlobalTemplate", "err", err, "chart", chart)
@@ -89,6 +94,7 @@ func (impl DeploymentTemplateHistoryServiceImpl) CreateDeploymentTemplateHistory
 		TemplateName:            chartRefDto.Name,
 		TemplateVersion:         chartRefDto.Version,
 		IsAppMetricsEnabled:     IsAppMetricsEnabled,
+		PipelineIds:             pipelineIds,
 		AuditLog: sql.AuditLog{
 			CreatedOn: chart.CreatedOn,
 			CreatedBy: chart.CreatedBy,
@@ -106,34 +112,6 @@ func (impl DeploymentTemplateHistoryServiceImpl) CreateDeploymentTemplateHistory
 		impl.logger.Errorw("err in creating history entry for deployment template", "err", err, "history", historyModel)
 		return err
 	}
-	for _, pipeline := range pipelines {
-		historyModel := &repository.DeploymentTemplateHistory{
-			AppId:                   chart.AppId,
-			PipelineId:              pipeline.Id,
-			ImageDescriptorTemplate: chart.ImageDescriptorTemplate,
-			Template:                chart.GlobalOverride,
-			Deployed:                false,
-			TemplateName:            chartRefDto.Name,
-			TemplateVersion:         chartRefDto.Version,
-			IsAppMetricsEnabled:     IsAppMetricsEnabled,
-			AuditLog: sql.AuditLog{
-				CreatedOn: chart.CreatedOn,
-				CreatedBy: chart.CreatedBy,
-				UpdatedOn: chart.UpdatedOn,
-				UpdatedBy: chart.UpdatedBy,
-			},
-		}
-		//creating new entry
-		if tx != nil {
-			_, err = impl.deploymentTemplateHistoryRepository.CreateHistoryWithTxn(historyModel, tx)
-		} else {
-			_, err = impl.deploymentTemplateHistoryRepository.CreateHistory(historyModel)
-		}
-		if err != nil {
-			impl.logger.Errorw("err in creating history entry for deployment template", "err", err, "history", historyModel)
-			return err
-		}
-	}
 	return err
 }
 

pkg/pipeline/history/repository/DeploymentTemplateHistoryRepository.go

@@ -57,6 +57,7 @@ type DeploymentTemplateHistory struct {
 	DeployedOn              time.Time `sql:"deployed_on"`
 	DeployedBy              int32     `sql:"deployed_by"`
 	MergeStrategy           string    `sql:"merge_strategy"`
+	PipelineIds             []int     `sql:"pipeline_ids,array"`
 	sql.AuditLog
 	//getting below data from cd_workflow_runner and users join
 	DeploymentStatus  string `sql:"-"`

scripts/sql/33403601_alter_deployment_template_history.down.sql

@@ -0,0 +1 @@
+-- not dropping the added column as it is used for audit and data loss can be impactful. The old code can run with the column added and should not cause any issue.

scripts/sql/33403601_alter_deployment_template_history.up.sql

@@ -0,0 +1,2 @@
+ALTER TABLE deployment_template_history
+    ADD COLUMN IF NOT EXISTS pipeline_ids integer[];