fix(api): clean builtin artifact actions (#4069)

close #4020

engine/api/action/builtin.go

@@ -1,8 +1,6 @@
 package action
 
 import (
-	"database/sql"
-
 	"github.com/go-gorp/gorp"
 
 	"github.com/ovh/cds/sdk"
@@ -292,144 +290,116 @@ Semver used if fully compatible with https://semver.org/
 		Advanced:    true,
 	})
 
-	return checkBuiltinAction(db, serveStaticAct)
-}
-
-// checkBuiltinAction add builtin actions in database if needed
-func checkBuiltinAction(db *gorp.DbMap, a *sdk.Action) error {
-	var name string
-	err := db.QueryRow(`SELECT action.name FROM action WHERE action.name = $1 and action.type = $2`, a.Name, sdk.BuiltinAction).Scan(&name)
-	if err != nil && err != sql.ErrNoRows {
-		return sdk.WithStack(err)
-	}
-
-	if err != nil && err == sql.ErrNoRows {
-		if err := createBuiltinAction(db, a); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-func createBuiltinAction(db *gorp.DbMap, a *sdk.Action) error {
-	tx, err := db.Begin()
-	if err != nil {
-		return sdk.WithStack(err)
-	}
-	defer tx.Rollback()
-
-	log.Info("createBuiltinAction> create builtin action %s", a.Name)
-	if err := Insert(tx, a); err != nil {
+	if err := checkBuiltinAction(db, serveStaticAct); err != nil {
 		return err
 	}
 
-	return sdk.WithStack(tx.Commit())
-}
-
-// CreateBuiltinArtifactActions  Create Action BuiltinArtifact
-func CreateBuiltinArtifactActions(db *gorp.DbMap) error {
-	var name string
-	query := `SELECT action.name FROM action where action.name = $1 and action.type = $2`
-
-	// Check ArtifactUpload action
-	err := db.QueryRow(query, sdk.ArtifactUpload, sdk.BuiltinAction).Scan(&name)
-	if err != nil && err == sql.ErrNoRows {
-		if err := createBuiltinArtifactUploadAction(db); err != nil {
-			return sdk.WrapError(err, "cannot create builtin artifact upload action")
-		}
+	artifactUpload := craftBuiltinArtifactUploadAction()
+	if err := checkBuiltinAction(db, artifactUpload); err != nil {
+		return err
 	}
 
-	// Check ArtifactDownload action
-	err = db.QueryRow(query, sdk.ArtifactDownload, sdk.BuiltinAction).Scan(&name)
-	if err != nil && err == sql.ErrNoRows {
-		if err := createBuiltinArtifactDownloadAction(db); err != nil {
-			return sdk.WrapError(err, "cannot create builtin artifact download action")
-		}
+	artifactDownload := craftBuiltinArtifactDownloadAction()
+	if err := checkBuiltinAction(db, artifactDownload); err != nil {
+		return err
 	}
 
 	return nil
 }
 
-func createBuiltinArtifactUploadAction(db *gorp.DbMap) error {
+func craftBuiltinArtifactUploadAction() *sdk.Action {
 	upload := sdk.NewAction(sdk.ArtifactUpload)
 	upload.Type = sdk.BuiltinAction
 	upload.Parameter(sdk.Parameter{
 		Name:        "path",
 		Type:        sdk.StringParameter,
-		Description: "Path of file to upload, example: ./src/yourFile.json"})
+		Description: "Path of file to upload, example: ./src/yourFile.json",
+	})
 	upload.Parameter(sdk.Parameter{
 		Name:        "tag",
 		Type:        sdk.StringParameter,
 		Description: "Artifact will be uploaded with a tag, generally {{.cds.version}}",
-		Value:       "{{.cds.version}}"})
+		Value:       "{{.cds.version}}",
+	})
 	upload.Parameter(sdk.Parameter{
 		Name:        "enabled",
 		Type:        sdk.BooleanParameter,
 		Description: "Enable artifact upload",
-		Value:       "true"})
+		Value:       "true",
+		Advanced:    true,
+	})
 	upload.Parameter(sdk.Parameter{
 		Name:        "destination",
 		Description: "Destination of this artifact. Use the name of integration attached on your project",
 		Value:       "", // empty is the default value
 		Type:        sdk.StringParameter,
 		Advanced:    true,
 	})
-
-	tx, err := db.Begin()
-	if err != nil {
-		return sdk.WithStack(err)
-	}
-	defer tx.Rollback()
-
-	log.Info("createBuiltinArtifactUploadAction> create builtin action %s", upload.Name)
-	if err := Insert(tx, upload); err != nil {
-		return err
-	}
-
-	return sdk.WithStack(tx.Commit())
+	return upload
 }
 
