[milo] Refactor all html knowledge out of backends.

milo/buildsource/builder.go

Index: milo/buildsource/builder.go
diff --git a/milo/buildsource/builder.go b/milo/buildsource/builder.go
new file mode 100644
index 0000000000000000000000000000000000000000..982b6253720ebef9d613ca2572c57ca2a4d656b9
--- /dev/null
+++ b/milo/buildsource/builder.go
@@ -0,0 +1,94 @@
+// Copyright 2017 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 buildsource
+
+import (
+	"strconv"
+	"strings"
+
+	"golang.org/x/net/context"
+
+	"github.com/luci/gae/service/datastore"
+
+	"github.com/luci/luci-go/common/errors"
+	"github.com/luci/luci-go/common/logging"
+	"github.com/luci/luci-go/milo/api/resp"
+	"github.com/luci/luci-go/milo/buildsource/buildbot"
+	"github.com/luci/luci-go/milo/buildsource/buildbucket"
+	"github.com/luci/luci-go/milo/common"
+)
+
+// BuilderID is the universal ID of a builder, and has the form:
+//   buildbucket/bucket/builder
+//   buildbot/master/builder
+type BuilderID string
+
+func parseTwo(path, nameType, nameA, nameB string) (a, b string, err error) {

[Ryan Tseng, 2017/07/13 22:00:46]

docstring - what is path? what is nameType? nameA? nameB? what does this return?
What is it for?

Actually this looks like a glorified SplitN(..., 2).  I think it's better to get
rid of this and do everything inline in Get(), or create another internal method
func (b BuilderID) Parse() (moduleName, builderGroupName, builderName string)
{...} would be a better idea.  Becaue:
1. The existence of parseTwo is confusing.  The function signature doesn't lead
it to imply that it is good at any particular job.
2. The error message are confusing.  This would say "bad BuilderID: cannot parse
source token path" when in reality what we want to say is "bad BuilderID: path
has less than 3 components", or "badBuilderID: Missing builder name"

I think this would be much simpler as a:
func (b builderID) parse() (moduleName, builderGroupName, builderName string)
{...}

your choice though

[iannucci, 2017/07/14 19:00:22]

Refactored

+	toks := strings.SplitN(path, "/", 2)
+	if len(toks) != 2 {
+		err = errors.Reason("bad BuilderID: cannot parse %s token %q", nameType, path).
+			Tag(common.CodeParameterError).Err()
+		return
+	}
+	a, b = toks[0], toks[1]
+	if a == "" {
+		err = errors.Reason("bad BuilderID: empty %s", nameA).Tag(common.CodeParameterError).Err()
+		return
+	}
+	if b == "" {
+		err = errors.Reason("bad BuilderID: empty %s", nameB).Tag(common.CodeParameterError).Err()
+		return
+	}
+	return
+}
+
+// Get allows you to obtain the resp.Builder that corresponds with this
+// BuilderID.
+func (b BuilderID) Get(ctx context.Context, limitStr string, cursorStr string) (*resp.Builder, error) {
+	// TODO(iannucci): replace these implementations with a BuildSummary query.
+
+	limit := 0
+	if limitStr != "" {
+		switch limitVal, err := strconv.ParseInt(limitStr, 10, 0); err {
+		case nil:
+			limit = int(limitVal)
+		default:
+			logging.WithError(err).Warningf(ctx, "ignoring bad limit %q", limitStr)
+		}
+	}
+	if limit <= 0 {
+		limit = 25
+	}
+
+	source, path, err := parseTwo(string(b), "BuilderID", "source", "path")
+	if err != nil {
+		return nil, err
+	}
+
+	switch source {
+	case "buildbot":
+		master, builder, err := parseTwo(path, "buildbot", "master", "builder")
+		if err != nil {
+			return nil, err
+		}
+
+		cursor, err := datastore.DecodeCursor(ctx, cursorStr)
+		if err != nil {
+			return nil, errors.Annotate(err, "bad cursor").Err()
+		}
+
+		return buildbot.GetBuilder(ctx, master, builder, limit, cursor)
+
+	case "buildbucket":
+		bucket, builder, err := parseTwo(path, "buildbucket", "bucket", "builder")
+		if err != nil {
+			return nil, err
+		}
+		return buildbucket.GetBuilder(ctx, bucket, builder, limit)
+	}
+
+	return nil, errors.Reason("bad BuilderID: unknown backend %q", source).
+		Tag(common.CodeParameterError).Err()
+}