Milo: Console view prototype

appengine/cmd/milo/buildbot/console.go

+// Copyright 2016 The LUCI Authors. All rights reserved.
+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+package buildbot
+
+import (
+        "fmt"
+        "strings"
+
+        "github.com/luci/gae/service/datastore"
+        "github.com/luci/luci-go/appengine/cmd/milo/resp"
+        "github.com/luci/luci-go/common/clock"
+        log "github.com/luci/luci-go/common/logging"
+        "github.com/luci/luci-go/common/sync/parallel"
+        "golang.org/x/net/context"
+)
+
+// getFullBuilds fetches all of the recent builds from the datastore.
+func getFullBuilds(c context.Context, masterName, builderName string, finished bool) ([]*buildbotBuild, error) {
+        // TODO(hinoka): Builder specific structs.
+        ds := datastore.Get(c)
+        q := datastore.NewQuery("buildbotBuild")
+        q = q.Eq("finished", finished)
+        q = q.Eq("master", masterName)
+        q = q.Eq("builder", builderName)
+        q = q.Limit(25) // TODO(hinoka): This should be adjustable
+        q = q.Order("-number")
+        q.Finalize()
+        buildbots := make([]*buildbotBuild, 0, 25)
+        err := ds.GetAll(q, &buildbots)
+        return buildbots, err
+}
+
+func GetConsoleBuilds(
+        c context.Context, builders []resp.BuilderRef, commits []string) (
+        [][]*resp.ConsoleBuild, error) {

[nodir, 2016/08/04 23:13:12]

document what indexes in the return value mean

[hinoka, 2016/08/05 00:10:41]

Done.

+
+        results := make([][]*resp.ConsoleBuild, len(commits))
+        for i := range results {
+                results[i] = make([]*resp.ConsoleBuild, len(builders))
+        }
+        // HACK(hinoka): This fetches 25 full builds and then filters them. Replace this
+        // with something more reasonable.
+        // This is kind of a hack but it's okay for now.
+        err := parallel.FanOutIn(func(taskC chan<- func() error) {
+                for i, builder := range builders {
+                        i := i // How do scopes werk
+                        f := strings.SplitN(builder.Name, "/", 2)

[nodir, 2016/08/04 23:13:13]

i am not sure what f stands for

[hinoka, 2016/08/05 00:10:42]

Done.

+                        if len(f) != 2 {
+                                taskC <- func() error {
+                                        return fmt.Errorf("%s is an invalid builder name", builder.Name)

[nodir, 2016/08/04 23:13:12]

builder var is captured, so there is a race
you need to do `builder := builder` in the beginning of the loop

[hinoka, 2016/08/05 00:10:41]

Oh good catch

+                                }
+                                return
+                        }
+                        master := f[0]
+                        builderName := f[1]
+                        taskC <- func() error {
+                                t1 := clock.Now(c)
+                                builds, err := getFullBuilds(c, master, builderName, true)
+                                if err != nil {
+                                        return err
+                                }
+                                t2 := clock.Now(c)
+                                var currentStatus *resp.Status
+                                for j, commit := range commits {
+                                        for _, build := range builds {
+                                                if build.Sourcestamp.Revision == commit {

[nodir, 2016/08/04 23:13:12]

why does this have only one source stamp? a build may span multiple commits if
buildbot build requests for different commits got merged into one build

[hinoka, 2016/08/05 00:10:41]

This is buildbot's own concept of sourcestamp, that is, the revision under
testing.  This is different than the concept of build request merging.

+                                                        results[j][i] = &resp.ConsoleBuild{

[nodir, 2016/08/04 23:13:12]

since there is no "break" in the end of this `if`, results[i][j] can be
overwritten by another build in |builds| that has same revision

this also demonstrates a design problem: what if there are multiple builds for
the same commit? do we show an arbitrary one? (that's not good)

maybe the return value should be [][][]resp.ConsoleBuild

[hinoka, 2016/08/05 00:10:41]

I think we should pick the latest one (This code won't pick the latest one, but
that's okay for now).  But I agree we should discuss what should happen with
multiple builds, particularly since DM has native support for multiple attempts
on the same quest.

+                                                                Link: &resp.Link{
+                                                                        Label: strings.Join(build.Text, " "),
+                                                                        URL: fmt.Sprintf(
+                                                                                "/buildbot/%s/%s/%d", master, builderName, build.Number),
+                                                                },
+                                                                Status: build.toStatus(),
+                                                        }
+                                                        currentStatus = &results[j][i].Status
+                                                }
+                                        }
+                                        if currentStatus != nil && results[j][i] == nil {
+                                                results[j][i] = &resp.ConsoleBuild{Status: *currentStatus}
+                                        }
+                                }
+                                log.Debugf(c,
+                                        "Builder %s took %s to query, %s to compute.", builderName,
+                                        t2.Sub(t1), clock.Since(c, t2))
+                                return nil
+                        }
+                }
+        })
+        return results, err
+}