[milo] Add an (uncached) method to get console rows.

[milo] Add an (uncached) method to get console rows.

Also plugs in git history call.

R=hinoka@chromium.org
BUG=468053
Review-Url: https://codereview.chromium.org/2978293002
Committed: https://github.com/luci/luci-go/commit/52e6f7699476bd5518331dbe9600f211763be68a

[iannucci, 2017-07-19 02:14:30]

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

[commit-bot: I haz the power, 2017-07-19 02:14:35]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-07-19 02:14:55]

Description was changed from

==========
[milo] Add an (uncached) method to get console rows.

Also plugs in git history call.

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

to

==========
[milo] Add an (uncached) method to get console rows.

Also plugs in git history call.

R=hinoka@chromium.org
BUG=468053
==========

[commit-bot: I haz the power, 2017-07-19 02:21:25]

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

[commit-bot: I haz the power, 2017-07-19 02:21:26]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-07-19 08:07:57]

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

[Ryan Tseng, 2017-07-19 08:07:58]

From what I can tell, I think columns this doesn't make?

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go
File milo/buildsource/console.go (right):

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:22: ds "github.com/luci/gae/service/datastore"
nit: "datastore" is preferred in Milo

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:34: // `commits` slice.
"row-oriented" could be little ambiguous.  Should say something more precise
like:
Returns a matrix of BuildSummery objects.  The first index always matches the
commit, while the second index always matches the builder.  The returned object
will always be size of [len(commits)][len(builders)].

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:38: var err error
nit: this can be inlined

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:46: ret := make([][]*model.BuildSummary,
len(commits))
might as well pre-allocate the whole matrix here

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:63: ret[i] = append(ret[i], bs)
Pretty sure this is going to get mis-aligned.  The builds have to be inserted in
the correct slots (that matches the builder index).

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

https://codereview.chromium.org/2978293002/diff/1/milo/common/model/build_sum...
milo/common/model/build_summary.go:75: ManifestRevisionIndex [][]byte
maybe "ManifestKey" is good enough?

https://codereview.chromium.org/2978293002/diff/1/milo/common/model/build_sum...
milo/common/model/build_summary.go:89: // ManifestRevisionIndexPrefix is a
binary prefix for querying
Holy long names batman (not in the size of the name, but the number of concepts
a person needs to juggle in their head just from reading this variable name).

I'd say "ManifestRevisionIndexPrefix is an incomplete ManifestRevisionIndex
missing a git revision.  This can be converted into a full ManifestRevisionIndex
by supplying a git revision using AddRevision()"

If this was named "ManifestKey", then "ManifestKeyPrefix" would sound a lot more
natural.

https://codereview.chromium.org/2978293002/diff/1/milo/frontend/view_console.go
File milo/frontend/view_console.go (right):

https://codereview.chromium.org/2978293002/diff/1/milo/frontend/view_console....
milo/frontend/view_console.go:84: // HACK(hinoka): This only supports
buildbot....
no longer~

[iannucci, 2017-07-20 00:13:50]

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

[commit-bot: I haz the power, 2017-07-20 00:14:00]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-07-20 00:14:56]

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go
File milo/buildsource/console.go (right):

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:22: ds "github.com/luci/gae/service/datastore"
On 2017/07/19 08:07:57, Ryan Tseng wrote:
> nit: "datastore" is preferred in Milo

Done.

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:34: // `commits` slice.
On 2017/07/19 08:07:57, Ryan Tseng wrote:
> "row-oriented" could be little ambiguous.  Should say something more precise
> like:
> Returns a matrix of BuildSummery objects.  The first index always matches the
> commit, while the second index always matches the builder.  The returned
object
> will always be size of [len(commits)][len(builders)].

Done.

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:38: var err error
On 2017/07/19 08:07:57, Ryan Tseng wrote:
> nit: this can be inlined

can't because we're assigning to rawCommits[i]

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:46: ret := make([][]*model.BuildSummary,
len(commits))
On 2017/07/19 08:07:58, Ryan Tseng wrote:
> might as well pre-allocate the whole matrix here

Acknowledged.

