From ba159976a04aeade595b47a169662f95df745d55 Mon Sep 17 00:00:00 2001 From: Daniel Oliveira Date: Thu, 7 May 2015 23:31:25 -0600 Subject: [PATCH] Fixing TODO comments in code /cc @oliveiradan 1. server/login.go:49 (// TODO(bradrydzewski) return an error message instead). Added error message if authorization fails. 2. server/repos.go:178 (TODO(bradrydzewski) verify repo not exists). Added a checking for the repo and return an error in case it does not exist. 3. server/queue.go:170: // TODO (bradrydzewski) change this interface to accept an io.Reader. All references to the API change been in question SetLogs() have been modified. 4. remote/github/github.go:106 // Fixed a crash in case *repo_.Language is nil , when de-referencing it. This could happen when a repo only has a readme, so github hasn't set the language yet. 5. ./server/queue.go:170: // TODO (bradrydzewski) change this interface to accept an io.Reader. All references to the API change been in question SetLogs() have been modified. 6. .remote/github/github.go:106 // Fixed a crash in case *repo_.Language is nil , when de-referencing it. This could happen when a repo only has a readme, so github hasn't set the language yet. --- datastore/builtin/repo_test.go | 3 ++- datastore/builtin/task.go | 12 ++++++++++-- datastore/builtin/task_test.go | 5 +++-- datastore/datastore.go | 3 ++- datastore/mock/mock.go | 3 ++- remote/github/github.go | 5 ++++- runner/builtin/updater.go | 15 ++++++++------- server/login.go | 2 +- server/queue.go | 13 ++----------- server/repos.go | 6 +++++- 10 files changed, 39 insertions(+), 28 deletions(-) diff --git a/datastore/builtin/repo_test.go b/datastore/builtin/repo_test.go index 9b72d2b44..998aadb0d 100644 --- a/datastore/builtin/repo_test.go +++ b/datastore/builtin/repo_test.go @@ -1,6 +1,7 @@ package builtin import ( + "bytes" "github.com/drone/drone/common" . "github.com/franela/goblin" "io/ioutil" @@ -63,7 +64,7 @@ func TestRepo(t *testing.T) { db.SetBuild(testRepo, &common.Build{State: "success"}) db.SetBuild(testRepo, &common.Build{State: "success"}) db.SetBuild(testRepo, &common.Build{State: "pending"}) - db.SetLogs(testRepo, 1, 1, []byte("foo")) + db.SetLogs(testRepo, 1, 1, (bytes.NewBuffer([]byte("foo")))) // first a little sanity to validate our test conditions _, err = db.BuildLast(testRepo) diff --git a/datastore/builtin/task.go b/datastore/builtin/task.go index 366104043..79bdd4209 100644 --- a/datastore/builtin/task.go +++ b/datastore/builtin/task.go @@ -2,19 +2,27 @@ package builtin import ( "bytes" - "github.com/boltdb/bolt" "io" + "io/ioutil" "strconv" + + "github.com/boltdb/bolt" ) // SetLogs inserts or updates a task logs for the // named repository and build number. -func (db *DB) SetLogs(repo string, build int, task int, log []byte) error { +func (db *DB) SetLogs(repo string, build int, task int, rd io.Reader) error { key := []byte(repo + "/" + strconv.Itoa(build) + "/" + strconv.Itoa(task)) t, err := db.Begin(true) if err != nil { return err } + + log, err := ioutil.ReadAll(rd) + if err != nil { + return err + } + err = t.Bucket(bucketBuildLogs).Put(key, log) if err != nil { t.Rollback() diff --git a/datastore/builtin/task_test.go b/datastore/builtin/task_test.go index 92cb14c2e..4d4a144d9 100644 --- a/datastore/builtin/task_test.go +++ b/datastore/builtin/task_test.go @@ -1,6 +1,7 @@ package builtin import ( + "bytes" "github.com/drone/drone/common" . "github.com/franela/goblin" "io/ioutil" @@ -30,13 +31,13 @@ func TestTask(t *testing.T) { g.It("Should set Logs", func() { db.SetRepo(&common.Repo{FullName: testRepo}) - err := db.SetLogs(testRepo, testBuild, testTask, testLogInfo) + err := db.SetLogs(testRepo, testBuild, testTask, (bytes.NewBuffer(testLogInfo))) g.Assert(err).Equal(nil) }) g.It("Should get logs", func() { db.SetRepo(&common.Repo{FullName: testRepo}) - db.SetLogs(testRepo, testBuild, testTask, testLogInfo) + db.SetLogs(testRepo, testBuild, testTask, (bytes.NewBuffer(testLogInfo))) buf, err := db.LogReader(testRepo, testBuild, testTask) g.Assert(err).Equal(nil) logInfo, err := ioutil.ReadAll(buf) diff --git a/datastore/datastore.go b/datastore/datastore.go index 0fd1b2a83..3cd8469ce 100644 --- a/datastore/datastore.go +++ b/datastore/datastore.go @@ -139,5 +139,6 @@ type Datastore interface { // SetLogs inserts or updates a task logs for the // named repository and build number. - SetLogs(string, int, int, []byte) error + SetLogs(string, int, int, io.Reader) error } + diff --git a/datastore/mock/mock.go b/datastore/mock/mock.go index 36e74c9b2..b999fefc4 100644 --- a/datastore/mock/mock.go +++ b/datastore/mock/mock.go @@ -291,7 +291,8 @@ func (m *Datastore) LogReader(_a0 string, _a1 int, _a2 int) (io.Reader, error) { return r0, r1 } -func (m *Datastore) SetLogs(_a0 string, _a1 int, _a2 int, _a3 []byte) error { + +func (m *Datastore) SetLogs(_a0 string, _a1 int, _a2 int, _a3 io.Reader) error { ret := m.Called(_a0, _a1, _a2, _a3) r0 := ret.Error(0) diff --git a/remote/github/github.go b/remote/github/github.go index 800d4a63c..181477099 100644 --- a/remote/github/github.go +++ b/remote/github/github.go @@ -103,7 +103,10 @@ func (g *GitHub) Repo(u *common.User, owner, name string) (*common.Repo, error) repo := &common.Repo{} repo.Owner = owner repo.Name = name - repo.Language = *repo_.Language + + if repo_.Language != nil { + repo.Language = *repo_.Language + } repo.FullName = *repo_.FullName repo.Link = *repo_.HTMLURL repo.Private = *repo_.Private diff --git a/runner/builtin/updater.go b/runner/builtin/updater.go index b87b0cf7a..5eae27ce9 100644 --- a/runner/builtin/updater.go +++ b/runner/builtin/updater.go @@ -3,7 +3,7 @@ package builtin import ( "encoding/json" "io" - "io/ioutil" + //"io/ioutil" "github.com/drone/drone/common" "github.com/drone/drone/datastore" @@ -77,10 +77,11 @@ func (u *updater) SetTask(r *common.Repo, b *common.Build, t *common.Task) error } func (u *updater) SetLogs(r *common.Repo, b *common.Build, t *common.Task, rc io.ReadCloser) error { - defer rc.Close() - out, err := ioutil.ReadAll(rc) - if err != nil { - return err - } - return u.store.SetLogs(r.FullName, b.Number, t.Number, out) + //defer rc.Close() + //out, err := ioutil.ReadAll(rc) + //if err != nil { + // return err + //} + //return u.store.SetLogs(r.FullName, b.Number, t.Number, out) + return u.store.SetLogs(r.FullName, b.Number, t.Number, rc) } diff --git a/server/login.go b/server/login.go index af4e1c283..6a9c8a41b 100644 --- a/server/login.go +++ b/server/login.go @@ -45,8 +45,8 @@ func GetLogin(c *gin.Context) { } // exit if authorization fails - // TODO(bradrydzewski) return an error message instead if c.Writer.Status() != 200 { + log.Errorf("authorization failed.") return } diff --git a/server/queue.go b/server/queue.go index 284d3245f..fb0f4aab5 100644 --- a/server/queue.go +++ b/server/queue.go @@ -3,7 +3,6 @@ package server import ( "encoding/json" "io" - "io/ioutil" "net" "strconv" @@ -167,16 +166,8 @@ func PushLogs(c *gin.Context) { bnum, _ := strconv.Atoi(c.Params.ByName("build")) tnum, _ := strconv.Atoi(c.Params.ByName("task")) - // TODO (bradrydzewski) change this interface to accept an io.Reader - // instead of a byte array so that we can buffer the write and so that - // we avoid unnecessary copies of the data in memory. - logs, err := ioutil.ReadAll(io.LimitReader(c.Request.Body, 5000000)) //5MB - defer c.Request.Body.Close() - if err != nil { - c.Fail(500, err) - return - } - err = store.SetLogs(repo.FullName, bnum, tnum, logs) + const maxBuffToRead int64 = 5000000 // 5MB. + err := store.SetLogs(repo.FullName, bnum, tnum, io.LimitReader(c.Request.Body, maxBuffToRead)) if err != nil { c.Fail(500, err) return diff --git a/server/repos.go b/server/repos.go index 95e76b119..7c89e09da 100644 --- a/server/repos.go +++ b/server/repos.go @@ -171,7 +171,11 @@ func PostRepo(c *gin.Context) { owner := c.Params.ByName("owner") name := c.Params.ByName("name") - // TODO(bradrydzewski) verify repo not exists + _, err := store.Repo(owner + "/" + name) + if err == nil { + c.String(409, "Repository already exists") + return + } // get the repository and user permissions // from the remote system.