From 9f92c243082e8a4f599cc8991dd7772a9ce3ccbb Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Mon, 26 Jun 2023 14:17:45 -0600 Subject: [PATCH] fix(schedules): updated_at automatically updates to any change to row (#894) * fix(schedules): updated_at automatically updates to any change to row * revert docker compose * change config to fields and improve comment * what is your why --- api/schedule/create.go | 2 +- api/schedule/update.go | 2 +- cmd/vela-server/schedule.go | 2 +- database/schedule/interface.go | 2 +- database/schedule/update.go | 19 +++++--- database/schedule/update_test.go | 76 +++++++++++++++++++++++++++++++- 6 files changed, 90 insertions(+), 13 deletions(-) diff --git a/api/schedule/create.go b/api/schedule/create.go index c8eb92741..365df170e 100644 --- a/api/schedule/create.go +++ b/api/schedule/create.go @@ -170,7 +170,7 @@ func CreateSchedule(c *gin.Context) { dbSchedule.SetActive(true) // send API call to update the schedule - err = database.FromContext(c).UpdateSchedule(dbSchedule) + err = database.FromContext(c).UpdateSchedule(dbSchedule, true) if err != nil { retErr := fmt.Errorf("unable to set schedule %s to active: %w", dbSchedule.GetName(), err) diff --git a/api/schedule/update.go b/api/schedule/update.go index 646e55fc2..b0862132a 100644 --- a/api/schedule/update.go +++ b/api/schedule/update.go @@ -123,7 +123,7 @@ func UpdateSchedule(c *gin.Context) { } // update the schedule within the database - err = database.FromContext(c).UpdateSchedule(s) + err = database.FromContext(c).UpdateSchedule(s, true) if err != nil { retErr := fmt.Errorf("unable to update scheduled %s: %w", scheduleName, err) diff --git a/cmd/vela-server/schedule.go b/cmd/vela-server/schedule.go index ab0a56b90..a9fd38234 100644 --- a/cmd/vela-server/schedule.go +++ b/cmd/vela-server/schedule.go @@ -350,7 +350,7 @@ func processSchedule(s *library.Schedule, compiler compiler.Engine, database dat } // send API call to update schedule for ensuring scheduled_at field is set - err = database.UpdateSchedule(s) + err = database.UpdateSchedule(s, false) if err != nil { return fmt.Errorf("unable to update schedule %s/%s: %w", r.GetFullName(), s.GetName(), err) } diff --git a/database/schedule/interface.go b/database/schedule/interface.go index 8aa14efb5..afa163063 100644 --- a/database/schedule/interface.go +++ b/database/schedule/interface.go @@ -45,5 +45,5 @@ type ScheduleInterface interface { // ListSchedulesForRepo defines a function that gets a list of schedules by repo ID. ListSchedulesForRepo(*library.Repo, int, int) ([]*library.Schedule, int64, error) // UpdateSchedule defines a function that updates an existing schedule. - UpdateSchedule(*library.Schedule) error + UpdateSchedule(*library.Schedule, bool) error } diff --git a/database/schedule/update.go b/database/schedule/update.go index b69f8eff0..dd03f62c5 100644 --- a/database/schedule/update.go +++ b/database/schedule/update.go @@ -2,7 +2,6 @@ // // Use of this source code is governed by the LICENSE file in this repository. -//nolint:dupl // ignore similar code with create.go package schedule import ( @@ -13,7 +12,7 @@ import ( ) // UpdateSchedule updates an existing schedule in the database. -func (e *engine) UpdateSchedule(s *library.Schedule) error { +func (e *engine) UpdateSchedule(s *library.Schedule, fields bool) error { e.logger.WithFields(logrus.Fields{ "schedule": s.GetName(), }).Tracef("updating schedule %s in the database", s.GetName()) @@ -27,9 +26,15 @@ func (e *engine) UpdateSchedule(s *library.Schedule) error { return err } - // send query to the database - return e.client. - Table(constants.TableSchedule). - Save(schedule). - Error + // If "fields" is true, update entire record; otherwise, just update scheduled_at (part of processSchedule) + // + // we do this because Gorm will automatically set `updated_at` with the Save function + // and the `updated_at` field should reflect the last time a user updated the record, rather than the scheduler + if fields { + err = e.client.Table(constants.TableSchedule).Save(schedule).Error + } else { + err = e.client.Table(constants.TableSchedule).Model(schedule).UpdateColumn("scheduled_at", s.GetScheduledAt()).Error + } + + return err } diff --git a/database/schedule/update_test.go b/database/schedule/update_test.go index 05fff4286..2554cefdc 100644 --- a/database/schedule/update_test.go +++ b/database/schedule/update_test.go @@ -11,7 +11,7 @@ import ( "github.com/DATA-DOG/go-sqlmock" ) -func TestSchedule_Engine_UpdateSchedule(t *testing.T) { +func TestSchedule_Engine_UpdateSchedule_Config(t *testing.T) { _repo := testRepo() _repo.SetID(1) _repo.SetOrg("foo") @@ -67,7 +67,79 @@ WHERE "id" = $10`). // run tests for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err = test.database.UpdateSchedule(_schedule) + err = test.database.UpdateSchedule(_schedule, true) + + if test.failure { + if err == nil { + t.Errorf("UpdateSchedule for %s should have returned err", test.name) + } + + return + } + + if err != nil { + t.Errorf("UpdateSchedule for %s returned err: %v", test.name, err) + } + }) + } +} + +func TestSchedule_Engine_UpdateSchedule_NotConfig(t *testing.T) { + _repo := testRepo() + _repo.SetID(1) + _repo.SetOrg("foo") + _repo.SetName("bar") + _repo.SetFullName("foo/bar") + + _schedule := testSchedule() + _schedule.SetID(1) + _schedule.SetRepoID(1) + _schedule.SetName("nightly") + _schedule.SetEntry("0 0 * * *") + _schedule.SetCreatedAt(1) + _schedule.SetCreatedBy("user1") + _schedule.SetUpdatedAt(1) + _schedule.SetUpdatedBy("user2") + _schedule.SetScheduledAt(1) + + _postgres, _mock := testPostgres(t) + defer func() { _sql, _ := _postgres.client.DB(); _sql.Close() }() + + // ensure the mock expects the query + _mock.ExpectExec(`UPDATE "schedules" SET "scheduled_at"=$1 WHERE "id" = $2`). + WithArgs(1, 1). + WillReturnResult(sqlmock.NewResult(1, 1)) + + _sqlite := testSqlite(t) + defer func() { _sql, _ := _sqlite.client.DB(); _sql.Close() }() + + err := _sqlite.CreateSchedule(_schedule) + if err != nil { + t.Errorf("unable to create test schedule 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.UpdateSchedule(_schedule, false) if test.failure { if err == nil {