code review 5431048: dashboard: builder-facing implementation and tests

dashboard: builder-facing implementation and tests

[adg, 2011-11-24 04:15:03]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg

[dsymonds, 2011-11-24 04:31:05]

Just looked at build.go so far.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:39: _, err := datastore.NewQuery("Commit").
instead of a query, why not put a field in Package for this?

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:50: func (p *Package) Exists(c
appengine.Context) os.Error {
this needs a comment. The name also feels wrong. I'd expect an Exists func to
return a bool, but this is more complex. If you ask whether something exists,
why is it an _error_ if it doesn't exist? Maybe this should return (bool,
os.Error).

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:89: func (c *Commit) Validate() os.Error {
Valid?

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:100: // It must be called from inside a
datastore transaction.
A lot of these new methods are exported, and depend on being run in very
specific contexts. Can they be unexported instead?

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:213: tx := func(c appengine.Context) os.Error
{
this seems big enough to pull out into its own standalone function.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:428: func logErr(w http.ResponseWriter, r
*http.Request, err os.Error) {
if this returned the error you passed in, you could save a lot of lines above.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:429: appengine.NewContext(r).Errorf("Error:
%v", err)
the "Error:" prefix seems redundant here.

[adg, 2011-11-24 04:57:31]

Thanks.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:39: _, err := datastore.NewQuery("Commit").
On 2011/11/24 04:31:05, dsymonds wrote:
> instead of a query, why not put a field in Package for this?

Done.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:50: func (p *Package) Exists(c
appengine.Context) os.Error {
It's now GetPackage. Makes a lot more sense that way.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:89: func (c *Commit) Validate() os.Error {
On 2011/11/24 04:31:05, dsymonds wrote:
> Valid?

Done.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:100: // It must be called from inside a
datastore transaction.
On 2011/11/24 04:31:05, dsymonds wrote:
> A lot of these new methods are exported, and depend on being run in very
> specific contexts. Can they be unexported instead?

They're all exported but this is the only one that has a special condition, that
being that it must run inside a transaction.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:213: tx := func(c appengine.Context) os.Error
{
On 2011/11/24 04:31:05, dsymonds wrote:
> this seems big enough to pull out into its own standalone function.

Done.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:428: func logErr(w http.ResponseWriter, r
*http.Request, err os.Error) {
On 2011/11/24 04:31:05, dsymonds wrote:
> if this returned the error you passed in, you could save a lot of lines above.

If you're referring to the recurring pattern:
  logErr(w, r, err)
  return
note that these are all in http.HandlerFuncs which don't have a return value.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:429: appengine.NewContext(r).Errorf("Error:
%v", err)
On 2011/11/24 04:31:05, dsymonds wrote:
> the "Error:" prefix seems redundant here.

I like it.

[dsymonds, 2011-11-24 06:20:28]

Here's the rest of the review.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:100: // It must be called from inside a
datastore transaction.
On 2011/11/24 04:57:31, adg wrote:
> On 2011/11/24 04:31:05, dsymonds wrote:
> > A lot of these new methods are exported, and depend on being run in very
> > specific contexts. Can they be unexported instead?
> 
> They're all exported but this is the only one that has a special condition,
that
> being that it must run inside a transaction.

Okay, but still: do they need to be exported? Several of these look like they
should really only be used within this package.

http://codereview.appspot.com/5431048/diff/3002/misc/dashboard/app/build/buil...
misc/dashboard/app/build/build.go:429: appengine.NewContext(r).Errorf("Error:
%v", err)
On 2011/11/24 04:57:31, adg wrote:
> On 2011/11/24 04:31:05, dsymonds wrote:
> > the "Error:" prefix seems redundant here.
> 
> I like it.

Sure, but those errors are only ever presented with an "ERROR" prefix
(dev_appserver), or a coloured "E" box. It seems redundant. Up to you.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.go
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
misc/dashboard/app/build/build.go:215: tc := *com // clone so we don't clobber
it
why clone at all?

var tc Commit
err := datastore.Get(c, com.Key(c), &tc)

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
misc/dashboard/app/build/build.go:230: // if this isn't the first Commit test
the parent commit exists
seems this should happen *before* the datastore.Put in line 227.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go
File misc/dashboard/app/build/test.go (right):

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:7: // TODO(adg): subrepo tests
also, auth checking.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:63: if !appengine.IsDevAppServer() {
duplicate check with nukeEntities

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:63: if !appengine.IsDevAppServer() {
duplicate check with nukeEntities.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:82: fail := func(f string, args ...interface{})
{
call it errorf and govet can check this file.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:83: f = "%d %s: " + f + "\n"
why a separate var? This seems trickier than it needs to be.
Also, consider renaming f to format.

fmt.Fprintf(w, "%d %s: ", i, t.path)
fmt.Fprintf(w, format, args...)

[adg, 2011-11-24 23:30:27]

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.go
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
misc/dashboard/app/build/build.go:215: tc := *com // clone so we don't clobber
it
On 2011/11/24 06:20:28, dsymonds wrote:
> why clone at all?
> 
> var tc Commit
> err := datastore.Get(c, com.Key(c), &tc)

Done.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
misc/dashboard/app/build/build.go:230: // if this isn't the first Commit test
the parent commit exists
On 2011/11/24 06:20:28, dsymonds wrote:
> seems this should happen *before* the datastore.Put in line 227.

Does it matter? If it fails then the whole transaction is rolled back.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go
File misc/dashboard/app/build/test.go (right):

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:7: // TODO(adg): subrepo tests
On 2011/11/24 06:20:28, dsymonds wrote:
> also, auth checking.

Done.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:63: if !appengine.IsDevAppServer() {
On 2011/11/24 06:20:28, dsymonds wrote:
> duplicate check with nukeEntities

I want to make sure that there's no way that either testHandler or nukeEntities
are ever run on production data. It's paranoid and technically unnecessary but
it makes me feel better knowing it's there.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:82: fail := func(f string, args ...interface{})
{
On 2011/11/24 06:20:28, dsymonds wrote:
> call it errorf and govet can check this file.

Done.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:82: fail := func(f string, args ...interface{})
{
On 2011/11/24 06:20:28, dsymonds wrote:
> call it errorf and govet can check this file.

Done.

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/test.go...
misc/dashboard/app/build/test.go:83: f = "%d %s: " + f + "\n"
On 2011/11/24 06:20:28, dsymonds wrote:
> why a separate var? This seems trickier than it needs to be.
> Also, consider renaming f to format.
> 
> fmt.Fprintf(w, "%d %s: ", i, t.path)
> fmt.Fprintf(w, format, args...)

Done.

[dsymonds, 2011-11-25 01:09:24]

LGTM

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.go
File misc/dashboard/app/build/build.go (right):

http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
misc/dashboard/app/build/build.go:230: // if this isn't the first Commit test
the parent commit exists
On 2011/11/24 23:30:29, adg wrote:
> On 2011/11/24 06:20:28, dsymonds wrote:
> > seems this should happen *before* the datastore.Put in line 227.
> 
> Does it matter? If it fails then the whole transaction is rolled back.

It doesn't matter for correctness, but it'll save an RPC if there's something
weird going on. Up to you.

[adg, 2011-11-25 01:53:13]

*** Submitted as http://code.google.com/p/go/source/detail?r=41974a4eed1f ***

dashboard: builder-facing implementation and tests

R=golang-dev, dsymonds
CC=golang-dev
http://codereview.appspot.com/5431048

[adg, 2011-11-25 01:53:32]

On 25 November 2011 12:09,  <dsymonds@golang.org> wrote:
> LGTM
>
>
> http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.go
> File misc/dashboard/app/build/build.go (right):
>
>
http://codereview.appspot.com/5431048/diff/3/misc/dashboard/app/build/build.g...
> misc/dashboard/app/build/build.go:230: // if this isn't the first Commit
> test the parent commit exists
> On 2011/11/24 23:30:29, adg wrote:
>>
>> On 2011/11/24 06:20:28, dsymonds wrote:
>> > seems this should happen *before* the datastore.Put in line 227.
>
>> Does it matter? If it fails then the whole transaction is rolled back.
>
> It doesn't matter for correctness, but it'll save an RPC if there's
> something weird going on. Up to you.

Clarity of code is more important to me than saving an RPC in an
unusual failure state.