From 255b90eda8fb9387d1eebbc865a80cf7f4a47cfa Mon Sep 17 00:00:00 2001 From: ecrupper Date: Fri, 13 Oct 2023 17:16:48 -0500 Subject: [PATCH 1/3] fix(admin/clean): clean executable when build is cleaned --- api/admin/clean.go | 10 ++ database/executable/clean.go | 40 ++++++++ database/executable/clean_test.go | 158 ++++++++++++++++++++++++++++++ database/executable/interface.go | 2 + database/integration_test.go | 24 +++++ 5 files changed, 234 insertions(+) create mode 100644 database/executable/clean.go create mode 100644 database/executable/clean_test.go diff --git a/api/admin/clean.go b/api/admin/clean.go index 39d8c32a4..26d574fa9 100644 --- a/api/admin/clean.go +++ b/api/admin/clean.go @@ -108,6 +108,16 @@ func CleanResources(c *gin.Context) { logrus.Infof("platform admin %s: cleaned %d builds in database", u.GetName(), builds) + // clean executables + err = database.FromContext(c).CleanBuildExecutables(ctx) + if err != nil { + retErr := fmt.Errorf("%d builds cleaned. unable to clean build executables: %w", builds, err) + + util.HandleError(c, http.StatusInternalServerError, retErr) + + return + } + // clean services services, err := database.FromContext(c).CleanServices(ctx, msg, before) if err != nil { diff --git a/database/executable/clean.go b/database/executable/clean.go new file mode 100644 index 000000000..eb2d0390a --- /dev/null +++ b/database/executable/clean.go @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executable + +import ( + "context" + + "github.com/go-vela/types/constants" + "github.com/sirupsen/logrus" +) + +const CleanExecutablesPostgres = ` + DELETE FROM build_executables + USING builds + WHERE builds.id = build_executables.build_id + AND builds.status = 'error'; +` + +const CleanExecutablesSqlite = ` + DELETE FROM build_executables + WHERE build_id IN ( + SELECT build_id FROM build_executables e + INNER JOIN builds b + ON e.build_id=b.id + WHERE b.status = 'error' + ); +` + +// CleanBuildExecutables pops executables which have a corresponding build that was cleaned. +func (e *engine) CleanBuildExecutables(ctx context.Context) error { + logrus.Trace("clearing build executables in the database") + + switch e.config.Driver { + case constants.DriverPostgres: + return e.client.Exec(CleanExecutablesPostgres).Error + + default: + return e.client.Exec(CleanExecutablesSqlite).Error + } +} diff --git a/database/executable/clean_test.go b/database/executable/clean_test.go new file mode 100644 index 000000000..10efaef05 --- /dev/null +++ b/database/executable/clean_test.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executable + +import ( + "context" + "fmt" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/go-vela/types/constants" + "github.com/go-vela/types/database" + "github.com/go-vela/types/library" +) + +func TestExecutable_Engine_CleanExecutables(t *testing.T) { + // setup types + _buildOne := testBuild() + _buildOne.SetID(1) + _buildOne.SetRepoID(1) + _buildOne.SetNumber(1) + _buildOne.SetStatus("pending") + _buildOne.SetCreated(1) + _buildOne.SetDeployPayload(nil) + + _buildTwo := testBuild() + _buildTwo.SetID(2) + _buildTwo.SetRepoID(1) + _buildTwo.SetNumber(2) + _buildTwo.SetStatus("error") + _buildTwo.SetCreated(1) + _buildTwo.SetDeployPayload(nil) + + _bExecutableOne := testBuildExecutable() + _bExecutableOne.SetID(1) + _bExecutableOne.SetBuildID(1) + _bExecutableOne.SetData([]byte("foo")) + + _bExecutableTwo := testBuildExecutable() + _bExecutableTwo.SetID(2) + _bExecutableTwo.SetBuildID(2) + _bExecutableTwo.SetData([]byte("bar")) + + _postgres, _mock := testPostgres(t) + defer func() { _sql, _ := _postgres.client.DB(); _sql.Close() }() + + _mock.ExpectExec("DELETE FROM build_executables USING builds WHERE builds.id = build_executables.build_id AND builds.status = 'error';"). + WillReturnResult(sqlmock.NewResult(1, 1)) + + _mock.ExpectQuery(`DELETE FROM "build_executables" WHERE build_id = $1 RETURNING *`).WithArgs(2).WillReturnError(fmt.Errorf("not found")) + + _sqlite := testSqlite(t) + defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() + + err := _sqlite.CreateBuildExecutable(context.TODO(), _bExecutableOne) + if err != nil { + t.Errorf("unable to create test build for sqlite: %v", err) + } + + err = _sqlite.CreateBuildExecutable(context.TODO(), _bExecutableTwo) + if err != nil { + t.Errorf("unable to create test build for sqlite: %v", err) + } + + err = _sqlite.client.AutoMigrate(&database.Build{}) + if err != nil { + t.Errorf("unable to create repo table for sqlite: %v", err) + } + + err = _sqlite.client.Table(constants.TableBuild).Create(database.BuildFromLibrary(_buildOne)).Error + if err != nil { + t.Errorf("unable to create test repo for sqlite: %v", err) + } + + err = _sqlite.client.Table(constants.TableBuild).Create(database.BuildFromLibrary(_buildTwo)).Error + if err != nil { + t.Errorf("unable to create test repo for sqlite: %v", err) + } + + // setup tests + tests := []struct { + failure bool + name string + database *engine + }{ + { + failure: false, + name: "postgres", + database: _postgres, + }, + { + failure: false, + name: "sqlite3", + database: _sqlite, + }, + } + + // run tests + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.database.CleanBuildExecutables(context.TODO()) + + if test.failure { + if err == nil { + t.Errorf("CleanExecutables for %s should have returned err", test.name) + } + + return + } + + if err != nil { + t.Errorf("CleanExecutables for %s returned err: %v", test.name, err) + } + + _, err = test.database.PopBuildExecutable(context.TODO(), 2) + if err == nil { + t.Errorf("CleanExecutables for %s should have returned an error", test.name) + } + }) + } +} + +// testBuild is a test helper function to create a library +// Build type with all fields set to their zero values. +func testBuild() *library.Build { + return &library.Build{ + ID: new(int64), + RepoID: new(int64), + PipelineID: new(int64), + Number: new(int), + Parent: new(int), + Event: new(string), + EventAction: new(string), + Status: new(string), + Error: new(string), + Enqueued: new(int64), + Created: new(int64), + Started: new(int64), + Finished: new(int64), + Deploy: new(string), + Clone: new(string), + Source: new(string), + Title: new(string), + Message: new(string), + Commit: new(string), + Sender: new(string), + Author: new(string), + Email: new(string), + Link: new(string), + Branch: new(string), + Ref: new(string), + BaseRef: new(string), + HeadRef: new(string), + Host: new(string), + Runtime: new(string), + Distribution: new(string), + } +} diff --git a/database/executable/interface.go b/database/executable/interface.go index fbff5048a..e6a92ded9 100644 --- a/database/executable/interface.go +++ b/database/executable/interface.go @@ -20,6 +20,8 @@ type BuildExecutableInterface interface { // // https://en.wikipedia.org/wiki/Data_manipulation_language + // CleanBuildExecutables defines a function that deletes errored builds' corresponding executables. + CleanBuildExecutables(context.Context) error // CreateBuildExecutable defines a function that creates a build executable. CreateBuildExecutable(context.Context, *library.BuildExecutable) error // PopBuildExecutable defines a function that gets and deletes a build executable. diff --git a/database/integration_test.go b/database/integration_test.go index 6aa55502d..016388602 100644 --- a/database/integration_test.go +++ b/database/integration_test.go @@ -423,6 +423,30 @@ func testExecutables(t *testing.T, db Interface, resources *Resources) { } methods["PopBuildExecutable"] = true + resources.Builds[0].SetStatus(constants.StatusError) + + _, err := db.UpdateBuild(context.TODO(), resources.Builds[0]) + if err != nil { + t.Errorf("unable to update build for clean executables test") + } + + err = db.CreateBuildExecutable(context.TODO(), resources.Executables[0]) + if err != nil { + t.Errorf("unable to create executable %d: %v", resources.Executables[0].GetID(), err) + } + + err = db.CleanBuildExecutables(context.TODO()) + if err != nil { + t.Errorf("unable to clean executable %d: %v", resources.Executables[0].GetID(), err) + } + + _, err = db.PopBuildExecutable(context.TODO(), resources.Builds[0].GetID()) + if err == nil { + t.Errorf("build executable not cleaned") + } + + methods["CleanBuildExecutables"] = true + // ensure we called all the methods we expected to for method, called := range methods { if !called { From 9c5af219679b0cbfc70c1b8e41d9595669128bfe Mon Sep 17 00:00:00 2001 From: ecrupper Date: Tue, 17 Oct 2023 08:42:19 -0500 Subject: [PATCH 2/3] report number of rows affected --- api/admin/clean.go | 8 ++++---- database/executable/clean.go | 9 +++++---- database/executable/clean_test.go | 6 +++++- database/executable/interface.go | 2 +- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/api/admin/clean.go b/api/admin/clean.go index 26d574fa9..efe3819c6 100644 --- a/api/admin/clean.go +++ b/api/admin/clean.go @@ -109,7 +109,7 @@ func CleanResources(c *gin.Context) { logrus.Infof("platform admin %s: cleaned %d builds in database", u.GetName(), builds) // clean executables - err = database.FromContext(c).CleanBuildExecutables(ctx) + executables, err := database.FromContext(c).CleanBuildExecutables(ctx) if err != nil { retErr := fmt.Errorf("%d builds cleaned. unable to clean build executables: %w", builds, err) @@ -121,7 +121,7 @@ func CleanResources(c *gin.Context) { // clean services services, err := database.FromContext(c).CleanServices(ctx, msg, before) if err != nil { - retErr := fmt.Errorf("%d builds cleaned. unable to update services: %w", builds, err) + retErr := fmt.Errorf("%d builds cleaned. %d executables cleaned. unable to update services: %w", builds, executables, err) util.HandleError(c, http.StatusInternalServerError, retErr) @@ -133,7 +133,7 @@ func CleanResources(c *gin.Context) { // clean steps steps, err := database.FromContext(c).CleanSteps(msg, before) if err != nil { - retErr := fmt.Errorf("%d builds cleaned. %d services cleaned. unable to update steps: %w", builds, services, err) + retErr := fmt.Errorf("%d builds cleaned. %d executables cleaned. %d services cleaned. unable to update steps: %w", builds, executables, services, err) util.HandleError(c, http.StatusInternalServerError, retErr) @@ -142,5 +142,5 @@ func CleanResources(c *gin.Context) { logrus.Infof("platform admin %s: cleaned %d steps in database", u.GetName(), steps) - c.JSON(http.StatusOK, fmt.Sprintf("%d builds cleaned. %d services cleaned. %d steps cleaned.", builds, services, steps)) + c.JSON(http.StatusOK, fmt.Sprintf("%d builds cleaned. %d executables cleaned. %d services cleaned. %d steps cleaned.", builds, executables, services, steps)) } diff --git a/database/executable/clean.go b/database/executable/clean.go index eb2d0390a..34579f778 100644 --- a/database/executable/clean.go +++ b/database/executable/clean.go @@ -27,14 +27,15 @@ const CleanExecutablesSqlite = ` ` // CleanBuildExecutables pops executables which have a corresponding build that was cleaned. -func (e *engine) CleanBuildExecutables(ctx context.Context) error { +func (e *engine) CleanBuildExecutables(ctx context.Context) (int64, error) { logrus.Trace("clearing build executables in the database") switch e.config.Driver { case constants.DriverPostgres: - return e.client.Exec(CleanExecutablesPostgres).Error - + res := e.client.Exec(CleanExecutablesPostgres) + return res.RowsAffected, res.Error default: - return e.client.Exec(CleanExecutablesSqlite).Error + res := e.client.Exec(CleanExecutablesSqlite) + return res.RowsAffected, res.Error } } diff --git a/database/executable/clean_test.go b/database/executable/clean_test.go index 10efaef05..edcb0cf21 100644 --- a/database/executable/clean_test.go +++ b/database/executable/clean_test.go @@ -98,7 +98,7 @@ func TestExecutable_Engine_CleanExecutables(t *testing.T) { // run tests for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := test.database.CleanBuildExecutables(context.TODO()) + got, err := test.database.CleanBuildExecutables(context.TODO()) if test.failure { if err == nil { @@ -112,6 +112,10 @@ func TestExecutable_Engine_CleanExecutables(t *testing.T) { t.Errorf("CleanExecutables for %s returned err: %v", test.name, err) } + if got != 1 { + t.Errorf("CleanExecutables for %s should have affected 1 row, affected %d", test.name, got) + } + _, err = test.database.PopBuildExecutable(context.TODO(), 2) if err == nil { t.Errorf("CleanExecutables for %s should have returned an error", test.name) diff --git a/database/executable/interface.go b/database/executable/interface.go index e6a92ded9..ee0cd056f 100644 --- a/database/executable/interface.go +++ b/database/executable/interface.go @@ -21,7 +21,7 @@ type BuildExecutableInterface interface { // https://en.wikipedia.org/wiki/Data_manipulation_language // CleanBuildExecutables defines a function that deletes errored builds' corresponding executables. - CleanBuildExecutables(context.Context) error + CleanBuildExecutables(context.Context) (int64, error) // CreateBuildExecutable defines a function that creates a build executable. CreateBuildExecutable(context.Context, *library.BuildExecutable) error // PopBuildExecutable defines a function that gets and deletes a build executable. From ce1f4dc8bbd3c4686e8e185eabb0d79fcd3a1233 Mon Sep 17 00:00:00 2001 From: ecrupper Date: Tue, 17 Oct 2023 08:48:59 -0500 Subject: [PATCH 3/3] integration test with rows affected --- database/integration_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/database/integration_test.go b/database/integration_test.go index 016388602..bcf2fcd7f 100644 --- a/database/integration_test.go +++ b/database/integration_test.go @@ -435,11 +435,15 @@ func testExecutables(t *testing.T, db Interface, resources *Resources) { t.Errorf("unable to create executable %d: %v", resources.Executables[0].GetID(), err) } - err = db.CleanBuildExecutables(context.TODO()) + count, err := db.CleanBuildExecutables(context.TODO()) if err != nil { t.Errorf("unable to clean executable %d: %v", resources.Executables[0].GetID(), err) } + if count != 1 { + t.Errorf("CleanBuildExecutables should have affected 1 row, affected %d", count) + } + _, err = db.PopBuildExecutable(context.TODO(), resources.Builds[0].GetID()) if err == nil { t.Errorf("build executable not cleaned")