[milo] Add BuildSummary and common models.

[milo] Add BuildSummary and common models.

R=estaab@chromium.org, hinoka@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2944633003
Committed: https://github.com/luci/luci-go/commit/6f16d5292547af0cdc836d17da8990f5993ab3be

[iannucci, 2017-06-16 23:12:24]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 23:12:26]

Dry run: 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-06-16 23:16:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 23:16:38]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36cbff8cdc1c5410)

[iannucci, 2017-06-16 23:45:47]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 23:45:57]

Dry run: 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-06-16 23:47:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 23:47:47]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36cc1e3cfae98e10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36cc1e3d18c29710)
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36cc1e3d5b1e4d10)

[iannucci, 2017-06-16 23:51:55]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 23:51:57]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-06-16 23:56:39]

PTAL

[commit-bot: I haz the power, 2017-06-16 23:58:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 23:58:07]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-06-17 00:44:27]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2017-06-17 00:44:29]

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
File milo/appengine/common/model/build_summary.go (right):

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:13: // BuildSummary is a datastore
model which is used for displaying
s/displaying/storing summarized data for/

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:22: // _id for a BuildSummary is
always 1
Why? What is this for?

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:35: //  
sha256(sorted(ConsoleEpochs.keys())
Is this a binary representation (instead of a hex representation?) If so then
note it here.

I think this being a string and then using the hex representation would be
easier to wrangle with.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:39: //   (console_name[str],
recorded_epoch[int])
// When a new console definition appears, this determines if the build needs to
be updated.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:44: //   (console_name[str],
sort_criteria[tuple], sort_values[tuple])
Explain sort_criteria[tuple] and sort_values[tuple]

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
File milo/appengine/common/model/summary.go (right):

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/summary.go:11: // Status indicates the 'goodness' of
the thing. This usually translates
On top of "goodness" It also describes where in the lifetime it is (pending,
running, etc)

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/summary.go:16: Start time.Time
Make this a pointer, nil to indicate still pending / not yet started

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/summary.go:19: End time.Time
Make this a pointer, to indicate no end time / still running.

https://codereview.chromium.org/2944633003/diff/40001/milo/appengine/common/m...
File milo/appengine/common/model/manifest_link.go (right):

https://codereview.chromium.org/2944633003/diff/40001/milo/appengine/common/m...
milo/appengine/common/model/manifest_link.go:42: cmpbin.WriteString(buf, m.Name)
These functions return errors, should probably propagate them

[Ryan Tseng, 2017-06-17 00:45:18]

lgtm % comments

[iannucci, 2017-06-17 01:29:59]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-06-17 01:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/2944633003/#ps60001
(title: "add comments")

[iannucci, 2017-06-17 01:30:05]

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
File milo/appengine/common/model/build_summary.go (right):

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:13: // BuildSummary is a datastore
model which is used for displaying
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> s/displaying/storing summarized data for/

Done.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:22: // _id for a BuildSummary is
always 1
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> Why? What is this for?

This is part of the key that luci/gae will generate. Basically they key will
always be `*BuildKey/"BuildSummary",1` now.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:35: //  
sha256(sorted(ConsoleEpochs.keys())
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> Is this a binary representation (instead of a hex representation?) If so then
> note it here.
> 
> I think this being a string and then using the hex representation would be
> easier to wrangle with.

I tend to prefer the raw representation (since it's 50% smaller). It's meant to
be opaque anyway :)

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:39: //   (console_name[str],
recorded_epoch[int])
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> // When a new console definition appears, this determines if the build needs
to
> be updated.

Done.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/build_summary.go:44: //   (console_name[str],
sort_criteria[tuple], sort_values[tuple])
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> Explain sort_criteria[tuple] and sort_values[tuple]

Done.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
File milo/appengine/common/model/summary.go (right):

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/summary.go:11: // Status indicates the 'goodness' of
the thing. This usually translates
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> On top of "goodness" It also describes where in the lifetime it is (pending,
> running, etc)
> 

Done.

https://codereview.chromium.org/2944633003/diff/1/milo/appengine/common/model...
milo/appengine/common/model/summary.go:16: Start time.Time
On 2017/06/17 00:44:28, Ryan Tseng wrote:
> Make this a pointer, nil to indicate still pending / not yet started

Won't work in datastore. 0 value of time.Time is pretty unique though.

https://codereview.chromium.org/2944633003/diff/40001/milo/appengine/common/m...
File milo/appengine/common/model/manifest_link.go (right):

https://codereview.chromium.org/2944633003/diff/40001/milo/appengine/common/m...
milo/appengine/common/model/manifest_link.go:42: cmpbin.WriteString(buf, m.Name)
On 2017/06/17 00:44:29, Ryan Tseng wrote:
> These functions return errors, should probably propagate them

They only return errors if the write returns an error. bytes.Buffer writes can't
return errors :)

[commit-bot: I haz the power, 2017-06-17 01:30:15]

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-06-17 01:36:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497662999280860,
"parent_rev": "a986b4c74e7bb8de370d1a8aa6784a4c5ccbddd8", "commit_rev":
"6f16d5292547af0cdc836d17da8990f5993ab3be"}

[commit-bot: I haz the power, 2017-06-17 01:36:47]

Description was changed from

==========
[milo] Add BuildSummary and common models.

R=estaab@chromium.org, hinoka@chromium.org
BUG=
==========

to

==========
[milo] Add BuildSummary and common models.

R=estaab@chromium.org, hinoka@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2944633003
Committed:
https://github.com/luci/luci-go/commit/6f16d5292547af0cdc836d17da8990f5993ab3be
==========

[commit-bot: I haz the power, 2017-06-17 01:36:48]

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