milo: Use service interface for swarming.

milo: Use service interface for swarming.

Milo now uses an internal "swarmingService" interface for swarming
operations. This allows the debug code to be moved into test files,
removing it from production. It also makes room for additional Swarming
service operations to be added in parallel.

BUG=chromium:687625
TEST=unit

Review-Url: https://codereview.chromium.org/2675493003
Committed: https://github.com/luci/luci-go/commit/f640dfb81c083686c36758c43176f983c9351d38

[dnj, 2017-02-02 02:57:14]

dnj@chromium.org changed reviewers:
+ hinoka@chromium.org

[dnj, 2017-02-02 02:57:14]

PTAL. This set of CLs adds a pRPC service to Milo called "BuildInfo". This
service offers a one-stop shop for users to query Milo about a specific build,
referencing it using familiar terms, and receive canonical build data (mostly
LogDog annotation protobufs) and metadata. It is envisioned as an entry point
for services that wish to learn about builds or view their logs.

CL chain:
https://codereview.chromium.org/2671633002 - (prep) Export host in LogDog
client.
https://codereview.chromium.org/2674603002 - (prep) Milo logdog module exports
AnnotationStream loading.
https://codereview.chromium.org/2675493003 - (prep) Refactor Swarming a bit, add
parallel fetcher.
https://codereview.chromium.org/2667353002 - Barebones (Unimplemented) BuildInfo
service addition.
https://codereview.chromium.org/2676503002 - (prep) Use strict for BuildBot
properties instead of []interface{}
https://codereview.chromium.org/2674513003 - BuildInfo implementation for
BuildBot builds.
https://codereview.chromium.org/2667363002 - BuildInfo implementation for
Swarming Kitchen builds.

[hinoka, 2017-02-02 22:36:51]

Is there a plan to add more swarming service implementations? If there is no
plan i'd rather not use interfaces, they make the code less readable/navigating
by breaking goto definitions.

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:32: var errNotMiloJob = errors.New("No a Milo
Job")
No -> Not

[dnj, 2017-02-02 22:44:47]

On 2017/02/02 22:36:51, hinoka wrote:
> Is there a plan to add more swarming service implementations? If there is no
> plan i'd rather not use interfaces, they make the code less
readable/navigating
> by breaking goto definitions.

Yes - a future CL adds a testing interface:
https://codereview.chromium.org/2667363002/diff/1/milo/appengine/swarming/bui...

Generically, though, I think it's better to state expectations (interface) and
then have implementations (prod/debug). Having two sets of functions that just
happen to be called together based on some string is obscure. It's much cleaner
to have the string choose the implementation, then swap it in. Even cleaner to
cut the string part out and just supply a prod implementation for prod and a
test one for test (which this CL does).

Additionally, ideally we'd move all debug code into _test.go files, but the
milo-level main tests muddy this a bit.

>
https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
> File milo/appengine/swarming/build.go (right):
> 
>
https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
> milo/appengine/swarming/build.go:32: var errNotMiloJob = errors.New("No a Milo
> Job")
> No -> Not

[dnj, 2017-02-03 00:13:06]

(ping)

[hinoka, 2017-02-03 01:02:24]

Generally lg but i'm super wary of indirection creep, as it makes the code
harder to work with going forward.

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:109: type swarmingFetch struct {
Can this just be a function? Not a struct?  There aren't enough input/outputs
for this to warrant a separate struct and I think this makes things more
complicated.

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:139: if err != nil {
If both fail the error is non-deterministic, I'd like to have
getSwarmingResult() errors take priority, like before.

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/htm...
File milo/appengine/swarming/html.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/htm...
milo/appengine/swarming/html.go:31: server = defaultSwarmingServer
This is only used once, lets not do this.  It's indirection creep.

[dnj, 2017-02-03 01:04:56]

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:109: type swarmingFetch struct {
On 2017/02/03 01:02:23, hinoka wrote:
> Can this just be a function? Not a struct?  There aren't enough input/outputs
> for this to warrant a separate struct and I think this makes things more
> complicated.

There will be (see the final CL in the chain).

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:139: if err != nil {
On 2017/02/03 01:02:23, hinoka wrote:
> If both fail the error is non-deterministic, I'd like to have
> getSwarmingResult() errors take priority, like before.

This is currently the case. Note that the log one returns "nil" to FanOutIn,
while the Swarming returns an error. The FanOutIn error will be nil if only log
fails.

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/htm...
File milo/appengine/swarming/html.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/htm...
milo/appengine/swarming/html.go:31: server = defaultSwarmingServer
On 2017/02/03 01:02:23, hinoka wrote:
> This is only used once, lets not do this.  It's indirection creep.

The final CL in the chain uses it, too.

[dnj, 2017-02-03 02:22:04]

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2675493003/diff/1/milo/appengine/swarming/bui...
milo/appengine/swarming/build.go:32: var errNotMiloJob = errors.New("No a Milo
Job")
On 2017/02/02 22:36:51, hinoka wrote:
> No -> Not

Done.

[hinoka, 2017-02-03 20:50:46]

https://codereview.chromium.org/2675493003/diff/40001/milo/appengine/swarming...
File milo/appengine/swarming/build.go (right):

https://codereview.chromium.org/2675493003/diff/40001/milo/appengine/swarming...
milo/appengine/swarming/build.go:109: type swarmingFetch struct {
How about splitting this into swarmingFetchRequest, swarmingFetchResponse, and
error?

[hinoka, 2017-02-03 20:54:09]

lgtm after the change

[hinoka, 2017-02-03 20:54:37]

Also don't make fetch a method, just a function is fine

[dnj, 2017-02-03 22:09:29]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-03 22:09:30]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org
Link to the patchset: https://codereview.chromium.org/2675493003/#ps60001
(title: "Update fetch signature")

[commit-bot: I haz the power, 2017-02-03 22:09:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 22:16:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486159769671700,
"parent_rev": "ce1e2318e3ca068031f804bdeeaea8efc928f658", "commit_rev":
"f640dfb81c083686c36758c43176f983c9351d38"}

[commit-bot: I haz the power, 2017-02-03 22:16:37]

Description was changed from

==========
milo: Use service interface for swarming.

Milo now uses an internal "swarmingService" interface for swarming
operations. This allows the debug code to be moved into test files,
removing it from production. It also makes room for additional Swarming
service operations to be added in parallel.

BUG=chromium:687625
TEST=unit
==========

to

==========
milo: Use service interface for swarming.

Milo now uses an internal "swarmingService" interface for swarming
operations. This allows the debug code to be moved into test files,
removing it from production. It also makes room for additional Swarming
service operations to be added in parallel.

BUG=chromium:687625
TEST=unit

Review-Url: https://codereview.chromium.org/2675493003
Committed:
https://github.com/luci/luci-go/commit/f640dfb81c083686c36758c43176f983c9351d38
==========

[commit-bot: I haz the power, 2017-02-03 22:16:38]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/f640dfb81c083686c36758c43176f983c9351d38