-func createBuiltinArtifactDownloadAction(db *gorp.DbMap) error {
+func craftBuiltinArtifactDownloadAction() *sdk.Action {
 	dl := sdk.NewAction(sdk.ArtifactDownload)
 	dl.Type = sdk.BuiltinAction
 	dl.Parameter(sdk.Parameter{
 		Name:        "path",
 		Description: "Path where artifacts will be downloaded",
-		Type:        sdk.StringParameter})
+		Type:        sdk.StringParameter,
+	})
 	dl.Parameter(sdk.Parameter{
 		Name:        "tag",
 		Description: "Artifact are uploaded with a tag, generally {{.cds.version}}",
-		Type:        sdk.StringParameter})
-	dl.Parameter(sdk.Parameter{
-		Name:        "pipeline",
-		Description: "Pipeline from where artifacts will be downloaded, generally {{.cds.pipeline}} or {{.cds.parent.pipeline}}",
-		Type:        sdk.StringParameter})
-	dl.Parameter(sdk.Parameter{
-		Name:        "application",
-		Description: "Application from where artifacts will be downloaded, generally {{.cds.application}}",
-		Type:        sdk.StringParameter})
+		Type:        sdk.StringParameter,
+		Value:       "{{.cds.version}}",
+	})
 	dl.Parameter(sdk.Parameter{
 		Name:        "enabled",
 		Type:        sdk.BooleanParameter,
 		Description: "Enable artifact download",
-		Value:       "true"})
+		Value:       "true",
+		Advanced:    true,
+	})
 	dl.Parameter(sdk.Parameter{
 		Name:        "pattern",
 		Type:        sdk.StringParameter,
 		Description: "Empty: download all files. Otherwise, enter regexp pattern to choose file: (fileA|fileB)",
-		Value:       ""})
+		Value:       "",
+		Advanced:    true,
+	})
+	return dl
+}
 