https://codereview.chromium.org/2978293002/diff/1/milo/buildsource/console.go...
milo/buildsource/console.go:63: ret[i] = append(ret[i], bs)
On 2017/07/19 08:07:58, Ryan Tseng wrote:
> Pretty sure this is going to get mis-aligned.  The builds have to be inserted
in
> the correct slots (that matches the builder index).

Acknowledged.

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

https://codereview.chromium.org/2978293002/diff/1/milo/common/model/build_sum...
milo/common/model/build_summary.go:75: ManifestRevisionIndex [][]byte
On 2017/07/19 08:07:58, Ryan Tseng wrote:
> maybe "ManifestKey" is good enough?

Done.

https://codereview.chromium.org/2978293002/diff/1/milo/common/model/build_sum...
milo/common/model/build_summary.go:89: // ManifestRevisionIndexPrefix is a
binary prefix for querying
On 2017/07/19 08:07:58, Ryan Tseng wrote:
> Holy long names batman (not in the size of the name, but the number of
concepts
> a person needs to juggle in their head just from reading this variable name).
> 
> I'd say "ManifestRevisionIndexPrefix is an incomplete ManifestRevisionIndex
> missing a git revision.  This can be converted into a full
ManifestRevisionIndex
> by supplying a git revision using AddRevision()"
> 
> If this was named "ManifestKey", then "ManifestKeyPrefix" would sound a lot
more
> natural.

Done.

https://codereview.chromium.org/2978293002/diff/1/milo/frontend/view_console.go
File milo/frontend/view_console.go (right):

https://codereview.chromium.org/2978293002/diff/1/milo/frontend/view_console....
milo/frontend/view_console.go:84: // HACK(hinoka): This only supports
buildbot....
On 2017/07/19 08:07:58, Ryan Tseng wrote:
> no longer~

Done.

[commit-bot: I haz the power, 2017-07-20 00:20:06]

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

[commit-bot: I haz the power, 2017-07-20 00:20:07]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-07-20 00:26:42]

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

[commit-bot: I haz the power, 2017-07-20 00:26:45]

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-07-20 00:33:02]

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

[commit-bot: I haz the power, 2017-07-20 00:33:03]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-07-20 00:36:26]

lgtm

https://codereview.chromium.org/2978293002/diff/20001/milo/common/model/build...
File milo/common/model/build_summary.go (right):

https://codereview.chromium.org/2978293002/diff/20001/milo/common/model/build...
milo/common/model/build_summary.go:79: ManifestKey [][]byte
ManifestKeys

Consider type ManifestKey []byte, so ManifestKeys is type []ManifestKey

[iannucci, 2017-07-20 00:43:59]

https://codereview.chromium.org/2978293002/diff/20001/milo/common/model/build...
File milo/common/model/build_summary.go (right):

https://codereview.chromium.org/2978293002/diff/20001/milo/common/model/build...
milo/common/model/build_summary.go:79: ManifestKey [][]byte
On 2017/07/20 00:36:25, Ryan Tseng wrote:
> ManifestKeys
> 
> Consider type ManifestKey []byte, so ManifestKeys is type []ManifestKey

Done.

[iannucci, 2017-07-20 00:44:24]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-07-20 00:44:26]

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/2978293002/#ps60001
(title: "make manifestkey its own type")

[commit-bot: I haz the power, 2017-07-20 00:44:38]

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-07-20 00:52:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1500511464841560,
"parent_rev": "1291040682d23ae49ab28d32bfe6781136aedbf1", "commit_rev":
"52e6f7699476bd5518331dbe9600f211763be68a"}

[commit-bot: I haz the power, 2017-07-20 00:52:17]

Description was changed from

==========
[milo] Add an (uncached) method to get console rows.

Also plugs in git history call.

R=hinoka@chromium.org
BUG=468053
==========

to

==========
[milo] Add an (uncached) method to get console rows.

Also plugs in git history call.

R=hinoka@chromium.org
BUG=468053

Review-Url: https://codereview.chromium.org/2978293002
Committed:
https://github.com/luci/luci-go/commit/52e6f7699476bd5518331dbe9600f211763be68a
==========

[commit-bot: I haz the power, 2017-07-20 00:52:19]

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