From d90fdb7c11ad8613cd7be3939bbc38e9374264b1 Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Tue, 17 Oct 2023 11:34:10 -0400 Subject: [PATCH] fix(admin/clean): clean executable when build is cleaned (#988) * fix(admin/clean): clean executable when build is cleaned * report number of rows affected * integration test with rows affected --------- Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com> --- api/admin/clean.go | 16 ++- database/executable/clean.go | 41 ++++++++ database/executable/clean_test.go | 162 ++++++++++++++++++++++++++++++ database/executable/interface.go | 2 + database/integration_test.go | 28 ++++++ 5 files changed, 246 insertions(+), 3 deletions(-) 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..efe3819c6 100644 --- a/api/admin/clean.go +++ b/api/admin/clean.go @@ -108,10 +108,20 @@ func CleanResources(c *gin.Context) { logrus.Infof("platform admin %s: cleaned %d builds in database", u.GetName(), builds) + // clean executables + 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 { - 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) @@ -123,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) @@ -132,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 new file mode 100644 index 000000000..34579f778 --- /dev/null +++ b/database/executable/clean.go @@ -0,0 +1,41 @@ +// 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) (int64, error) { + logrus.Trace("clearing build executables in the database") + + switch e.config.Driver { + case constants.DriverPostgres: + res := e.client.Exec(CleanExecutablesPostgres) + return res.RowsAffected, res.Error + default: + res := e.client.Exec(CleanExecutablesSqlite) + return res.RowsAffected, res.Error + } +} diff --git a/database/executable/clean_test.go b/database/executable/clean_test.go new file mode 100644 index 000000000..edcb0cf21 --- /dev/null +++ b/database/executable/clean_test.go @@ -0,0 +1,162 @@ +// 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) { + got, 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) + } + + 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) + } + }) + } +} + +// 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..ee0cd056f 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) (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. diff --git a/database/integration_test.go b/database/integration_test.go index 6aa55502d..bcf2fcd7f 100644 --- a/database/integration_test.go +++ b/database/integration_test.go @@ -423,6 +423,34 @@ 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) + } + + 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") + } + + methods["CleanBuildExecutables"] = true + // ensure we called all the methods we expected to for method, called := range methods { if !called {