+// checkBuiltinAction add builtin actions in database if needed
+func checkBuiltinAction(db *gorp.DbMap, a *sdk.Action) error {
 	tx, err := db.Begin()
 	if err != nil {
 		return sdk.WithStack(err)
 	}
 	defer tx.Rollback()
 
-	log.Info("createBuiltinArtifactDownloadAction> create builtin action %s", dl.Name)
-	if err := Insert(tx, dl); err != nil {
-		return err
+	nb, err := tx.SelectInt("SELECT COUNT(1) FROM action WHERE action.name = $1 and action.type = $2", a.Name, sdk.BuiltinAction)
+	if err != nil {
+		return sdk.WrapError(err, "unable to count action %s", a.Name)
 	}
 
+	// If the action doesn't exist, let's create
+	if nb == 0 {
+		log.Debug("createBuiltinAction> create builtin action %s", a.Name)
+		if err := Insert(tx, a); err != nil {
+			return err
+		}
+		return sdk.WithStack(tx.Commit())
+	}
+
+	id, err := tx.SelectInt("SELECT id FROM action WHERE action.name = $1 and action.type = $2", a.Name, sdk.BuiltinAction)
+	if err != nil {
+		return sdk.WrapError(err, "unable to get action %s ID", a.Name)
+	}
+
+	a.ID = id
+	log.Debug("createBuiltinAction> update builtin action %s", a.Name)
+	if err := Update(tx, a); err != nil {
+		return err
+	}
 	return sdk.WithStack(tx.Commit())
 }

engine/api/api.go

@@ -756,6 +756,9 @@ func (a *API) Serve(ctx context.Context) error {
 	migrate.Add(sdk.Migration{Name: "WorkflowNotification", Release: "0.38.1", Mandatory: true, ExecFunc: func(ctx context.Context) error {
 		return migrate.WorkflowNotifications(a.Cache, a.DBConnectionFactory.GetDBMap)
 	}})
+	migrate.Add(sdk.Migration{Name: "CleanArtifactBuiltinActions", Release: "0.39.0", Mandatory: true, ExecFunc: func(ctx context.Context) error {
+		return migrate.CleanArtifactBuiltinActions(a.Cache, a.DBConnectionFactory.GetDBMap)
+	}})
 	if os.Getenv("CDS_MIGRATE_ENABLE") == "true" {
 		migrate.Add(sdk.Migration{Name: "MigrateActionDEPRECATEDGitClone", Release: "0.37.0", Mandatory: true, ExecFunc: func(ctx context.Context) error {
 			return migrate.MigrateActionDEPRECATEDGitClone(a.mustDB, a.Cache)

engine/api/bootstrap/bootstrap.go

@@ -34,10 +34,6 @@ func InitiliazeDB(defaultValues sdk.DefaultValues, DBFunc func() *gorp.DbMap) er
 		return sdk.WrapError(err, "Cannot InitializeDefaultGroupName")
 	}
 
-	if err := action.CreateBuiltinArtifactActions(dbGorp); err != nil {
-		return sdk.WrapError(err, "Cannot setup builtin Artifact actions")
-	}
-
 	if err := action.CreateBuiltinActions(dbGorp); err != nil {
 		return sdk.WrapError(err, "Cannot setup builtin actions")
 	}

engine/api/migrate/pipeline.go

@@ -0,0 +1,84 @@
+package migrate
+
+import (
+	"github.com/go-gorp/gorp"
+	"github.com/ovh/cds/engine/api/cache"
+	"github.com/ovh/cds/engine/api/pipeline"
+	"github.com/ovh/cds/sdk"
+	"github.com/ovh/cds/sdk/log"
+)
+
+func CleanArtifactBuiltinActions(store cache.Store, DBFunc func() *gorp.DbMap) error {
+	db := DBFunc()
+
+	log.Info("migrate>CleanArtifactBuiltinActions> Start migration")
+
+	var all []struct {
+		ProjectKey   string `db:"projectkey"`
+		PipelineName string `db:"name"`
+	}
+
+	query := `SELECT project.projectkey, pipeline.name FROM pipeline JOIN project on project.id = pipeline.project_id`
+	if _, err := db.Select(&all, query); err != nil {
+		return sdk.WithStack(err)
+	}
+
+	log.Info("migrate>CleanArtifactBuiltinActions> %d pipelines to migrate", len(all))
+	for i := range all {
+		if err := migratePipelineCleanArtifactBuiltinActions(db, store, all[i].ProjectKey, all[i].PipelineName); err != nil {
+			log.Error("cannot migrate pipeline %s/%s: %v", all[i].ProjectKey, all[i].PipelineName, err)
+			continue
+		}
+	}
+
+	log.Info("migrate>End CleanArtifactBuiltinActions migration")
+	return nil
+}
+
+func migratePipelineCleanArtifactBuiltinActions(db *gorp.DbMap, store cache.Store, projetKey, pipelineName string) error {
+	tx, err := db.Begin()
+	if err != nil {
+		return sdk.WithStack(err)
+	}
+	defer tx.Rollback() // nolint
+
+	pip, err := pipeline.LoadPipeline(tx, projetKey, pipelineName, true)
+	if err != nil {
+		return sdk.WithStack(err)
+	}
+
+	paramsDownload := []string{"path", "tag", "enabled", "pattern"}
+	paramsUpload := []string{"path", "tag", "enabled", "destination"}
+
+	for _, s := range pip.Stages {
+		for _, j := range s.Jobs {
+			for i := range j.Action.Actions {
+				step := &j.Action.Actions[i]
+				var paramIDXToRemove []int
+				if step.Name == sdk.ArtifactDownload && step.Type == sdk.BuiltinAction { // Artifact download
+
+					for ip := range step.Parameters {
+						if !sdk.IsInArray(step.Parameters[ip].Name, paramsDownload) {
+							paramIDXToRemove = append(paramIDXToRemove, ip)
+						}
+					}
+				} else if step.Name == sdk.ArtifactUpload && step.Type == sdk.BuiltinAction { // Artifact upload
+
+					for ip := range step.Parameters {
+						if !sdk.IsInArray(step.Parameters[ip].Name, paramsUpload) {
+							paramIDXToRemove = append(paramIDXToRemove, ip)
+						}
+					}
+				}
+				for _, id := range paramIDXToRemove { // Remove the deprecated params
+					step.Parameters = append(step.Parameters[:id], step.Parameters[id+1:]...)
+				}
+			}
+			if err := pipeline.UpdateJob(tx, &j, 1); err != nil {
+				return sdk.WithStack(err)
+			}
+		}
+	}
+
+	return sdk.WithStack(tx.Commit())
+}

sdk/exportentities/action.go

@@ -118,10 +118,6 @@ func newSteps(a sdk.Action) []Step {
 				if tag != nil {
 					artifactDownloadArgs["tag"] = tag.Value
 				}
-				application := sdk.ParameterFind(&act.Parameters, "application")
-				if application != nil && application.Value != "" {
-					artifactDownloadArgs["application"] = application.Value
-				}
 				pattern := sdk.ParameterFind(&act.Parameters, "pattern")
 				if pattern != nil && pattern.Value != "" {
 					artifactDownloadArgs["pattern"] = pattern.Value
@@ -130,10 +126,6 @@ func newSteps(a sdk.Action) []Step {
 				if enabled != nil && enabled.Value == "false" {
 					artifactDownloadArgs["enabled"] = enabled.Value
 				}
-				pipeline := sdk.ParameterFind(&act.Parameters, "pipeline")
-				if pipeline != nil && pipeline.Value != "" {
-					artifactDownloadArgs["pipeline"] = pipeline.Value
-				}
 				s["artifactDownload"] = artifactDownloadArgs
 			case sdk.ArtifactUpload:
 				artifactUploadArgs := map[string]string{}
@@ -146,7 +138,7 @@ func newSteps(a sdk.Action) []Step {
 					artifactUploadArgs["tag"] = tag.Value
 				}
 				destination := sdk.ParameterFind(&act.Parameters, "destination")
-				if destination != nil {
+				if destination != nil && destination.Value != "" {
 					artifactUploadArgs["destination"] = destination.Value
 				}
 				s["artifactUpload"] = artifactUploadArgs
@@ -582,6 +574,8 @@ func (s Step) AsArtifactDownload() (*sdk.Action, bool, error) {
 	if err := mapstructure.Decode(bI, &argss); err != nil {
 		return nil, true, sdk.NewErrorWithStack(err, sdk.ErrMalformattedStep)
 	}
+	delete(argss, "pipeline")    // we don't want to import these old values
+	delete(argss, "application") // we don't want to import these old values
 	a := sdk.NewStepArtifactDownload(argss)
 
 	var err error