From e8b993e7dacf9c5fd018ed41f24ae9344e587be7 Mon Sep 17 00:00:00 2001 From: Michael Nutt Date: Tue, 30 Dec 2014 11:37:57 -0500 Subject: [PATCH 1/3] Add support for after_success and after_failure to email notifications This allows you to restrict email notifications to only be sent after the build changes from success to failure or failure to success. It errs on the side of sending the notification; if the build is in another state (hung, for instance) or there was no previous build on the branch the email will also be sent. Since the notify plugin shouldn't really have any responsibility for querying the database to find the previous commit's status, we store it on the commit when we save it. --- plugin/notify/email/email.go | 28 ++++++++- server/datastore/database/commit.go | 33 ++++++++-- server/datastore/database/commit_test.go | 78 ++++++++++++++++++++++++ server/datastore/database/database.go | 1 + server/datastore/migrate/migrate.go | 10 +++ shared/model/commit.go | 1 + 6 files changed, 143 insertions(+), 8 deletions(-) diff --git a/plugin/notify/email/email.go b/plugin/notify/email/email.go index f113101f5..6d38ff13a 100644 --- a/plugin/notify/email/email.go +++ b/plugin/notify/email/email.go @@ -12,9 +12,11 @@ import ( ) const ( - NotifyAlways = "always" // always send email notification - NotifyNever = "never" // never send email notifications - NotifyAuthor = "author" // only send email notifications to the author + NotifyAlways = "always" // always send email notification + NotifyNever = "never" // never send email notifications + NotifyAuthor = "author" // only send email notifications to the author + NotifyAfterFailure = "after_failure" // only send a notification if the previous commit failed + NotifyAfterSuccess = "after_success" // only send a notification if the previous commit succeeded NotifyTrue = "true" // alias for NotifyTrue NotifyFalse = "false" // alias for NotifyFalse @@ -70,6 +72,16 @@ func (e *Email) sendFailure(context *model.Request) error { switch e.Failure { case NotifyFalse, NotifyNever, NotifyOff: return nil + // if the last commit in this branch was a success, notify + case NotifyAfterSuccess: + if context.Commit.PriorStatus != "Success" { + return nil + } + // if the last commit in this branch was a failure, notify + case NotifyAfterFailure: + if context.Commit.PriorStatus != "Failure" { + return nil + } // if configured to email the author, replace // the recipiends with the commit author email. case NotifyBlame, NotifyAuthor: @@ -103,6 +115,16 @@ func (e *Email) sendSuccess(context *model.Request) error { switch e.Success { case NotifyFalse, NotifyNever, NotifyOff: return nil + // if the last commit in this branch was a success, notify + case NotifyAfterSuccess: + if context.Commit.PriorStatus == "Failure" { + return nil + } + // if the last commit in this branch was a failure, notify + case NotifyAfterFailure: + if context.Commit.PriorStatus == "Success" { + return nil + } // if configured to email the author, replace // the recipiends with the commit author email. case NotifyBlame, NotifyAuthor: diff --git a/server/datastore/database/commit.go b/server/datastore/database/commit.go index ac743e69a..72f2ecc5a 100644 --- a/server/datastore/database/commit.go +++ b/server/datastore/database/commit.go @@ -40,6 +40,15 @@ func (db *Commitstore) GetCommitLast(repo *model.Repo, branch string) (*model.Co return commit, err } +// GetCommitPrior retrieves the latest commit +// from the datastore for the specified repository +// and branch. +func (db *Commitstore) GetCommitPrior(oldCommit *model.Commit) (*model.Commit, error) { + var commit = new(model.Commit) + var err = meddler.QueryRow(db, commit, rebind(commitPriorQuery), oldCommit.RepoID, oldCommit.Branch, oldCommit.Created) + return commit, err +} + // GetCommitList retrieves a list of latest commits // from the datastore for the specified repository. func (db *Commitstore) GetCommitList(repo *model.Repo) ([]*model.Commit, error) { @@ -70,16 +79,18 @@ func (db *Commitstore) PostCommit(commit *model.Commit) error { commit.Created = time.Now().UTC().Unix() } commit.Updated = time.Now().UTC().Unix() + + priorCommit, err := db.GetCommitPrior(commit) + if err == nil { + commit.PriorStatus = priorCommit.Status + } + return meddler.Save(db, commitTable, commit) } // PutCommit saves a commit in the datastore. func (db *Commitstore) PutCommit(commit *model.Commit) error { - if commit.Created == 0 { - commit.Created = time.Now().UTC().Unix() - } - commit.Updated = time.Now().UTC().Unix() - return meddler.Save(db, commitTable, commit) + return db.PostCommit(commit) } // DelCommit removes the commit from the datastore. @@ -170,6 +181,18 @@ ORDER BY commit_id DESC LIMIT 1 ` +// SQL query to retrieve the prior Commit (by commit_created) in the same branch and repo as the specified Commit. +const commitPriorQuery = ` +SELECT * +FROM commits +WHERE repo_id = ? + AND commit_branch = ? + AND commit_created < ? + AND commit_status IN ('Success', 'Failure') +ORDER BY commit_created DESC +LIMIT 1 +` + // SQL statement to cancel all running Commits. const commitKillStmt = ` UPDATE commits SET commit_status = 'Killed' diff --git a/server/datastore/database/commit_test.go b/server/datastore/database/commit_test.go index d629ee250..974d40133 100644 --- a/server/datastore/database/commit_test.go +++ b/server/datastore/database/commit_test.go @@ -48,6 +48,84 @@ func TestCommitstore(t *testing.T) { g.Assert(commit.ID != 0).IsTrue() }) + g.It("Should Get a Commit's prior status", func() { + commit := model.Commit{ + RepoID: 1, + Branch: "foo", + Status: "Failure", + Created: 1419892236, + Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", + } + err := cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "").IsTrue() + + commit = model.Commit{ + RepoID: 1, + Branch: "foo", + Status: "Success", + Created: 1419893583, + Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", + } + err = cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "Failure").IsTrue() + }) + + g.It("Should not find prior status from commits on other branches", func() { + commit := model.Commit{ + RepoID: 1, + Branch: "foo", + Status: "Failure", + Created: 1419892236, + Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", + } + err := cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "").IsTrue() + + commit = model.Commit{ + RepoID: 1, + Branch: "bar", + Status: "Success", + Created: 1419893583, + Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", + } + err = cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "").IsTrue() + }) + + g.It("Should not find prior status from commits that didn't succeed or fail", func() { + commit := model.Commit{ + RepoID: 1, + Branch: "foo", + Status: "Pending", + Created: 1419892236, + Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", + } + err := cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "").IsTrue() + + commit = model.Commit{ + RepoID: 1, + Branch: "bar", + Status: "Success", + Created: 1419893583, + Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", + } + err = cs.PostCommit(&commit) + g.Assert(err == nil).IsTrue() + g.Assert(commit.ID != 0).IsTrue() + g.Assert(commit.PriorStatus == "").IsTrue() + }) + g.It("Should Get a Commit by ID", func() { commit := model.Commit{ RepoID: 1, diff --git a/server/datastore/database/database.go b/server/datastore/database/database.go index 6ca59d66f..aecb32e9c 100644 --- a/server/datastore/database/database.go +++ b/server/datastore/database/database.go @@ -38,6 +38,7 @@ func Connect(driver, datasource string) (*sql.DB, error) { var migrations = []migration.Migrator{ migrate.Setup, migrate.Migrate_20142110, + migrate.Migrate_20143012, } return migration.Open(driver, datasource, migrations) } diff --git a/server/datastore/migrate/migrate.go b/server/datastore/migrate/migrate.go index 42a3c57c5..5985cba10 100644 --- a/server/datastore/migrate/migrate.go +++ b/server/datastore/migrate/migrate.go @@ -39,6 +39,12 @@ func Migrate_20142110(tx migration.LimitedTx) error { return nil } +// Migrate_20143012 adds the prior commit's status to the current commit +func Migrate_20143012(tx migration.LimitedTx) error { + _, err := tx.Exec(transform(commitPriorStatusColumn)) + return err +} + var userTable = ` CREATE TABLE IF NOT EXISTS users ( user_id INTEGER PRIMARY KEY AUTOINCREMENT @@ -110,6 +116,10 @@ var repoTokenUpdate = ` UPDATE repos SET repo_token = ''; ` +var commitPriorStatusColumn = ` +ALTER TABLE commits ADD COLUMN commit_prior_status VARCHAR(255); +` + var commitTable = ` CREATE TABLE IF NOT EXISTS commits ( commit_id INTEGER PRIMARY KEY AUTOINCREMENT diff --git a/shared/model/commit.go b/shared/model/commit.go index da8c0c656..0bc3aa57e 100644 --- a/shared/model/commit.go +++ b/shared/model/commit.go @@ -8,6 +8,7 @@ type Commit struct { ID int64 `meddler:"commit_id,pk" json:"id"` RepoID int64 `meddler:"repo_id" json:"-"` Status string `meddler:"commit_status" json:"status"` + PriorStatus string `meddler:"commit_prior_status" json:"prior_status"` Started int64 `meddler:"commit_started" json:"started_at"` Finished int64 `meddler:"commit_finished" json:"finished_at"` Duration int64 `meddler:"commit_duration" json:"duration"` From b94280c15c914be162ae91e422fda62e7a174e2e Mon Sep 17 00:00:00 2001 From: Michael Nutt Date: Tue, 30 Dec 2014 13:25:14 -0500 Subject: [PATCH 2/3] Change from notification `after_success` and `after_failure` to `change` Also removes the extra db field; instead, just send Prior as part of the Request. This reverts commit e8b993e7dacf9c5fd018ed41f24ae9344e587be7. --- plugin/notify/email/email.go | 31 +++------- server/datastore/commit.go | 10 +++ server/datastore/database/commit.go | 35 +++++------ server/datastore/database/commit_test.go | 78 ------------------------ server/datastore/database/database.go | 1 - server/datastore/migrate/migrate.go | 10 --- server/worker/docker/docker.go | 6 ++ shared/model/commit.go | 1 - shared/model/request.go | 1 + 9 files changed, 43 insertions(+), 130 deletions(-) diff --git a/plugin/notify/email/email.go b/plugin/notify/email/email.go index 6d38ff13a..666c6d251 100644 --- a/plugin/notify/email/email.go +++ b/plugin/notify/email/email.go @@ -12,11 +12,10 @@ import ( ) const ( - NotifyAlways = "always" // always send email notification - NotifyNever = "never" // never send email notifications - NotifyAuthor = "author" // only send email notifications to the author - NotifyAfterFailure = "after_failure" // only send a notification if the previous commit failed - NotifyAfterSuccess = "after_success" // only send a notification if the previous commit succeeded + NotifyAlways = "always" // always send email notification + NotifyNever = "never" // never send email notifications + NotifyAuthor = "author" // only send email notifications to the author + NotifyAfterChange = "change" // only if the previous commit has a different status NotifyTrue = "true" // alias for NotifyTrue NotifyFalse = "false" // alias for NotifyFalse @@ -72,14 +71,9 @@ func (e *Email) sendFailure(context *model.Request) error { switch e.Failure { case NotifyFalse, NotifyNever, NotifyOff: return nil - // if the last commit in this branch was a success, notify - case NotifyAfterSuccess: - if context.Commit.PriorStatus != "Success" { - return nil - } - // if the last commit in this branch was a failure, notify - case NotifyAfterFailure: - if context.Commit.PriorStatus != "Failure" { + // if the last commit in this branch was a different status, notify + case NotifyAfterChange: + if context.Prior.Status == context.Commit.Status { return nil } // if configured to email the author, replace @@ -115,14 +109,9 @@ func (e *Email) sendSuccess(context *model.Request) error { switch e.Success { case NotifyFalse, NotifyNever, NotifyOff: return nil - // if the last commit in this branch was a success, notify - case NotifyAfterSuccess: - if context.Commit.PriorStatus == "Failure" { - return nil - } - // if the last commit in this branch was a failure, notify - case NotifyAfterFailure: - if context.Commit.PriorStatus == "Success" { + // if the last commit in this branch was a different status, notify + case NotifyAfterChange: + if context.Prior.Status == context.Commit.Status { return nil } // if configured to email the author, replace diff --git a/server/datastore/commit.go b/server/datastore/commit.go index 9f94d955b..802125efa 100644 --- a/server/datastore/commit.go +++ b/server/datastore/commit.go @@ -31,6 +31,10 @@ type Commitstore interface { // from the datastore accessible to the specified user. GetCommitListActivity(user *model.User) ([]*model.CommitRepo, error) + // GetCommitPrior retrieves the latest commit + // from the datastore for the specified repository and branch. + GetCommitPrior(commit *model.Commit) (*model.Commit, error) + // PostCommit saves a commit in the datastore. PostCommit(commit *model.Commit) error @@ -82,6 +86,12 @@ func GetCommitListActivity(c context.Context, user *model.User) ([]*model.Commit return FromContext(c).GetCommitListActivity(user) } +// GetCommitPrior retrieves the latest commit +// from the datastore for the specified repository and branch. +func GetCommitPrior(c context.Context, commit *model.Commit) (*model.Commit, error) { + return FromContext(c).GetCommitPrior(commit) +} + // PostCommit saves a commit in the datastore. func PostCommit(c context.Context, commit *model.Commit) error { return FromContext(c).PostCommit(commit) diff --git a/server/datastore/database/commit.go b/server/datastore/database/commit.go index 72f2ecc5a..fdb6369a2 100644 --- a/server/datastore/database/commit.go +++ b/server/datastore/database/commit.go @@ -40,15 +40,6 @@ func (db *Commitstore) GetCommitLast(repo *model.Repo, branch string) (*model.Co return commit, err } -// GetCommitPrior retrieves the latest commit -// from the datastore for the specified repository -// and branch. -func (db *Commitstore) GetCommitPrior(oldCommit *model.Commit) (*model.Commit, error) { - var commit = new(model.Commit) - var err = meddler.QueryRow(db, commit, rebind(commitPriorQuery), oldCommit.RepoID, oldCommit.Branch, oldCommit.Created) - return commit, err -} - // GetCommitList retrieves a list of latest commits // from the datastore for the specified repository. func (db *Commitstore) GetCommitList(repo *model.Repo) ([]*model.Commit, error) { @@ -73,24 +64,30 @@ func (db *Commitstore) GetCommitListActivity(user *model.User) ([]*model.CommitR return commits, err } +// GetCommitPrior retrieves the latest commit +// from the datastore for the specified repository and branch. +func (db *Commitstore) GetCommitPrior(oldCommit *model.Commit) (*model.Commit, error) { + var commit = new(model.Commit) + var err = meddler.QueryRow(db, commit, rebind(commitPriorQuery), oldCommit.RepoID, oldCommit.Branch, oldCommit.ID) + return commit, err +} + // PostCommit saves a commit in the datastore. func (db *Commitstore) PostCommit(commit *model.Commit) error { if commit.Created == 0 { commit.Created = time.Now().UTC().Unix() } commit.Updated = time.Now().UTC().Unix() - - priorCommit, err := db.GetCommitPrior(commit) - if err == nil { - commit.PriorStatus = priorCommit.Status - } - return meddler.Save(db, commitTable, commit) } // PutCommit saves a commit in the datastore. func (db *Commitstore) PutCommit(commit *model.Commit) error { - return db.PostCommit(commit) + if commit.Created == 0 { + commit.Created = time.Now().UTC().Unix() + } + commit.Updated = time.Now().UTC().Unix() + return meddler.Save(db, commitTable, commit) } // DelCommit removes the commit from the datastore. @@ -185,11 +182,11 @@ LIMIT 1 const commitPriorQuery = ` SELECT * FROM commits -WHERE repo_id = ? +WHERE repo_id = ? AND commit_branch = ? - AND commit_created < ? + AND commit_id < ? AND commit_status IN ('Success', 'Failure') -ORDER BY commit_created DESC +ORDER BY commit_id DESC LIMIT 1 ` diff --git a/server/datastore/database/commit_test.go b/server/datastore/database/commit_test.go index 974d40133..d629ee250 100644 --- a/server/datastore/database/commit_test.go +++ b/server/datastore/database/commit_test.go @@ -48,84 +48,6 @@ func TestCommitstore(t *testing.T) { g.Assert(commit.ID != 0).IsTrue() }) - g.It("Should Get a Commit's prior status", func() { - commit := model.Commit{ - RepoID: 1, - Branch: "foo", - Status: "Failure", - Created: 1419892236, - Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", - } - err := cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "").IsTrue() - - commit = model.Commit{ - RepoID: 1, - Branch: "foo", - Status: "Success", - Created: 1419893583, - Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", - } - err = cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "Failure").IsTrue() - }) - - g.It("Should not find prior status from commits on other branches", func() { - commit := model.Commit{ - RepoID: 1, - Branch: "foo", - Status: "Failure", - Created: 1419892236, - Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", - } - err := cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "").IsTrue() - - commit = model.Commit{ - RepoID: 1, - Branch: "bar", - Status: "Success", - Created: 1419893583, - Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", - } - err = cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "").IsTrue() - }) - - g.It("Should not find prior status from commits that didn't succeed or fail", func() { - commit := model.Commit{ - RepoID: 1, - Branch: "foo", - Status: "Pending", - Created: 1419892236, - Sha: "85f8c029b902ed9400bc600bac301a0aadb144ac", - } - err := cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "").IsTrue() - - commit = model.Commit{ - RepoID: 1, - Branch: "bar", - Status: "Success", - Created: 1419893583, - Sha: "25f8c029b902ed9400bc600bac301a0aadb144ac", - } - err = cs.PostCommit(&commit) - g.Assert(err == nil).IsTrue() - g.Assert(commit.ID != 0).IsTrue() - g.Assert(commit.PriorStatus == "").IsTrue() - }) - g.It("Should Get a Commit by ID", func() { commit := model.Commit{ RepoID: 1, diff --git a/server/datastore/database/database.go b/server/datastore/database/database.go index aecb32e9c..6ca59d66f 100644 --- a/server/datastore/database/database.go +++ b/server/datastore/database/database.go @@ -38,7 +38,6 @@ func Connect(driver, datasource string) (*sql.DB, error) { var migrations = []migration.Migrator{ migrate.Setup, migrate.Migrate_20142110, - migrate.Migrate_20143012, } return migration.Open(driver, datasource, migrations) } diff --git a/server/datastore/migrate/migrate.go b/server/datastore/migrate/migrate.go index 5985cba10..42a3c57c5 100644 --- a/server/datastore/migrate/migrate.go +++ b/server/datastore/migrate/migrate.go @@ -39,12 +39,6 @@ func Migrate_20142110(tx migration.LimitedTx) error { return nil } -// Migrate_20143012 adds the prior commit's status to the current commit -func Migrate_20143012(tx migration.LimitedTx) error { - _, err := tx.Exec(transform(commitPriorStatusColumn)) - return err -} - var userTable = ` CREATE TABLE IF NOT EXISTS users ( user_id INTEGER PRIMARY KEY AUTOINCREMENT @@ -116,10 +110,6 @@ var repoTokenUpdate = ` UPDATE repos SET repo_token = ''; ` -var commitPriorStatusColumn = ` -ALTER TABLE commits ADD COLUMN commit_prior_status VARCHAR(255); -` - var commitTable = ` CREATE TABLE IF NOT EXISTS commits ( commit_id INTEGER PRIMARY KEY AUTOINCREMENT diff --git a/server/worker/docker/docker.go b/server/worker/docker/docker.go index bff55e2a6..8bbd622ff 100644 --- a/server/worker/docker/docker.go +++ b/server/worker/docker/docker.go @@ -120,6 +120,8 @@ func (d *Docker) Do(c context.Context, r *worker.Work) { Depth: git.GitDepth(script.Git), } + priorCommit, _ := datastore.GetCommitPrior(c, r.Commit) + // send all "started" notifications if script.Notifications == nil { script.Notifications = ¬ify.Notification{} @@ -129,6 +131,7 @@ func (d *Docker) Do(c context.Context, r *worker.Work) { Repo: r.Repo, Commit: r.Commit, Host: r.Host, + Prior: priorCommit, }) // create an instance of the Docker builder @@ -168,11 +171,14 @@ func (d *Docker) Do(c context.Context, r *worker.Work) { // notify all listeners that the build is finished commitc.Publish(r) + priorCommit, _ = datastore.GetCommitPrior(c, r.Commit) + // send all "finished" notifications script.Notifications.Send(&model.Request{ User: r.User, Repo: r.Repo, Commit: r.Commit, Host: r.Host, + Prior: priorCommit, }) } diff --git a/shared/model/commit.go b/shared/model/commit.go index 0bc3aa57e..da8c0c656 100644 --- a/shared/model/commit.go +++ b/shared/model/commit.go @@ -8,7 +8,6 @@ type Commit struct { ID int64 `meddler:"commit_id,pk" json:"id"` RepoID int64 `meddler:"repo_id" json:"-"` Status string `meddler:"commit_status" json:"status"` - PriorStatus string `meddler:"commit_prior_status" json:"prior_status"` Started int64 `meddler:"commit_started" json:"started_at"` Finished int64 `meddler:"commit_finished" json:"finished_at"` Duration int64 `meddler:"commit_duration" json:"duration"` diff --git a/shared/model/request.go b/shared/model/request.go index 0bf752513..594cbeb2f 100644 --- a/shared/model/request.go +++ b/shared/model/request.go @@ -5,4 +5,5 @@ type Request struct { User *User `json:"-"` Repo *Repo `json:"repo"` Commit *Commit `json:"commit"` + Prior *Commit `json:"prior_commit"` } From 0106a5e21dce75fd8b10004c99be070342baf162 Mon Sep 17 00:00:00 2001 From: Michael Nutt Date: Tue, 30 Dec 2014 13:29:38 -0500 Subject: [PATCH 3/3] no longer need to limit Prior commit to those that have status Success or Failure --- server/datastore/database/commit.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/datastore/database/commit.go b/server/datastore/database/commit.go index fdb6369a2..a34c2662d 100644 --- a/server/datastore/database/commit.go +++ b/server/datastore/database/commit.go @@ -185,7 +185,6 @@ FROM commits WHERE repo_id = ? AND commit_branch = ? AND commit_id < ? - AND commit_status IN ('Success', 'Failure') ORDER BY commit_id DESC LIMIT 1 `