[milo] initial call to get git history

[milo] initial call to get git history

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

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

[iannucci, 2017-07-17 19:24:44]

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

[commit-bot: I haz the power, 2017-07-17 19:24:55]

Dry run: CQ is trying da patch.

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

[tandrii(chromium), 2017-07-17 19:26:04]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2017-07-17 19:26:04]

drive-by: i'm adding paging to gitiles at this very moment, so don't do that :)

[iannucci, 2017-07-17 19:30:11]

On 2017/07/17 19:26:04, tandrii(chromium) wrote:
> drive-by: i'm adding paging to gitiles at this very moment, so don't do that
:)

paging based on git commit hashes? (please say yes)

[commit-bot: I haz the power, 2017-07-17 19:31:03]

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

[commit-bot: I haz the power, 2017-07-17 19:31:04]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-07-17 20:25:50]

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

[Ryan Tseng, 2017-07-17 20:25:51]

lgtm tho a more conventional pattern would've been preferred, this code might be
more interesting but it took longer to decipher :)

ie.

func getConsoleProto(...) {}

func cachedGetConsoleProto(...) {
  // Get from cache
  ...
  if notInCache {
    stuff = getConsoleProto()
    // put in cache
  }
}

func GetHistory(...) {
  if useCache {
    return getConsole()
  return cachedGetConsole()
}

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
File milo/api/proto/console_git_info.proto (right):

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
milo/api/proto/console_git_info.proto:1: // Copyright 2016 The LUCI Authors. All
rights reserved.
2017

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
milo/api/proto/console_git_info.proto:16: // The raw commit ID.
Is this usually the hash?

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go
File milo/git/history.go (right):

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode63
milo/git/history.go:63: func protoCache(c context.Context, enabled bool, key
string, out proto.Message, get func() error) error {
comments plz

Also this is fairly specific and internal function for this module, can we make
it return (*milo.ConsoleGitInfo, err) instead?  It makes the flow easier to
read.

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode113
milo/git/history.go:113: _, _ = wr.Write(data) // err is buffered on the writer
till Close
might as well skip assignment

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode115
milo/git/history.go:115: logging.WithError(err).Warningf(c, "gziping proto")
nit: gzipping

[iannucci, 2017-07-17 21:50:43]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-07-17 21:50:44]

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/2979153002/#ps20001
(title: "fix nits")

[commit-bot: I haz the power, 2017-07-17 21:50:54]

CQ is trying da patch.

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

[iannucci, 2017-07-17 21:54:39]

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
File milo/api/proto/console_git_info.proto (right):

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
milo/api/proto/console_git_info.proto:1: // Copyright 2016 The LUCI Authors. All
rights reserved.
On 2017/07/17 20:25:51, Ryan Tseng wrote:
> 2017

Done.

https://codereview.chromium.org/2979153002/diff/1/milo/api/proto/console_git_...
milo/api/proto/console_git_info.proto:16: // The raw commit ID.
On 2017/07/17 20:25:51, Ryan Tseng wrote:
> Is this usually the hash?

Done.

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go
File milo/git/history.go (right):

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode63
milo/git/history.go:63: func protoCache(c context.Context, enabled bool, key
string, out proto.Message, get func() error) error {
On 2017/07/17 20:25:51, Ryan Tseng wrote:
> comments plz
> 

Done.

> Also this is fairly specific and internal function for this module, can we
make
> it return (*milo.ConsoleGitInfo, err) instead?  It makes the flow easier to
> read.

No, I plan to use this for other proto caches as well.

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode113
milo/git/history.go:113: _, _ = wr.Write(data) // err is buffered on the writer
till Close
On 2017/07/17 20:25:51, Ryan Tseng wrote:
> might as well skip assignment

Done.

https://codereview.chromium.org/2979153002/diff/1/milo/git/history.go#newcode115
milo/git/history.go:115: logging.WithError(err).Warningf(c, "gziping proto")
On 2017/07/17 20:25:51, Ryan Tseng wrote:
> nit: gzipping

Done.

[commit-bot: I haz the power, 2017-07-17 21:59:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1500328243860080,
"parent_rev": "33f9af6122fbc8938f1208ad0cbd35a2ffcc929b", "commit_rev":
"b6f50320fa6dcc98867278fa297b5c4a300c026e"}

[commit-bot: I haz the power, 2017-07-17 21:59:53]

Description was changed from

==========
[milo] initial call to get git history

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

to

==========
[milo] initial call to get git history

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

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

[commit-bot: I haz the power, 2017-07-17 21:59:54]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-go/commit/b6f50320fa6dcc98867278fa297b5c4a300c026e