From 4362adf78b89ffb3ea099cff0ac1000b1e9e1521 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Wed, 22 Apr 2015 10:50:45 -0600 Subject: [PATCH] Fix error handling, remove unused bucketBuildTasks --- datastore/bolt/bolt.go | 2 -- datastore/bolt/repo.go | 19 ++++++---- datastore/bolt/repo_del_test.go | 63 --------------------------------- datastore/bolt/repo_test.go | 46 ++++++++++++++++++++---- datastore/bolt/util.go | 5 +-- 5 files changed, 55 insertions(+), 80 deletions(-) delete mode 100644 datastore/bolt/repo_del_test.go diff --git a/datastore/bolt/bolt.go b/datastore/bolt/bolt.go index 018ab30f5..cd0186e31 100644 --- a/datastore/bolt/bolt.go +++ b/datastore/bolt/bolt.go @@ -24,7 +24,6 @@ var ( bucketRepoUsers = []byte("repo_users") bucketBuild = []byte("build") bucketBuildStatus = []byte("build_status") - bucketBuildTasks = []byte("build_tasks") bucketBuildLogs = []byte("build_logs") bucketBuildSeq = []byte("build_seq") ) @@ -51,7 +50,6 @@ func New(path string) (*DB, error) { tx.CreateBucketIfNotExists(bucketRepoUsers) tx.CreateBucketIfNotExists(bucketBuild) tx.CreateBucketIfNotExists(bucketBuildStatus) - tx.CreateBucketIfNotExists(bucketBuildTasks) tx.CreateBucketIfNotExists(bucketBuildLogs) tx.CreateBucketIfNotExists(bucketBuildSeq) return nil diff --git a/datastore/bolt/repo.go b/datastore/bolt/repo.go index e1f70217f..9d2daa705 100644 --- a/datastore/bolt/repo.go +++ b/datastore/bolt/repo.go @@ -153,20 +153,25 @@ func (db *DB) DelRepo(repo *common.Repo) error { // deleteTracesOfRepo cleans up build leftovers when a repo is removed func (db *DB) deleteTracesOfRepo(t *bolt.Tx, repoKey []byte) error { - err := error(nil) - // bucketBuildSeq uses the repoKey directly - t.Bucket(bucketBuildSeq).Delete(repoKey) + err := t.Bucket(bucketBuildSeq).Delete(repoKey) + if err != nil { + // only error here is if our Tx is read-only + return err + } // the other buckets use repoKey with '/buildNumber', at least. // validating that an additiona '/' is there ensures that we don't // match 'github.com/drone/droney' when we're cleaning up after // 'github.com/drone/drone'. prefix := append(repoKey, '/') - deleteWithPrefix(t, bucketBuildLogs, prefix, true) - deleteWithPrefix(t, bucketBuildStatus, prefix, true) - deleteWithPrefix(t, bucketBuildTasks, prefix, true) - deleteWithPrefix(t, bucketBuild, prefix, true) + buckets := [][]byte{bucketBuildStatus, bucketBuildLogs, bucketBuild} + for _, b := range buckets { + err = deleteWithPrefix(t, b, prefix) + if err != nil { + break + } + } return err } diff --git a/datastore/bolt/repo_del_test.go b/datastore/bolt/repo_del_test.go deleted file mode 100644 index 22e101087..000000000 --- a/datastore/bolt/repo_del_test.go +++ /dev/null @@ -1,63 +0,0 @@ -package bolt - -import ( - "io/ioutil" - "os" - "testing" - - "github.com/drone/drone/common" - . "github.com/franela/goblin" -) - -func TestRepoDel(t *testing.T) { - g := Goblin(t) - g.Describe("Delete repo", func() { - - var db *DB // temporary database - - user := &common.User{Login: "freya"} - repoUri := string("github.com/octopod/hq") - - // create a new database before each unit - // test and destroy afterwards. - g.BeforeEach(func() { - file, err := ioutil.TempFile(os.TempDir(), "drone-bolt") - if err != nil { - panic(err) - } - - db = Must(file.Name()) - }) - g.AfterEach(func() { - os.Remove(db.Path()) - }) - - g.It("should cleanup", func() { - repo := &common.Repo{FullName: repoUri} - err := db.SetRepoNotExists(user, repo) - g.Assert(err).Equal(nil) - - db.SetBuild(repoUri, &common.Build{State: "success"}) - db.SetBuild(repoUri, &common.Build{State: "success"}) - db.SetBuild(repoUri, &common.Build{State: "pending"}) - - db.SetBuildStatus(repoUri, 1, &common.Status{Context: "success"}) - db.SetBuildStatus(repoUri, 2, &common.Status{Context: "success"}) - db.SetBuildStatus(repoUri, 3, &common.Status{Context: "pending"}) - - // first a little sanity to validate our test conditions - _, err = db.BuildLast(repoUri) - g.Assert(err).Equal(nil) - - // now run our specific test suite - // 1. ensure that we can delete the repo - err = db.DelRepo(repo) - g.Assert(err).Equal(nil) - - // 2. ensure that deleting the repo cleans up other references - _, err = db.Build(repoUri, 1) - g.Assert(err).Equal(ErrKeyNotFound) - }) - }) - -} diff --git a/datastore/bolt/repo_test.go b/datastore/bolt/repo_test.go index ab45babc7..397f1d4bf 100644 --- a/datastore/bolt/repo_test.go +++ b/datastore/bolt/repo_test.go @@ -1,10 +1,12 @@ package bolt import ( - "github.com/drone/drone/common" - . "github.com/franela/goblin" + "io/ioutil" "os" "testing" + + "github.com/drone/drone/common" + . "github.com/franela/goblin" ) func TestRepo(t *testing.T) { @@ -13,12 +15,17 @@ func TestRepo(t *testing.T) { testUser := "octocat" testRepo := "github.com/octopod/hq" testRepo2 := "github.com/octopod/avengers" + commUser := &common.User{Login: "freya"} var db *DB // Temp database - // create a new database before each unit - // test and destroy afterwards. + // create a new database before each unit test and destroy afterwards. g.BeforeEach(func() { - db = Must("/tmp/drone.test.db") + file, err := ioutil.TempFile(os.TempDir(), "drone-bolt") + if err != nil { + panic(err) + } + + db = Must(file.Name()) }) g.AfterEach(func() { os.Remove(db.Path()) @@ -41,7 +48,7 @@ func TestRepo(t *testing.T) { g.Assert(repo.FullName).Equal(testRepo) }) - g.It("Should del Repo", func() { + g.It("Should be deletable", func() { db.SetRepo(&common.Repo{FullName: testRepo}) db.Repo(testRepo) @@ -49,6 +56,33 @@ func TestRepo(t *testing.T) { g.Assert(err_).Equal(nil) }) + g.It("Should cleanup builds when deleted", func() { + repo := &common.Repo{FullName: testRepo} + err := db.SetRepoNotExists(commUser, repo) + g.Assert(err).Equal(nil) + + db.SetBuild(testRepo, &common.Build{State: "success"}) + db.SetBuild(testRepo, &common.Build{State: "success"}) + db.SetBuild(testRepo, &common.Build{State: "pending"}) + + db.SetBuildStatus(testRepo, 1, &common.Status{Context: "success"}) + db.SetBuildStatus(testRepo, 2, &common.Status{Context: "success"}) + db.SetBuildStatus(testRepo, 3, &common.Status{Context: "pending"}) + + // first a little sanity to validate our test conditions + _, err = db.BuildLast(testRepo) + g.Assert(err).Equal(nil) + + // now run our specific test suite + // 1. ensure that we can delete the repo + err = db.DelRepo(repo) + g.Assert(err).Equal(nil) + + // 2. ensure that deleting the repo cleans up other references + _, err = db.Build(testRepo, 1) + g.Assert(err).Equal(ErrKeyNotFound) + }) + g.It("Should get RepoList", func() { db.SetRepoNotExists(&common.User{Login: testUser}, &common.Repo{FullName: testRepo}) db.SetRepoNotExists(&common.User{Login: testUser}, &common.Repo{FullName: testRepo2}) diff --git a/datastore/bolt/util.go b/datastore/bolt/util.go index 1d992f3f5..7affb98e5 100644 --- a/datastore/bolt/util.go +++ b/datastore/bolt/util.go @@ -85,16 +85,17 @@ func splice(t *bolt.Tx, bucket, index, value []byte) error { return update(t, bucket, index, &keys) } -func deleteWithPrefix(t *bolt.Tx, bucket, prefix []byte, ignoreErr bool) error { +func deleteWithPrefix(t *bolt.Tx, bucket, prefix []byte) error { var err error c := t.Bucket(bucket).Cursor() for k, _ := c.Seek(prefix); bytes.HasPrefix(k, prefix); k, _ = c.Next() { err = c.Delete() - if !ignoreErr && err != nil { + if err != nil { break } } + // only error here is if our Tx is read-only return err }