From 746aa9168f70946883505aebb6fc81a99c5bc39f Mon Sep 17 00:00:00 2001 From: ecrupper Date: Tue, 22 Aug 2023 09:10:49 -0500 Subject: [PATCH] refactor(db): return step on created and updated --- api/admin/step.go | 4 ++-- api/build/cancel.go | 2 +- api/build/clean.go | 2 +- api/step/create.go | 5 +---- api/step/plan.go | 8 +------- api/step/update.go | 5 +---- database/integration_test.go | 9 ++------- database/step/clean_test.go | 8 ++++---- database/step/count_build_test.go | 4 ++-- database/step/count_test.go | 4 ++-- database/step/create.go | 11 +++++------ database/step/create_test.go | 7 ++++++- database/step/delete_test.go | 2 +- database/step/get_build_test.go | 2 +- database/step/get_test.go | 2 +- database/step/interface.go | 4 ++-- database/step/list_build_test.go | 4 ++-- database/step/list_image_test.go | 4 ++-- database/step/list_status_test.go | 4 ++-- database/step/list_test.go | 4 ++-- database/step/update.go | 11 +++++------ database/step/update_test.go | 9 +++++++-- router/middleware/step/step_test.go | 2 +- 23 files changed, 54 insertions(+), 63 deletions(-) diff --git a/api/admin/step.go b/api/admin/step.go index 5f03aff6b..2da12ece1 100644 --- a/api/admin/step.go +++ b/api/admin/step.go @@ -66,7 +66,7 @@ func UpdateStep(c *gin.Context) { } // send API call to update the step - err = database.FromContext(c).UpdateStep(input) + s, err := database.FromContext(c).UpdateStep(input) if err != nil { retErr := fmt.Errorf("unable to update step %d: %w", input.GetID(), err) @@ -75,5 +75,5 @@ func UpdateStep(c *gin.Context) { return } - c.JSON(http.StatusOK, input) + c.JSON(http.StatusOK, s) } diff --git a/api/build/cancel.go b/api/build/cancel.go index d8d4e6c06..55dc7cecf 100644 --- a/api/build/cancel.go +++ b/api/build/cancel.go @@ -231,7 +231,7 @@ func CancelBuild(c *gin.Context) { if step.GetStatus() == constants.StatusRunning || step.GetStatus() == constants.StatusPending { step.SetStatus(constants.StatusCanceled) - err = database.FromContext(c).UpdateStep(step) + _, err = database.FromContext(c).UpdateStep(step) if err != nil { retErr := fmt.Errorf("unable to update step %s for build %s: %w", step.GetName(), entry, err) util.HandleError(c, http.StatusNotFound, retErr) diff --git a/api/build/clean.go b/api/build/clean.go index a93a9355c..0dd9ea81e 100644 --- a/api/build/clean.go +++ b/api/build/clean.go @@ -49,7 +49,7 @@ func CleanBuild(ctx context.Context, database database.Interface, b *library.Bui s.SetFinished(time.Now().UTC().Unix()) // send API call to update the step - err := database.UpdateStep(s) + _, err := database.UpdateStep(s) if err != nil { logrus.Errorf("unable to kill step %s for build %d: %v", s.GetName(), b.GetNumber(), err) } diff --git a/api/step/create.go b/api/step/create.go index d87bfaae0..58e71a99e 100644 --- a/api/step/create.go +++ b/api/step/create.go @@ -112,7 +112,7 @@ func CreateStep(c *gin.Context) { } // send API call to create the step - err = database.FromContext(c).CreateStep(input) + s, err := database.FromContext(c).CreateStep(input) if err != nil { retErr := fmt.Errorf("unable to create step for build %s: %w", entry, err) @@ -121,8 +121,5 @@ func CreateStep(c *gin.Context) { return } - // send API call to capture the created step - s, _ := database.FromContext(c).GetStepForBuild(b, input.GetNumber()) - c.JSON(http.StatusCreated, s) } diff --git a/api/step/plan.go b/api/step/plan.go index b072cf4f7..daf795284 100644 --- a/api/step/plan.go +++ b/api/step/plan.go @@ -61,17 +61,11 @@ func planStep(database database.Interface, b *library.Build, c *pipeline.Contain s.SetCreated(time.Now().UTC().Unix()) // send API call to create the step - err := database.CreateStep(s) + s, err := database.CreateStep(s) if err != nil { return nil, fmt.Errorf("unable to create step %s: %w", s.GetName(), err) } - // send API call to capture the created step - s, err = database.GetStepForBuild(b, s.GetNumber()) - if err != nil { - return nil, fmt.Errorf("unable to get step %s: %w", s.GetName(), err) - } - // populate environment variables from step library // // https://pkg.go.dev/github.com/go-vela/types/library#step.Environment diff --git a/api/step/update.go b/api/step/update.go index 8a64fbb12..d0563d98d 100644 --- a/api/step/update.go +++ b/api/step/update.go @@ -147,7 +147,7 @@ func UpdateStep(c *gin.Context) { } // send API call to update the step - err = database.FromContext(c).UpdateStep(s) + s, err = database.FromContext(c).UpdateStep(s) if err != nil { retErr := fmt.Errorf("unable to update step %s: %w", entry, err) @@ -156,8 +156,5 @@ func UpdateStep(c *gin.Context) { return } - // send API call to capture the updated step - s, _ = database.FromContext(c).GetStepForBuild(b, s.GetNumber()) - c.JSON(http.StatusOK, s) } diff --git a/database/integration_test.go b/database/integration_test.go index cd4e3f9a7..626040c91 100644 --- a/database/integration_test.go +++ b/database/integration_test.go @@ -1429,7 +1429,7 @@ func testSteps(t *testing.T, db Interface, resources *Resources) { // create the steps for _, step := range resources.Steps { - err := db.CreateStep(step) + _, err := db.CreateStep(step) if err != nil { t.Errorf("unable to create step %d: %v", step.GetID(), err) } @@ -1534,16 +1534,11 @@ func testSteps(t *testing.T, db Interface, resources *Resources) { // update the steps for _, step := range resources.Steps { step.SetStatus("success") - err = db.UpdateStep(step) + got, err := db.UpdateStep(step) if err != nil { t.Errorf("unable to update step %d: %v", step.GetID(), err) } - // lookup the step by ID - got, err := db.GetStep(step.GetID()) - if err != nil { - t.Errorf("unable to get step %d by ID: %v", step.GetID(), err) - } if !reflect.DeepEqual(got, step) { t.Errorf("GetStep() is %v, want %v", got, step) } diff --git a/database/step/clean_test.go b/database/step/clean_test.go index 4d0c68e07..e772751ac 100644 --- a/database/step/clean_test.go +++ b/database/step/clean_test.go @@ -64,22 +64,22 @@ func TestStep_Engine_CleanStep(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepThree) + _, err = _sqlite.CreateStep(_stepThree) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepFour) + _, err = _sqlite.CreateStep(_stepFour) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/count_build_test.go b/database/step/count_build_test.go index 401e1d6b3..df5a830f4 100644 --- a/database/step/count_build_test.go +++ b/database/step/count_build_test.go @@ -46,12 +46,12 @@ func TestStep_Engine_CountStepsForBuild(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/count_test.go b/database/step/count_test.go index eff1fec8c..dc473bd96 100644 --- a/database/step/count_test.go +++ b/database/step/count_test.go @@ -41,12 +41,12 @@ func TestStep_Engine_CountSteps(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/create.go b/database/step/create.go index 03ec3a953..2d18bc9ce 100644 --- a/database/step/create.go +++ b/database/step/create.go @@ -12,7 +12,7 @@ import ( ) // CreateStep creates a new step in the database. -func (e *engine) CreateStep(s *library.Step) error { +func (e *engine) CreateStep(s *library.Step) (*library.Step, error) { e.logger.WithFields(logrus.Fields{ "step": s.GetNumber(), }).Tracef("creating step %s in the database", s.GetName()) @@ -27,12 +27,11 @@ func (e *engine) CreateStep(s *library.Step) error { // https://pkg.go.dev/github.com/go-vela/types/database#Step.Validate err := step.Validate() if err != nil { - return err + return nil, err } // send query to the database - return e.client. - Table(constants.TableStep). - Create(step). - Error + result := e.client.Table(constants.TableStep).Create(step) + + return step.ToLibrary(), result.Error } diff --git a/database/step/create_test.go b/database/step/create_test.go index 1f7b3e1b8..0bf3ffca6 100644 --- a/database/step/create_test.go +++ b/database/step/create_test.go @@ -5,6 +5,7 @@ package step import ( + "reflect" "testing" "github.com/DATA-DOG/go-sqlmock" @@ -57,7 +58,7 @@ VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16) RETURNING "id"`) // run tests for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := test.database.CreateStep(_step) + got, err := test.database.CreateStep(_step) if test.failure { if err == nil { @@ -70,6 +71,10 @@ VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16) RETURNING "id"`) if err != nil { t.Errorf("CreateStep for %s returned err: %v", test.name, err) } + + if !reflect.DeepEqual(got, _step) { + t.Errorf("CreateStep for %s returned %s, want %s", test.name, got, _step) + } }) } } diff --git a/database/step/delete_test.go b/database/step/delete_test.go index c3b35ee8a..d77f81a25 100644 --- a/database/step/delete_test.go +++ b/database/step/delete_test.go @@ -31,7 +31,7 @@ func TestStep_Engine_DeleteStep(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_step) + _, err := _sqlite.CreateStep(_step) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/get_build_test.go b/database/step/get_build_test.go index ce4bfff37..8428598f5 100644 --- a/database/step/get_build_test.go +++ b/database/step/get_build_test.go @@ -41,7 +41,7 @@ func TestStep_Engine_GetStepForBuild(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_step) + _, err := _sqlite.CreateStep(_step) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/get_test.go b/database/step/get_test.go index 382fa0b7c..b30ced1b0 100644 --- a/database/step/get_test.go +++ b/database/step/get_test.go @@ -36,7 +36,7 @@ func TestStep_Engine_GetStep(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_step) + _, err := _sqlite.CreateStep(_step) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/interface.go b/database/step/interface.go index 68423c8c9..e7a377d26 100644 --- a/database/step/interface.go +++ b/database/step/interface.go @@ -31,7 +31,7 @@ type StepInterface interface { // CountStepsForBuild defines a function that gets the count of steps by build ID. CountStepsForBuild(*library.Build, map[string]interface{}) (int64, error) // CreateStep defines a function that creates a new step. - CreateStep(*library.Step) error + CreateStep(*library.Step) (*library.Step, error) // DeleteStep defines a function that deletes an existing step. DeleteStep(*library.Step) error // GetStep defines a function that gets a step by ID. @@ -47,5 +47,5 @@ type StepInterface interface { // ListStepStatusCount defines a function that gets a list of all step statuses and the count of their occurrence. ListStepStatusCount() (map[string]float64, error) // UpdateStep defines a function that updates an existing step. - UpdateStep(*library.Step) error + UpdateStep(*library.Step) (*library.Step, error) } diff --git a/database/step/list_build_test.go b/database/step/list_build_test.go index c79100eae..4ebe4c0be 100644 --- a/database/step/list_build_test.go +++ b/database/step/list_build_test.go @@ -56,12 +56,12 @@ func TestStep_Engine_ListStepsForBuild(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/list_image_test.go b/database/step/list_image_test.go index 574545493..b7170634f 100644 --- a/database/step/list_image_test.go +++ b/database/step/list_image_test.go @@ -41,12 +41,12 @@ func TestStep_Engine_ListStepImageCount(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/list_status_test.go b/database/step/list_status_test.go index 57f500650..7716b02e9 100644 --- a/database/step/list_status_test.go +++ b/database/step/list_status_test.go @@ -46,12 +46,12 @@ func TestStep_Engine_ListStepStatusCount(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/list_test.go b/database/step/list_test.go index cb1c05cb2..c98852b1b 100644 --- a/database/step/list_test.go +++ b/database/step/list_test.go @@ -51,12 +51,12 @@ func TestStep_Engine_ListSteps(t *testing.T) { _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_stepOne) + _, err := _sqlite.CreateStep(_stepOne) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } - err = _sqlite.CreateStep(_stepTwo) + _, err = _sqlite.CreateStep(_stepTwo) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } diff --git a/database/step/update.go b/database/step/update.go index 87f8d2aa9..c9e5f73d0 100644 --- a/database/step/update.go +++ b/database/step/update.go @@ -12,7 +12,7 @@ import ( ) // UpdateStep updates an existing step in the database. -func (e *engine) UpdateStep(s *library.Step) error { +func (e *engine) UpdateStep(s *library.Step) (*library.Step, error) { e.logger.WithFields(logrus.Fields{ "step": s.GetNumber(), }).Tracef("updating step %s in the database", s.GetName()) @@ -27,12 +27,11 @@ func (e *engine) UpdateStep(s *library.Step) error { // https://pkg.go.dev/github.com/go-vela/types/database#Step.Validate err := step.Validate() if err != nil { - return err + return nil, err } // send query to the database - return e.client. - Table(constants.TableStep). - Save(step). - Error + result := e.client.Table(constants.TableStep).Save(step) + + return step.ToLibrary(), result.Error } diff --git a/database/step/update_test.go b/database/step/update_test.go index c2a10a113..38d8c1bdb 100644 --- a/database/step/update_test.go +++ b/database/step/update_test.go @@ -5,6 +5,7 @@ package step import ( + "reflect" "testing" "github.com/DATA-DOG/go-sqlmock" @@ -33,7 +34,7 @@ WHERE "id" = $16`). _sqlite := testSqlite(t) defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() - err := _sqlite.CreateStep(_step) + _, err := _sqlite.CreateStep(_step) if err != nil { t.Errorf("unable to create test step for sqlite: %v", err) } @@ -59,7 +60,7 @@ WHERE "id" = $16`). // run tests for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err = test.database.UpdateStep(_step) + got, err := test.database.UpdateStep(_step) if test.failure { if err == nil { @@ -72,6 +73,10 @@ WHERE "id" = $16`). if err != nil { t.Errorf("UpdateStep for %s returned err: %v", test.name, err) } + + if !reflect.DeepEqual(got, _step) { + t.Errorf("UpdateStep for %s returned %s, want %s", test.name, got, _step) + } }) } } diff --git a/router/middleware/step/step_test.go b/router/middleware/step/step_test.go index 6c9aede32..9ca2c8faf 100644 --- a/router/middleware/step/step_test.go +++ b/router/middleware/step/step_test.go @@ -89,7 +89,7 @@ func TestStep_Establish(t *testing.T) { _, _ = db.CreateRepo(context.TODO(), r) _, _ = db.CreateBuild(context.TODO(), b) - _ = db.CreateStep(want) + _, _ = db.CreateStep(want) // setup context gin.SetMode(gin.TestMode)